Iox #325 deadlock in shared mem mutex

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/locking_policy.hpp

@@ -29,7 +29,7 @@ class ThreadSafePolicy
     bool tryLock() const noexcept;
 
   private:
-    mutable posix::mutex m_mutex{true}; // recursive lock
+    mutable posix::mutex m_mutex{posix::mutex::Recursive::ON, posix::mutex::Robust::ON}; // recursive lock
 };
 
 class SingleThreadedPolicy

iceoryx_posh/include/iceoryx_posh/internal/popo/receiver_handler.hpp

@@ -35,7 +35,7 @@ class ThreadSafe
     void unlock();
 
   private:
-    mutex_t m_mutex{true}; // recursive lock
+    mutex_t m_mutex{mutex_t::Recursive::ON, mutex_t::Robust::ON};
 };
 
 class SingleThreaded

iceoryx_posh/include/iceoryx_posh/internal/popo/receiver_port_data.hpp

@@ -51,7 +51,7 @@ struct ReceiverPortData : public BasePortData
 
     // event callback related
     mutable std::atomic_bool m_chunkSendCallbackActive{false};
-    mutable mutex_t m_chunkSendCallbackMutex{false};
+    mutable mutex_t m_chunkSendCallbackMutex{mutex_t::Recursive::OFF, mutex_t::Robust::ON};
     iox::relative_ptr<posix::Semaphore> m_chunkSendSemaphore{nullptr};
 
     // offer semaphore that is stored in shared memory

iceoryx_utils/include/iceoryx_utils/internal/concurrent/locked_loffli.hpp

@@ -33,7 +33,7 @@ class LockedLoFFLi
     uint32_t* m_freeIndices{nullptr};
 
     using mutex_t = posix::mutex;
-    mutable mutex_t m_accessMutex{false};
+    mutable mutex_t m_accessMutex{mutex_t::Recursive::OFF, mutex_t::Robust::OFF};
 
     uint32_t m_invalidIndex{0};
 

iceoryx_utils/include/iceoryx_utils/internal/posix_wrapper/mutex.hpp

@@ -32,7 +32,8 @@ namespace posix
 ///     #include "iceoryx_utils/internal/posix_wrapper/mutex.hpp"
 ///
 ///     int main() {
-///         cxx::optional<posix::mutex> myMutex = posix::mutex::CreateMutex(false);
+///         cxx::optional<posix::mutex> myMutex = posix::mutex::CreateMutex(iox::posix::mutex::Recursive::OFF,
+///         iox::posix::mutex::Robust::OFF);
 ///
 ///         // always verify if the mutex could be created since we aren't
 ///         // throwing exceptions
@@ -53,42 +54,55 @@ namespace posix
 class mutex
 {
   public:
-    /// @brief the construction of the mutex can fail which will lead to a call
-    ///         to std::terminate which is alright for the moment since we are
-    ///         intending to get rid of the mutex sooner or later
-    mutex(const bool f_isRecursive);
+    enum class Recursive
+    {
+        ON,
+        OFF
+    };
+
+    enum class Robust
+    {
+        ON,
+        OFF
+    };
+
+    /// @brief The construction of the mutex can fail, which will lead to a call to std::terminate, which is alright
+    /// for the moment since we are intending to get rid of the mutex sooner or later.
+    /// @param[in] recursive Sets the recursive attribute of the mutex. If recursive is ON, the same thread, which has
+    /// already locked the mutex, can lock the mutex again without getting blocked.
+    /// @param[in] robust If robust is set ON, a process or thread can exit while having the lock without causing an
+    /// invalid mutex. In the next lock call to this mutex the system recognizes the mutex is locked by a dead process
+    /// or thread and allows the mutex to be restored.
+    mutex(const Recursive recursive, const Robust robust);
 
     ~mutex();
 
-    /// @brief all copy and move assignment methods need to be deleted otherwise
-    ///         undefined behavior or race conditions will occure if you copy
-    ///         or move mutexe when its possible that they are locked or will
-    ///         be locked
+    /// @brief all copy and move assignment methods need to be deleted otherwise  undefined behavior or race conditions
+    /// will occur if you copy or move mutex when its possible that they are locked or will be locked.
     mutex(const mutex&) = delete;
     mutex(mutex&&) = delete;
     mutex& operator=(const mutex&) = delete;
     mutex& operator=(mutex&&) = delete;
 
-    /// @brief Locks the mutex object and returns true if the underlying c
-    ///         function did not returned any error. If the mutex is already
-    ///         locked the method is blocking till the mutex can be locked.
+    /// @brief Locks the mutex object and returns true if the underlying c function did not returned any error. If the
+    /// mutex is already locked the method is blocking till the mutex can be locked.
     bool lock();
+
     /// @brief Unlocks the mutex object and returns true if the underlying c
     ///         function did not returned any error.
     ///        IMPORTANT! Unlocking and unlocked mutex is undefined behavior
     ///         and the underlying c function will report success in this case!
     bool unlock();
 
-    /// @brief  Tries to lock the mutex object. If it is not possible to lock
-    ///         the mutex object try_lock will return an error. If the c
-    ///         function fails it will return false, otherwise true.
+    /// @brief  Tries to lock the mutex object. If it is not possible to lock the mutex object try_lock will return an
+    /// error. If the c function fails it will return false, otherwise true.
     bool try_lock();
 
-    /// @brief  Returns the native handle which then can be used in
-    ///         pthread_mutex_** calls. Required when a pthread_mutex_**
-    ///         call is not abstracted with this wrapper.
+    /// @brief  Returns the native handle which then can be used in pthread_mutex_** calls. Required when a
+    /// pthread_mutex_** call is not abstracted with this wrapper.
     pthread_mutex_t get_native_handle() const noexcept;
 
+  private:
     pthread_mutex_t m_handle;
 };
 } // namespace posix

iceoryx_utils/source/posix_wrapper/mutex.cpp

@@ -23,7 +23,7 @@ namespace iox
 {
 namespace posix
 {
-mutex::mutex(bool f_isRecursive)
+mutex::mutex(const Recursive recursive, const Robust robust)
 {
     pthread_mutexattr_t attr;
     bool isInitialized{true};
@@ -41,7 +41,7 @@ mutex::mutex(bool f_isRecursive)
                                       {0},
                                       {},
                                       &attr,
-                                      f_isRecursive ? PTHREAD_MUTEX_RECURSIVE_NP : PTHREAD_MUTEX_FAST_NP)
+                                      recursive == Recursive::ON ? PTHREAD_MUTEX_RECURSIVE_NP : PTHREAD_MUTEX_FAST_NP)
                           .hasErrors();
     isInitialized &= !cxx::makeSmartC(pthread_mutexattr_setprotocol,
                                       cxx::ReturnMode::PRE_DEFINED_SUCCESS_CODE,
@@ -50,6 +50,16 @@ mutex::mutex(bool f_isRecursive)
                                       &attr,
                                       PTHREAD_PRIO_NONE)
                           .hasErrors();
+    if (robust == Robust::ON)
+    {
+        isInitialized &= !cxx::makeSmartC(pthread_mutexattr_setrobust,
+                                          cxx::ReturnMode::PRE_DEFINED_SUCCESS_CODE,
+                                          {0},
+                                          {},
+                                          &attr,
+                                          PTHREAD_MUTEX_ROBUST)
+                              .hasErrors();
+    }
 
     isInitialized &=
         !cxx::makeSmartC(pthread_mutex_init, cxx::ReturnMode::PRE_DEFINED_SUCCESS_CODE, {0}, {}, &m_handle, &attr)
@@ -69,10 +79,8 @@ mutex::~mutex()
 
     if (destroyCall.hasErrors())
     {
-        std::cerr << "could not destroy mutex ::: pthread_mutex_destroy returned " << destroyCall.getReturnValue()
-                  << " "
-                  << "( " << strerror(destroyCall.getReturnValue()) << ") " << std::endl;
-        std::terminate();
+        std::cerr << "Could not destroy mutex ::: pthread_mutex_destroy returned " << destroyCall.getReturnValue()
+                  << " (error: " << destroyCall.getErrorString() << "). This is a resource leak." << std::endl;
     }
 }
 
@@ -83,8 +91,21 @@ pthread_mutex_t mutex::get_native_handle() const noexcept
 
 bool mutex::lock()
 {
-    return !cxx::makeSmartC(pthread_mutex_lock, cxx::ReturnMode::PRE_DEFINED_SUCCESS_CODE, {0}, {}, &m_handle)
-                .hasErrors();
+    auto lockMutex =
+        cxx::makeSmartC(pthread_mutex_lock, cxx::ReturnMode::PRE_DEFINED_SUCCESS_CODE, {0, EOWNERDEAD}, {}, &m_handle);
+    if (lockMutex.getReturnValue() == EOWNERDEAD)
+    {
+        auto consistent =
+            cxx::makeSmartC(pthread_mutex_consistent, cxx::ReturnMode::PRE_DEFINED_SUCCESS_CODE, {0}, {}, &m_handle);
+        if (consistent.hasErrors())
+        {
+            std::cerr << "Could not make robust mutex consistent ::::  "
+                      << "pthread_mutex_consistent returned " << consistent.getReturnValue()
+                      << " (error: " << consistent.getErrorString() << ")." << std::endl;
+            return false;
+        }
+    }
+    return !lockMutex.hasErrors();
 }
 
 bool mutex::unlock()

iceoryx_utils/test/moduletests/test_posix_mutex.cpp

@@ -24,11 +24,12 @@ using namespace iox::units::duration_literals;
 class Mutex_test : public Test
 {
   public:
-    class MutexMock : public iox::posix::mutex
+    using mutex_t = iox::posix::mutex;
+    class MutexMock : public mutex_t
     {
       public:
-        MutexMock(const bool isRecursive)
-            : iox::posix::mutex(isRecursive)
+        MutexMock(const Recursive recursive, const Robust robust)
+            : mutex_t(recursive, robust)
         {
         }
     };
@@ -47,7 +48,7 @@ class Mutex_test : public Test
         }
     }
 
-    iox::posix::mutex sut{false};
+    mutex_t sut{mutex_t::Recursive::OFF, mutex_t::Robust::OFF};
 };
 
 TEST_F(Mutex_test, TryLockWithNoLock)
@@ -68,33 +69,3 @@ TEST_F(Mutex_test, LockAndUnlock)
     EXPECT_THAT(sut.lock(), Eq(true));
     EXPECT_THAT(sut.unlock(), Eq(true));
 }
-
-// in qnx you can destroy a locked mutex, without error if the thread holding the lock is destructing it.
-TEST_F(Mutex_test, DestructorFailsOnLockedMutex)
-{
-    std::string output = internal::GetCapturedStderr();
-    std::set_terminate([]() { std::cout << "", std::abort(); });
-
-    EXPECT_DEATH(
-        {
-            std::thread* t;
-            {
-                iox::posix::mutex mtx{false};
-                iox::posix::Timer hold(1000_ms);
-                t = new std::thread([&] {
-                    mtx.lock();
-                    iox::posix::Timer ct(5000_ms);
-                    while (!ct.hasExpiredComparedToCreationTime()) // come back in any case!
-                        ;
-                });
-
-                while (!hold.hasExpiredComparedToCreationTime())
-                    ;
-            }
-            t->join();
-            delete t;
-        },
-        ".*");
-
-    internal::CaptureStderr();
-}