Fix deadlock with security and timers. [11923] (#2034)

* Refs #11680. Fix deadlock with security and timers. Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Apply suggestions from code review Co-authored-by: Miguel Company <miguelcompany@eprosima.com> * Refs 11923. Linter fix Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
eProsima · Jul 5, 2021 · c0783e7 · c0783e7
1 parent 279a776
commit c0783e7
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 5 deletions.
diff --git a/src/cpp/rtps/security/SecurityManager.cpp b/src/cpp/rtps/security/SecurityManager.cpp
@@ -455,6 +455,10 @@ void SecurityManager::destroy()
 
         SecurityException exception;
 
+        // AuthUniquePtr must be removed after unlock SecurityManager's mutex.
+        // This is to avoid a deadlock with TimedEvents.
+        std::vector<DiscoveredParticipantInfo::AuthUniquePtr> auths_to_remove;
+
         for (auto& dp_it : discovered_participants_)
         {
             ParticipantCryptoHandle* participant_crypto_handle = dp_it.second.get_participant_crypto();
@@ -475,7 +479,9 @@ void SecurityManager::destroy()
                 authentication_plugin_->return_sharedsecret_handle(shared_secret_handle, exception);
             }
 
-            remove_discovered_participant_info(dp_it.second.get_auth());
+            DiscoveredParticipantInfo::AuthUniquePtr remote_participant_info = dp_it.second.get_auth();
+            remove_discovered_participant_info(remote_participant_info);
+            auths_to_remove.push_back(std::move(remote_participant_info));
         }
 
         discovered_participants_.clear();
@@ -500,6 +506,8 @@ void SecurityManager::destroy()
 
         mutex_.unlock();
 
+        auths_to_remove.clear();
+
         delete_entities();
 
         if (crypto_plugin_ != nullptr)
@@ -528,7 +536,7 @@ void SecurityManager::destroy()
 }
 
 void SecurityManager::remove_discovered_participant_info(
-        DiscoveredParticipantInfo::AuthUniquePtr&& auth_ptr)
+        const DiscoveredParticipantInfo::AuthUniquePtr& auth_ptr)
 {
     SecurityException exception;
 
@@ -562,7 +570,11 @@ bool SecurityManager::restore_discovered_participant_info(
     }
     else
     {
-        remove_discovered_participant_info(std::move(auth_ptr));
+        // AuthUniquePtr must be removed after unlock SecurityManager's mutex.
+        // This is to avoid a deadlock with TimedEvents.
+        DiscoveredParticipantInfo::AuthUniquePtr remote_participant_info = std::move(auth_ptr);
+        remove_discovered_participant_info(remote_participant_info);
+        lock.unlock();
     }
 
     return returned_value;
@@ -736,9 +748,13 @@ void SecurityManager::remove_participant(
             authentication_plugin_->return_sharedsecret_handle(shared_secret_handle, exception);
         }
 
-        remove_discovered_participant_info(dp_it->second.get_auth());
+        // AuthUniquePtr must be removed after unlock SecurityManager's mutex.
+        // This is to avoid a deadlock with TimedEvents.
+        DiscoveredParticipantInfo::AuthUniquePtr remote_participant_info = dp_it->second.get_auth();
+        remove_discovered_participant_info(remote_participant_info);
 
         discovered_participants_.erase(dp_it);
+        lock.unlock();
     }
 }
 

diff --git a/src/cpp/rtps/security/SecurityManager.h b/src/cpp/rtps/security/SecurityManager.h
@@ -426,7 +426,7 @@ class SecurityManager
     void cancel_init();
 
     void remove_discovered_participant_info(
-            DiscoveredParticipantInfo::AuthUniquePtr&& auth_ptr);
+            const DiscoveredParticipantInfo::AuthUniquePtr& auth_ptr);
 
     bool restore_discovered_participant_info(
             const GUID_t& remote_participant_key,