Fix segfault when creating two participant with same fixed id (#3443)

* Refs #18050. Add regression test Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #18050. Fix segfault Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Apply suggestions from code review Co-authored-by: Miguel Company <miguelcompany@eprosima.com> * Refs #18050. Apply suggestions Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #18050. improvement regression test Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Apply suggestions from code review Co-authored-by: Miguel Company <miguelcompany@eprosima.com> * Refs #18051. Apply suggestions. Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #18051. Fix mock Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #18051. Fix statistics mock Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #18051. Fix uncrustify Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #18051. Fix memory leak Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> --------- Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> Co-authored-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 3a168ed) # Conflicts: # src/cpp/rtps/RTPSDomain.cpp # src/cpp/rtps/RTPSDomainImpl.hpp
eProsima · Jun 27, 2023 · 59094af · 59094af
1 parent f09ff93
commit 59094af
Show file tree

Hide file tree

Showing 7 changed files with 145 additions and 25 deletions.
diff --git a/src/cpp/fastdds/domain/DomainParticipantFactory.cpp b/src/cpp/fastdds/domain/DomainParticipantFactory.cpp
@@ -166,30 +166,38 @@ DomainParticipant* DomainParticipantFactory::create_participant(
             new eprosima::fastdds::statistics::dds::DomainParticipantImpl(dom_part, did, pqos, listen);
 #endif // FASTDDS_STATISTICS
 
+    if (fastrtps::rtps::GUID_t::unknown() != dom_part_impl->guid())
     {
-        std::lock_guard<std::mutex> guard(mtx_participants_);
-        using VectorIt = std::map<DomainId_t, std::vector<DomainParticipantImpl*>>::iterator;
-        VectorIt vector_it = participants_.find(did);
-
-        if (vector_it == participants_.end())
         {
-            // Insert the vector
-            std::vector<DomainParticipantImpl*> new_vector;
-            auto pair_it = participants_.insert(std::make_pair(did, std::move(new_vector)));
-            vector_it = pair_it.first;
-        }
+            std::lock_guard<std::mutex> guard(mtx_participants_);
+            using VectorIt = std::map<DomainId_t, std::vector<DomainParticipantImpl*>>::iterator;
+            VectorIt vector_it = participants_.find(did);
 
-        vector_it->second.push_back(dom_part_impl);
-    }
+            if (vector_it == participants_.end())
+            {
+                // Insert the vector
+                std::vector<DomainParticipantImpl*> new_vector;
+                auto pair_it = participants_.insert(std::make_pair(did, std::move(new_vector)));
+                vector_it = pair_it.first;
+            }
 
-    if (factory_qos_.entity_factory().autoenable_created_entities)
-    {
-        if (ReturnCode_t::RETCODE_OK != dom_part->enable())
+            vector_it->second.push_back(dom_part_impl);
+        }
+
+        if (factory_qos_.entity_factory().autoenable_created_entities)
         {
-            delete_participant(dom_part);
-            return nullptr;
+            if (ReturnCode_t::RETCODE_OK != dom_part->enable())
+            {
+                delete_participant(dom_part);
+                return nullptr;
+            }
         }
     }
+    else
+    {
+        delete dom_part_impl;
+        return nullptr;
+    }
 
     return dom_part;
 }

diff --git a/src/cpp/fastdds/domain/DomainParticipantImpl.cpp b/src/cpp/fastdds/domain/DomainParticipantImpl.cpp
@@ -124,7 +124,10 @@ DomainParticipantImpl::DomainParticipantImpl(
 
     // Pre calculate participant id and generated guid
     participant_id_ = qos_.wire_protocol().participant_id;
-    eprosima::fastrtps::rtps::RTPSDomainImpl::create_participant_guid(participant_id_, guid_);
+    if (!eprosima::fastrtps::rtps::RTPSDomainImpl::create_participant_guid(participant_id_, guid_))
+    {
+        EPROSIMA_LOG_ERROR(DOMAIN_PARTICIPANT, "Error generating GUID for participant");
+    }
 
     /* Fill physical data properties if they are found and empty */
     std::string* property_value = fastrtps::rtps::PropertyPolicyHelper::find_property(
@@ -251,6 +254,8 @@ ReturnCode_t DomainParticipantImpl::enable()
 {
     // Should not have been previously enabled
     assert(get_rtps_participant() == nullptr);
+    // Should not have failed assigning the GUID
+    assert (guid_ != GUID_t::unknown());
 
     fastrtps::rtps::RTPSParticipantAttributes rtps_attr;
     utils::set_attributes_from_qos(rtps_attr, qos_);

diff --git a/src/cpp/rtps/RTPSDomain.cpp b/src/cpp/rtps/RTPSDomain.cpp
@@ -602,22 +602,48 @@ uint32_t RTPSDomainImpl::getNewId()
     return m_maxRTPSParticipantID++;
 }
 
-void RTPSDomainImpl::create_participant_guid(
+bool RTPSDomainImpl::reserve_participant_id(
+        int32_t& participant_id)
+{
+    std::lock_guard<std::mutex> guard(m_mutex);
+    if (participant_id < 0)
+    {
+        participant_id = getNewId();
+    }
+    else
+    {
+        if (m_RTPSParticipantIDs[participant_id].reserved == true)
+        {
+            return false;
+        }
+        m_RTPSParticipantIDs[participant_id].reserved = true;
+    }
+
+    return true;
+}
+
+bool RTPSDomainImpl::create_participant_guid(
         int32_t& participant_id,
         GUID_t& guid)
 {
-    if (participant_id < 0)
+    bool ret_value = get_instance()->reserve_participant_id(participant_id);
+
+    if (ret_value)
     {
+<<<<<<< HEAD
         auto instance = get_instance();
         std::lock_guard<std::mutex> guard(instance->m_mutex);
         do
         {
             participant_id = instance->getNewId();
         } while (instance->m_RTPSParticipantIDs.find(participant_id) != instance->m_RTPSParticipantIDs.end());
+=======
+        guid_prefix_create(participant_id, guid.guidPrefix);
+        guid.entityId = c_EntityId_RTPSParticipant;
+>>>>>>> 3a168ed6f (Fix segfault when creating two participant with same fixed id (#3443))
     }
 
-    guid_prefix_create(participant_id, guid.guidPrefix);
-    guid.entityId = c_EntityId_RTPSParticipant;
+    return ret_value;
 }
 
 RTPSParticipantImpl* RTPSDomainImpl::find_local_participant(

diff --git a/src/cpp/rtps/RTPSDomainImpl.hpp b/src/cpp/rtps/RTPSDomainImpl.hpp
@@ -158,8 +158,10 @@ class RTPSDomainImpl
      * @param [in, out] participant_id   Participant identifier for which to generate the GUID.
      *                                   When negative, it will be modified to the first non-existent participant id.
      * @param [out]     guid             GUID corresponding to participant_id
+     *
+     * @return True value if guid was created. False in other case.
      */
-    static void create_participant_guid(
+    static bool create_participant_guid(
             int32_t& participant_id,
             GUID_t& guid);
 
@@ -218,6 +220,26 @@ class RTPSDomainImpl
      */
     uint32_t getNewId();
 
+<<<<<<< HEAD
+=======
+    bool prepare_participant_id(
+            int32_t input_id,
+            uint32_t& participant_id);
+
+    /**
+     * Reserves a participant id.
+     * @param [in, out] participant_id   Participant identifier for reservation.
+     *                                   When negative, it will be modified to the first non-reserved participant id.
+     *
+     * @return True value if reservation was possible. False in other case.
+     */
+    bool reserve_participant_id(
+            int32_t& participant_id);
+
+    uint32_t get_id_for_prefix(
+            uint32_t participant_id);
+
+>>>>>>> 3a168ed6f (Fix segfault when creating two participant with same fixed id (#3443))
     void removeRTPSParticipant_nts(
             t_p_RTPSParticipant&);
 

diff --git a/test/mock/dds/DomainParticipantImpl/fastdds/domain/DomainParticipantImpl.hpp b/test/mock/dds/DomainParticipantImpl/fastdds/domain/DomainParticipantImpl.hpp
@@ -84,6 +84,7 @@ class DomainParticipantImpl
     {
         participant_->impl_ = this;
 
+        guid_.guidPrefix.value[11] = 1;
         eprosima::fastrtps::TopicAttributes top_attr;
         eprosima::fastrtps::xmlparser::XMLProfileManager::getDefaultTopicAttributes(top_attr);
         default_topic_qos_.history() = top_attr.historyQos;

diff --git a/test/mock/rtps/RTPSDomainImpl/rtps/RTPSDomainImpl.hpp b/test/mock/rtps/RTPSDomainImpl/rtps/RTPSDomainImpl.hpp
@@ -61,10 +61,12 @@ class RTPSDomainImpl
         return nullptr;
     }
 
-    static void create_participant_guid(
+    static bool create_participant_guid(
             int32_t& /*participant_id*/,
-            GUID_t& /*guid*/)
+            GUID_t& guid)
     {
+        guid.guidPrefix.value[11] = 1;
+        return true;
     }
 
     /**

diff --git a/test/unittest/dds/participant/ParticipantTests.cpp b/test/unittest/dds/participant/ParticipantTests.cpp
@@ -3792,6 +3792,62 @@ TEST(ParticipantTests, UnsupportedMethods)
     ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(participant), ReturnCode_t::RETCODE_OK);
 }
 
+/*
+ * Regression test for redmine issue #18050.
+ *
+ * This test tries to create two participants with the same fixed id.
+ */
+TEST(ParticipantTests, TwoParticipantWithSameFixedId)
+{
+    // Test participants enabled from beginning
+    {
+        DomainParticipantQos participant_qos;
+        participant_qos.wire_protocol().participant_id = 1;
+
+        // Create the first participant
+        DomainParticipant* participant1 =
+                DomainParticipantFactory::get_instance()->create_participant(0, participant_qos);
+        ASSERT_NE(participant1, nullptr);
+
+        // Creating a second participant with the same fixed id should fail
+        DomainParticipant* participant2 =
+                DomainParticipantFactory::get_instance()->create_participant(0, participant_qos);
+        ASSERT_EQ(participant2, nullptr);
+
+        // Destroy the first participant
+        ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(participant1), ReturnCode_t::RETCODE_OK);
+    }
+
+    // Test participants disabled from beginning
+    {
+        DomainParticipantFactoryQos factory_qos;
+        ASSERT_EQ(ReturnCode_t::RETCODE_OK, DomainParticipantFactory::get_instance()->get_qos(factory_qos));
+        factory_qos.entity_factory().autoenable_created_entities = false;
+        ASSERT_EQ(ReturnCode_t::RETCODE_OK, DomainParticipantFactory::get_instance()->set_qos(factory_qos));
+
+        DomainParticipantQos participant_qos;
+        participant_qos.wire_protocol().participant_id = 1;
+
+        // Create the first participant
+        DomainParticipant* participant1 =
+                DomainParticipantFactory::get_instance()->create_participant(0, participant_qos);
+        ASSERT_NE(participant1, nullptr);
+
+        // Creating a second participant with the same fixed id should fail
+        DomainParticipant* participant2 =
+                DomainParticipantFactory::get_instance()->create_participant(0, participant_qos);
+        ASSERT_EQ(participant2, nullptr);
+
+        ASSERT_EQ(ReturnCode_t::RETCODE_OK, participant1->enable());
+
+        // Destroy the first participant
+        ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(participant1), ReturnCode_t::RETCODE_OK);
+
+        factory_qos.entity_factory().autoenable_created_entities = true;
+        ASSERT_EQ(ReturnCode_t::RETCODE_OK, DomainParticipantFactory::get_instance()->set_qos(factory_qos));
+    }
+}
+
 } // namespace dds
 } // namespace fastdds
 } // namespace eprosima