Fix data race in PDPListener and SecurityManager (#4188)

* Refs #20166: Regression Test

Signed-off-by: Mario Dominguez <[email protected]>

* Refs #20166: Use the participant proxy data stored the collection in SecurityManager::discovered_participant()

Signed-off-by: Mario Dominguez <[email protected]>

* Refs #20166: Rev suggestion

Signed-off-by: Mario Dominguez <[email protected]>

* Refs #20166: Test Rev suggestion

Signed-off-by: Mario Dominguez <[email protected]>

---------

Signed-off-by: Mario Dominguez <[email protected]>
(cherry picked from commit 84334da)

Author: Mario-DL

src/cpp/rtps/builtin/discovery/participant/DS/PDPSecurityInitiatorListener.cpp

@@ -71,15 +71,16 @@ void PDPSecurityInitiatorListener::process_alive_data(
 
     if (old_data == nullptr)
     {
+        auto callback_data = new_data;
         reader->getMutex().unlock();
         lock.unlock();
 
         //! notify security manager in order to start handshake
-        bool ret = parent_pdp_->getRTPSParticipant()->security_manager().discovered_participant(new_data);
+        bool ret = parent_pdp_->getRTPSParticipant()->security_manager().discovered_participant(callback_data);
         //! Reply to the remote participant
         if (ret)
         {
-            response_cb_(temp_participant_data_);
+            response_cb_(callback_data);
         }
 
         // Take again the reader lock

src/cpp/rtps/security/SecurityManager.cpp

@@ -601,6 +601,8 @@ bool SecurityManager::discovered_participant(
     // Create or find information
     bool undiscovered = false;
     DiscoveredParticipantInfo::AuthUniquePtr remote_participant_info;
+    // Use the information from the collection
+    const ParticipantProxyData* remote_participant_data = nullptr;
     {
         std::lock_guard<shared_mutex> _(mutex_);
 
@@ -614,13 +616,14 @@ bool SecurityManager::discovered_participant(
 
         undiscovered = map_ret.second;
         remote_participant_info = map_ret.first->second->get_auth();
+        remote_participant_data = &map_ret.first->second->participant_data();
     }
 
     bool notify_part_authorized = false;
-    if (undiscovered && remote_participant_info)
+    if (undiscovered && remote_participant_info && remote_participant_data != nullptr)
     {
         // Configure the timed event but do not start it
-        const GUID_t guid = participant_data.m_guid;
+        const GUID_t guid = remote_participant_data->m_guid;
         remote_participant_info->event_.reset(new TimedEvent(participant_->getEventResource(),
                 [&, guid]() -> bool
                 {
@@ -634,8 +637,8 @@ bool SecurityManager::discovered_participant(
         // Validate remote participant.
         ValidationResult_t validation_ret = authentication_plugin_->validate_remote_identity(&remote_identity_handle,
                         *local_identity_handle_,
-                        participant_data.identity_token_,
-                        participant_data.m_guid, exception);
+                        remote_participant_data->identity_token_,
+                        remote_participant_data->m_guid, exception);
 
         switch (validation_ret)
         {
@@ -655,21 +658,21 @@ bool SecurityManager::discovered_participant(
             // TODO(Ricardo) Send event.
             default:
 
-                on_validation_failed(participant_data, exception);
+                on_validation_failed(*remote_participant_data, exception);
 
                 std::lock_guard<shared_mutex> _(mutex_);
 
                 // Remove created element, because authentication failed.
-                discovered_participants_.erase(participant_data.m_guid);
+                discovered_participants_.erase(remote_participant_data->m_guid);
 
                 //TODO(Ricardo) cryptograhy registration in AUTHENTICAITON_OK
                 return false;
         }
 
-        EPROSIMA_LOG_INFO(SECURITY, "Discovered participant " << participant_data.m_guid);
+        EPROSIMA_LOG_INFO(SECURITY, "Discovered participant " << remote_participant_data->m_guid);
 
         // Match entities
-        match_builtin_endpoints(participant_data);
+        match_builtin_endpoints(*remote_participant_data);
 
         // Store new remote handle.
         remote_participant_info->auth_status_ = auth_status;
@@ -681,7 +684,7 @@ bool SecurityManager::discovered_participant(
         {
             //TODO(Ricardo) Shared secret on this case?
             std::shared_ptr<SecretHandle> ss;
-            notify_part_authorized = participant_authorized(participant_data, remote_participant_info, ss);
+            notify_part_authorized = participant_authorized(*remote_participant_data, remote_participant_info, ss);
         }
     }
     else
@@ -704,15 +707,15 @@ bool SecurityManager::discovered_participant(
     if (remote_participant_info->auth_status_ == AUTHENTICATION_REQUEST_NOT_SEND)
     {
         // Maybe send request.
-        returnedValue = on_process_handshake(participant_data, remote_participant_info,
+        returnedValue = on_process_handshake(*remote_participant_data, remote_participant_info,
                         MessageIdentity(), HandshakeMessageToken(), notify_part_authorized);
     }
 
-    restore_discovered_participant_info(participant_data.m_guid, remote_participant_info);
+    restore_discovered_participant_info(remote_participant_data->m_guid, remote_participant_info);
 
     if (notify_part_authorized)
     {
-        notify_participant_authorized(participant_data);
+        notify_participant_authorized(*remote_participant_data);
     }
 
     return returnedValue;

test/blackbox/common/BlackboxTestsSecurity.cpp

@@ -3848,6 +3848,56 @@ TEST(Security, AllowUnauthenticatedParticipants_TwoParticipantsDifferentCertific
     ASSERT_FALSE(writer.is_matched());
 }
 
+// Regresion test for redmine issue 20166
+TEST(Security, InANonSecureParticipantWithTwoSecureParticipantScenario_TheTwoSecureParticipantsCorrectlyCommunicate)
+{
+    // Create
+    PubSubReader<HelloWorldPubSubType> non_secure_reader("HelloWorldTopic");
+    PubSubReader<HelloWorldPubSubType> secure_reader("HelloWorldTopic");
+    PubSubWriter<HelloWorldPubSubType> secure_writer("HelloWorldTopic");
+
+    // Configure security
+    const std::string governance_file("governance_helloworld_all_enable.smime");
+    const std::string permissions_file("permissions_helloworld.smime");
+    CommonPermissionsConfigure(secure_reader, secure_writer, governance_file, permissions_file);
+
+    secure_writer.history_depth(10).
+            reliability(eprosima::fastrtps::RELIABLE_RELIABILITY_QOS).init();
+
+    ASSERT_TRUE(secure_writer.isInitialized());
+
+    non_secure_reader.history_depth(10).
+            reliability(eprosima::fastrtps::RELIABLE_RELIABILITY_QOS).init();
+
+    ASSERT_TRUE(non_secure_reader.isInitialized());
+
+    secure_reader.history_depth(10).
+            reliability(eprosima::fastrtps::RELIABLE_RELIABILITY_QOS).init();
+
+    ASSERT_TRUE(secure_reader.isInitialized());
+
+    // Wait for the authorization
+    secure_reader.waitAuthorized();
+    secure_writer.waitAuthorized();
+
+    // Wait for discovery
+    secure_writer.wait_discovery(std::chrono::seconds(5));
+    secure_reader.wait_discovery(std::chrono::seconds(5));
+
+    // Data is correctly sent and received
+    auto data = default_helloworld_data_generator();
+
+    secure_reader.startReception(data);
+
+    secure_writer.send(data);
+
+    // In this test all data should be sent.
+    ASSERT_TRUE(data.empty());
+
+    secure_reader.block_for_all();
+    EXPECT_EQ(non_secure_reader.getReceivedCount(), 0u);
+}
+
 // *INDENT-OFF*
 TEST_P(Security, BuiltinAuthenticationAndAccessAndCryptoPlugin_PermissionsDisableDiscoveryDisableAccessEncrypt_validation_ok_enable_discovery_enable_access_encrypt)
 // *INDENT-ON*