Enforce SHM ports open mode exclusions (#4635) (#4646)

* Enforce SHM ports open mode exclusions (#4635) * Refs #20701. Added unit regression tests. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #20701. Added blackbox regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #20701. Fix issue. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 3d159dc) # Conflicts: # test/blackbox/common/BlackboxTestsTransportSHM.cpp * Fix conflicts Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Fix build on unit test Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Fix build on unit test Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
eProsima · Apr 12, 2024 · 54090bf · 54090bf
1 parent dbdc7a9
commit 54090bf
Show file tree

Hide file tree

Showing 3 changed files with 117 additions and 1 deletion.
diff --git a/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp b/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp
@@ -898,12 +898,22 @@ class SharedMemGlobal
 
         void lock_read_exclusive()
         {
+            if (OpenMode::ReadShared == open_mode())
+            {
+                throw std::runtime_error("port is opened ReadShared");
+            }
+
             std::string lock_name = std::string(node_->domain_name) + "_port" + std::to_string(node_->port_id) + "_el";
             read_exclusive_lock_ = std::unique_ptr<RobustExclusiveLock>(new RobustExclusiveLock(lock_name));
         }
 
         void lock_read_shared()
         {
+            if (OpenMode::ReadExclusive == open_mode())
+            {
+                throw std::runtime_error("port is opened ReadExclusive");
+            }
+
             std::string lock_name = std::string(node_->domain_name) + "_port" + std::to_string(node_->port_id) + "_sl";
             read_shared_lock_ = std::unique_ptr<RobustSharedLock>(new RobustSharedLock(lock_name));
         }
@@ -1124,7 +1134,24 @@ class SharedMemGlobal
                         std::stringstream ss;
 
                         ss << port_node->port_id << " (" << port_node->uuid.to_string() <<
-                            ") because is ReadExclusive locked";
+                            ") because it was already locked";
+
+                        err_reason = ss.str();
+                        port.reset();
+                    }
+                }
+                else if (open_mode == Port::OpenMode::ReadShared)
+                {
+                    try
+                    {
+                        port->lock_read_shared();
+                    }
+                    catch (const std::exception&)
+                    {
+                        std::stringstream ss;
+
+                        ss << port_node->port_id << " (" << port_node->uuid.to_string() <<
+                            ") because it had a ReadExclusive lock";
 
                         err_reason = ss.str();
                         port.reset();

diff --git a/test/blackbox/common/BlackboxTestsTransportSHM.cpp b/test/blackbox/common/BlackboxTestsTransportSHM.cpp
@@ -16,6 +16,7 @@
 
 #include "BlackboxTests.hpp"
 
+#include "PubSubParticipant.hpp"
 #include "PubSubReader.hpp"
 #include "PubSubWriter.hpp"
 
@@ -28,6 +29,8 @@ using namespace eprosima::fastrtps;
 
 using SharedMemTransportDescriptor = eprosima::fastdds::rtps::SharedMemTransportDescriptor;
 using test_SharedMemTransportDescriptor = eprosima::fastdds::rtps::test_SharedMemTransportDescriptor;
+using Locator = eprosima::fastdds::rtps::Locator;
+using LocatorList = eprosima::fastdds::rtps::LocatorList;
 
 TEST(SHM, TransportPubSub)
 {
@@ -70,6 +73,58 @@ TEST(SHM, TransportPubSub)
     reader.wait_participant_undiscovery();
 }
 
+/* Regression test for redmine issue #20701
+ *
+ * This test checks that the SHM transport will not listen on the same port
+ * in unicast and multicast at the same time.
+ * It does so by specifying custom default locators on a DataReader and then
+ * checking that the port mutation took place, thus producing a different port.
+ */
+TEST(SHM, SamePortUnicastMulticast)
+{
+    PubSubReader<HelloWorldPubSubType> participant(TEST_TOPIC_NAME);
+
+    Locator locator;
+    locator.kind = LOCATOR_KIND_SHM;
+    locator.port = global_port;
+
+    LocatorList unicast_list;
+    LocatorList multicast_list;
+
+    // Note: this is using knowledge of the SHM locator address format since
+    // SHMLocator is not exposed to the user.
+    locator.address[0] = 'U';
+    unicast_list.push_back(locator);
+
+    // Note: this is using knowledge of the SHM locator address format since
+    // SHMLocator is not exposed to the user.
+    locator.address[0] = 'M';
+    multicast_list.push_back(locator);
+
+    // Create the reader with the custom transport and locators
+    auto testTransport = std::make_shared<SharedMemTransportDescriptor>();
+    participant
+            .disable_builtin_transport()
+            .add_user_transport_to_pparams(testTransport)
+            .set_default_unicast_locators(unicast_list)
+            .set_default_multicast_locators(multicast_list)
+            .init();
+
+    ASSERT_TRUE(participant.isInitialized());
+
+    // Retrieve the listening locators and check that one port is different
+    LocatorList reader_locators;
+    participant.get_native_reader().get_listening_locators(reader_locators);
+
+    ASSERT_EQ(reader_locators.size(), 2u);
+    auto it = reader_locators.begin();
+    auto first_port = it->port;
+    ++it;
+    auto second_port = it->port;
+    EXPECT_NE(first_port, second_port);
+    EXPECT_TRUE(first_port == global_port || second_port == global_port);
+}
+
 TEST(SHM, Test300KFragmentation)
 {
     PubSubReader<Data1mbPubSubType> reader(TEST_TOPIC_NAME);

diff --git a/test/unittest/transport/SharedMemTests.cpp b/test/unittest/transport/SharedMemTests.cpp
@@ -1013,6 +1013,40 @@ TEST_F(SHMTransportTests, port_lock_read_exclusive)
     port = shared_mem_manager->open_port(0, 1, 1000, SharedMemGlobal::Port::OpenMode::ReadExclusive);
 }
 
+// Regression test for redmine issue #20701
+TEST_F(SHMTransportTests, port_lock_read_shared_then_exclusive)
+{
+    const std::string domain_name("SHMTests");
+
+    auto shared_mem_manager = SharedMemManager::create(domain_name);
+
+    shared_mem_manager->remove_port(0);
+
+    auto port = shared_mem_manager->open_port(0, 1, 1000, SharedMemGlobal::Port::OpenMode::ReadShared);
+    ASSERT_THROW(shared_mem_manager->open_port(0, 1, 1000, SharedMemGlobal::Port::OpenMode::ReadExclusive),
+            std::exception);
+
+    port.reset();
+    port = shared_mem_manager->open_port(0, 1, 1000, SharedMemGlobal::Port::OpenMode::ReadExclusive);
+}
+
+// Regression test for redmine issue #20701
+TEST_F(SHMTransportTests, port_lock_read_exclusive_then_shared)
+{
+    const std::string domain_name("SHMTests");
+
+    auto shared_mem_manager = SharedMemManager::create(domain_name);
+
+    shared_mem_manager->remove_port(0);
+
+    auto port = shared_mem_manager->open_port(0, 1, 1000, SharedMemGlobal::Port::OpenMode::ReadExclusive);
+    ASSERT_THROW(shared_mem_manager->open_port(0, 1, 1000, SharedMemGlobal::Port::OpenMode::ReadShared),
+            std::exception);
+
+    port.reset();
+    port = shared_mem_manager->open_port(0, 1, 1000, SharedMemGlobal::Port::OpenMode::ReadShared);
+}
+
 TEST_F(SHMTransportTests, robust_exclusive_lock)
 {
     const std::string lock_name = "robust_exclusive_lock_test1_el";