Fix liveliness core dump caused by unprotected code (#2828)

* fix: liveliness core dump caused by omit mutex lock * Refactor that includes the fix and speeds things up Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Fixing Clang warnings Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> Co-authored-by: jim <jim@jim-ubuntu> Co-authored-by: Miguel Barro <miguelbarro@eprosima.com>
eProsima · Jul 14, 2022 · 796176a · 796176a
1 parent b5f87dd
commit 796176a
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 17 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -62,7 +62,8 @@ if(MSVC OR MSVC_IDE)
     # C4555  expression has no effect; expected expression with side-effect
     # C4715: 'Test': not all control paths return a value
     # C5038 data member 'member1' will be initialized after data member 'member2'
-    add_compile_options(/w34101 /w34189 /w34555 /w34715 /w35038)
+    # C4100 'identifier' : unreferenced formal parameter (matches clang -Wunused-lambda-capture)
+    add_compile_options(/w34101 /w34189 /w34555 /w34715 /w35038 /w44100)
 
     if(EPROSIMA_BUILD)
         string(REPLACE "/DNDEBUG" "" CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO}")

diff --git a/src/cpp/rtps/writer/LivelinessManager.cpp b/src/cpp/rtps/writer/LivelinessManager.cpp
@@ -33,7 +33,7 @@ LivelinessManager::LivelinessManager(
 
 LivelinessManager::~LivelinessManager()
 {
-    std::unique_lock<std::mutex> lock(mutex_);
+    std::lock_guard<std::mutex> _(mutex_);
     timer_owner_ = nullptr;
     timer_.cancel_timer();
 }
@@ -52,12 +52,11 @@ bool LivelinessManager::add_writer(
     {
         // collection guard
         std::lock_guard<shared_mutex> _(col_mutex_);
+        // writers_ elements guard
+        std::lock_guard<std::mutex> __(mutex_);
 
         for (LivelinessData& writer : writers_)
         {
-            // writers_ elements guard
-            std::lock_guard<std::mutex> __(mutex_);
-
             if (writer.guid == guid &&
                     writer.kind == kind &&
                     writer.lease_duration == lease_duration)
@@ -101,11 +100,11 @@ bool LivelinessManager::remove_writer(
     {
         // collection guard
         std::lock_guard<shared_mutex> _(col_mutex_);
+        // writers_ elements guard
+        std::lock_guard<std::mutex> __(mutex_);
 
-        removed = writers_.remove_if([guid, kind, lease_duration, &status, this](LivelinessData& writer)
+        removed = writers_.remove_if([guid, kind, lease_duration, &status](LivelinessData& writer)
                         {
-                            // writers_ elements guard
-                            std::lock_guard<std::mutex> _(mutex_);
                             status = writer.status;
                             return writer.guid == guid &&
                             writer.kind == kind &&
@@ -288,20 +287,18 @@ bool LivelinessManager::assert_liveliness(
 
 bool LivelinessManager::calculate_next()
 {
-    timer_owner_ = nullptr;
+    // Keep this lock order to prevent ABBA deadlocks
+    shared_lock<shared_mutex> _(col_mutex_);
+    std::lock_guard<std::mutex> __(mutex_);
 
+    bool any_alive = false;
     steady_clock::time_point min_time = steady_clock::now() + nanoseconds(c_TimeInfinite.to_ns());
 
-    bool any_alive = false;
+    timer_owner_ = nullptr;
 
     // collection guard
-    shared_lock<shared_mutex> _(col_mutex_);
-
     for (LivelinessData& writer : writers_)
     {
-        // writers_ elements guard
-        std::lock_guard<std::mutex> __(mutex_);
-
         if (writer.status == LivelinessData::WriterStatus::ALIVE)
         {
             if (writer.time < min_time)
@@ -362,12 +359,12 @@ bool LivelinessManager::timer_expired()
 bool LivelinessManager::is_any_alive(
         LivelinessQosPolicyKind kind)
 {
+    // Keep this lock order to prevent ABBA deadlocks
     shared_lock<shared_mutex> _(col_mutex_);
+    std::lock_guard<std::mutex> __(mutex_);
 
     for (const auto& writer : writers_)
     {
-        std::unique_lock<std::mutex> lock(mutex_);
-
         if (writer.kind == kind && writer.status == LivelinessData::WriterStatus::ALIVE)
         {
             return true;