From c5cd2256218f1dbeccca7a32b2f2fce8bbdc0514 Mon Sep 17 00:00:00 2001 From: Eduardo Ponz Segrelles Date: Tue, 19 Mar 2024 19:52:46 +0100 Subject: [PATCH] Explicitly pass vendor ID to readFromCdrMessage (#4583) * Refs #20641: Add regression test Signed-off-by: EduPonz * Refs #20641: Explicitely pass vendor ID to all readFromCdrMessage calls Signed-off-by: EduPonz * Refs #20641: Treat unkown vendor IDs as ours Signed-off-by: EduPonz --------- Signed-off-by: EduPonz --- .../builtin/data/ParticipantProxyData.cpp | 10 ++- src/cpp/rtps/builtin/data/ReaderProxyData.cpp | 35 ++++++-- src/cpp/rtps/builtin/data/WriterProxyData.cpp | 47 +++++++--- .../discovery/endpoint/EDPSimpleListeners.cpp | 4 +- .../discovery/participant/PDPListener.cpp | 2 +- .../participant/PDPServerListener.cpp | 3 +- .../rtps/participant/RTPSParticipantImpl.cpp | 9 +- .../common/BlackboxTestsDiscovery.cpp | 80 +++++++++++++++++- .../common/BlackboxTestsTransportUDP.cpp | 27 +----- .../common/DatagramInjectionTransport.hpp | 22 +++++ test/blackbox/datagrams/20641.bin | Bin 0 -> 384 bytes 11 files changed, 187 insertions(+), 52 deletions(-) create mode 100644 test/blackbox/datagrams/20641.bin diff --git a/src/cpp/rtps/builtin/data/ParticipantProxyData.cpp b/src/cpp/rtps/builtin/data/ParticipantProxyData.cpp index db13b6d3085..17475f93718 100644 --- a/src/cpp/rtps/builtin/data/ParticipantProxyData.cpp +++ b/src/cpp/rtps/builtin/data/ParticipantProxyData.cpp @@ -468,11 +468,17 @@ bool ParticipantProxyData::readFromCDRMessage( } case fastdds::dds::PID_NETWORK_CONFIGURATION_SET: { + VendorId_t local_vendor_id = source_vendor_id; + if (c_VendorId_Unknown == local_vendor_id) + { + local_vendor_id = ((c_VendorId_Unknown == m_VendorId) ? c_VendorId_eProsima : m_VendorId); + } + // Ignore custom PID when coming from other vendors - if (c_VendorId_eProsima != source_vendor_id) + if (c_VendorId_eProsima != local_vendor_id) { EPROSIMA_LOG_INFO(RTPS_PROXY_DATA, - "Ignoring custom PID" << pid << " from vendor " << source_vendor_id); + "Ignoring custom PID" << pid << " from vendor " << local_vendor_id); return true; } diff --git a/src/cpp/rtps/builtin/data/ReaderProxyData.cpp b/src/cpp/rtps/builtin/data/ReaderProxyData.cpp index 9591bbe697d..9596be161d3 100644 --- a/src/cpp/rtps/builtin/data/ReaderProxyData.cpp +++ b/src/cpp/rtps/builtin/data/ReaderProxyData.cpp @@ -644,6 +644,8 @@ bool ReaderProxyData::readFromCDRMessage( auto param_process = [this, &network, &is_shm_transport_available, &should_filter_locators, source_vendor_id]( CDRMessage_t* msg, const ParameterId_t& pid, uint16_t plength) { + VendorId_t vendor_id = c_VendorId_Unknown; + switch (pid) { case fastdds::dds::PID_VENDORID: @@ -656,6 +658,7 @@ bool ReaderProxyData::readFromCDRMessage( } is_shm_transport_available &= (p.vendorId == c_VendorId_eProsima); + vendor_id = p.vendorId; break; } case fastdds::dds::PID_DURABILITY: @@ -844,11 +847,17 @@ bool ReaderProxyData::readFromCDRMessage( } case fastdds::dds::PID_NETWORK_CONFIGURATION_SET: { + VendorId_t local_vendor_id = source_vendor_id; + if (c_VendorId_Unknown == local_vendor_id) + { + local_vendor_id = ((c_VendorId_Unknown == vendor_id) ? c_VendorId_eProsima : vendor_id); + } + // Ignore custom PID when coming from other vendors - if (c_VendorId_eProsima != source_vendor_id) + if (c_VendorId_eProsima != local_vendor_id) { EPROSIMA_LOG_INFO(RTPS_PROXY_DATA, - "Ignoring custom PID" << pid << " from vendor " << source_vendor_id); + "Ignoring custom PID" << pid << " from vendor " << local_vendor_id); return true; } @@ -991,12 +1000,18 @@ bool ReaderProxyData::readFromCDRMessage( case fastdds::dds::PID_DISABLE_POSITIVE_ACKS: { + VendorId_t local_vendor_id = source_vendor_id; + if (c_VendorId_Unknown == local_vendor_id) + { + local_vendor_id = ((c_VendorId_Unknown == vendor_id) ? c_VendorId_eProsima : vendor_id); + } + // Ignore custom PID when coming from other vendors except RTI Connext - if ((c_VendorId_eProsima != source_vendor_id) && - (fastdds::rtps::c_VendorId_rti_connext != source_vendor_id)) + if ((c_VendorId_eProsima != local_vendor_id) && + (fastdds::rtps::c_VendorId_rti_connext != local_vendor_id)) { EPROSIMA_LOG_INFO(RTPS_PROXY_DATA, - "Ignoring custom PID" << pid << " from vendor " << source_vendor_id); + "Ignoring custom PID" << pid << " from vendor " << local_vendor_id); return true; } @@ -1044,11 +1059,17 @@ bool ReaderProxyData::readFromCDRMessage( case fastdds::dds::PID_DATASHARING: { + VendorId_t local_vendor_id = source_vendor_id; + if (c_VendorId_Unknown == local_vendor_id) + { + local_vendor_id = ((c_VendorId_Unknown == vendor_id) ? c_VendorId_eProsima : vendor_id); + } + // Ignore custom PID when coming from other vendors - if (c_VendorId_eProsima != source_vendor_id) + if (c_VendorId_eProsima != local_vendor_id) { EPROSIMA_LOG_INFO(RTPS_PROXY_DATA, - "Ignoring custom PID" << pid << " from vendor " << source_vendor_id); + "Ignoring custom PID" << pid << " from vendor " << local_vendor_id); return true; } diff --git a/src/cpp/rtps/builtin/data/WriterProxyData.cpp b/src/cpp/rtps/builtin/data/WriterProxyData.cpp index daff3042da2..2ed6f46daf2 100644 --- a/src/cpp/rtps/builtin/data/WriterProxyData.cpp +++ b/src/cpp/rtps/builtin/data/WriterProxyData.cpp @@ -631,6 +631,8 @@ bool WriterProxyData::readFromCDRMessage( auto param_process = [this, &network, &is_shm_transport_available, &should_filter_locators, source_vendor_id]( CDRMessage_t* msg, const ParameterId_t& pid, uint16_t plength) { + VendorId_t vendor_id = c_VendorId_Unknown; + switch (pid) { case fastdds::dds::PID_VENDORID: @@ -643,6 +645,7 @@ bool WriterProxyData::readFromCDRMessage( } is_shm_transport_available &= (p.vendorId == c_VendorId_eProsima); + vendor_id = p.vendorId; break; } case fastdds::dds::PID_DURABILITY: @@ -838,12 +841,18 @@ bool WriterProxyData::readFromCDRMessage( } case fastdds::dds::PID_PERSISTENCE_GUID: { + VendorId_t local_vendor_id = source_vendor_id; + if (c_VendorId_Unknown == local_vendor_id) + { + local_vendor_id = ((c_VendorId_Unknown == vendor_id) ? c_VendorId_eProsima : vendor_id); + } + // Ignore custom PID when coming from other vendors except RTI Connext - if ((c_VendorId_eProsima != source_vendor_id) && - (fastdds::rtps::c_VendorId_rti_connext != source_vendor_id)) + if ((c_VendorId_eProsima != local_vendor_id) && + (fastdds::rtps::c_VendorId_rti_connext != local_vendor_id)) { EPROSIMA_LOG_INFO(RTPS_PROXY_DATA, - "Ignoring custom PID" << pid << " from vendor " << source_vendor_id); + "Ignoring custom PID" << pid << " from vendor " << local_vendor_id); return true; } @@ -858,11 +867,17 @@ bool WriterProxyData::readFromCDRMessage( } case fastdds::dds::PID_NETWORK_CONFIGURATION_SET: { + VendorId_t local_vendor_id = source_vendor_id; + if (c_VendorId_Unknown == local_vendor_id) + { + local_vendor_id = ((c_VendorId_Unknown == vendor_id) ? c_VendorId_eProsima : vendor_id); + } + // Ignore custom PID when coming from other vendors - if (c_VendorId_eProsima != source_vendor_id) + if (c_VendorId_eProsima != local_vendor_id) { EPROSIMA_LOG_INFO(RTPS_PROXY_DATA, - "Ignoring custom PID" << pid << " from vendor " << source_vendor_id); + "Ignoring custom PID" << pid << " from vendor " << local_vendor_id); return true; } @@ -973,12 +988,18 @@ bool WriterProxyData::readFromCDRMessage( } case fastdds::dds::PID_DISABLE_POSITIVE_ACKS: { + VendorId_t local_vendor_id = source_vendor_id; + if (c_VendorId_Unknown == local_vendor_id) + { + local_vendor_id = ((c_VendorId_Unknown == vendor_id) ? c_VendorId_eProsima : vendor_id); + } + // Ignore custom PID when coming from other vendors except RTI Connext - if ((c_VendorId_eProsima != source_vendor_id) && - (fastdds::rtps::c_VendorId_rti_connext != source_vendor_id)) + if ((c_VendorId_eProsima != local_vendor_id) && + (fastdds::rtps::c_VendorId_rti_connext != local_vendor_id)) { EPROSIMA_LOG_INFO(RTPS_PROXY_DATA, - "Ignoring custom PID" << pid << " from vendor " << source_vendor_id); + "Ignoring custom PID" << pid << " from vendor " << local_vendor_id); return true; } @@ -1032,11 +1053,17 @@ bool WriterProxyData::readFromCDRMessage( case fastdds::dds::PID_DATASHARING: { + VendorId_t local_vendor_id = source_vendor_id; + if (c_VendorId_Unknown == local_vendor_id) + { + local_vendor_id = ((c_VendorId_Unknown == vendor_id) ? c_VendorId_eProsima : vendor_id); + } + // Ignore custom PID when coming from other vendors - if (c_VendorId_eProsima != source_vendor_id) + if (c_VendorId_eProsima != local_vendor_id) { EPROSIMA_LOG_INFO(RTPS_PROXY_DATA, - "Ignoring custom PID" << pid << " from vendor " << source_vendor_id); + "Ignoring custom PID" << pid << " from vendor " << local_vendor_id); return true; } diff --git a/src/cpp/rtps/builtin/discovery/endpoint/EDPSimpleListeners.cpp b/src/cpp/rtps/builtin/discovery/endpoint/EDPSimpleListeners.cpp index 8a3cc1c7dff..e62f4d6e135 100644 --- a/src/cpp/rtps/builtin/discovery/endpoint/EDPSimpleListeners.cpp +++ b/src/cpp/rtps/builtin/discovery/endpoint/EDPSimpleListeners.cpp @@ -72,7 +72,7 @@ void EDPBasePUBListener::add_writer_from_change( auto temp_writer_data = edp->get_temporary_writer_proxies_pool().get(); if (temp_writer_data->readFromCDRMessage(&tempMsg, network, - edp->mp_RTPSParticipant->has_shm_transport(), true)) + edp->mp_RTPSParticipant->has_shm_transport(), true, change->vendor_id)) { if (temp_writer_data->guid().guidPrefix == edp->mp_RTPSParticipant->getGuid().guidPrefix) { @@ -183,7 +183,7 @@ void EDPBaseSUBListener::add_reader_from_change( auto temp_reader_data = edp->get_temporary_reader_proxies_pool().get(); if (temp_reader_data->readFromCDRMessage(&tempMsg, network, - edp->mp_RTPSParticipant->has_shm_transport(), true)) + edp->mp_RTPSParticipant->has_shm_transport(), true, change->vendor_id)) { if (temp_reader_data->guid().guidPrefix == edp->mp_RTPSParticipant->getGuid().guidPrefix) { diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp b/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp index 3d18053da8b..d116e0f74d4 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp @@ -107,7 +107,7 @@ void PDPListener::onNewCacheChangeAdded( CDRMessage_t msg(change->serializedPayload); temp_participant_data_.clear(); if (temp_participant_data_.readFromCDRMessage(&msg, true, parent_pdp_->getRTPSParticipant()->network_factory(), - parent_pdp_->getRTPSParticipant()->has_shm_transport(), true)) + parent_pdp_->getRTPSParticipant()->has_shm_transport(), true, change_in->vendor_id)) { // After correctly reading it change->instanceHandle = temp_participant_data_.m_key; diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp b/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp index 45eacebf8b9..5d3dbf37169 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp @@ -141,7 +141,8 @@ void PDPServerListener::onNewCacheChangeAdded( true, pdp_server()->getRTPSParticipant()->network_factory(), pdp_server()->getRTPSParticipant()->has_shm_transport(), - true)) + true, + change_in->vendor_id)) { if (parent_pdp_->getRTPSParticipant()->is_participant_ignored(participant_data.m_guid.guidPrefix)) { diff --git a/src/cpp/rtps/participant/RTPSParticipantImpl.cpp b/src/cpp/rtps/participant/RTPSParticipantImpl.cpp index 9eb751e21b2..e78e330be73 100644 --- a/src/cpp/rtps/participant/RTPSParticipantImpl.cpp +++ b/src/cpp/rtps/participant/RTPSParticipantImpl.cpp @@ -2972,7 +2972,8 @@ bool RTPSParticipantImpl::fill_discovery_data_from_cdr_message( true, network_factory(), has_shm_transport(), - false); + false, + c_VendorId_eProsima); return ret && (data.m_guid.entityId == c_EntityId_RTPSParticipant); } @@ -2992,7 +2993,8 @@ bool RTPSParticipantImpl::fill_discovery_data_from_cdr_message( &serialized_msg, network_factory(), has_shm_transport(), - false); + false, + c_VendorId_eProsima); return ret && (data.guid().entityId.is_writer()); } @@ -3012,7 +3014,8 @@ bool RTPSParticipantImpl::fill_discovery_data_from_cdr_message( &serialized_msg, network_factory(), has_shm_transport(), - false); + false, + c_VendorId_eProsima); return ret && (data.guid().entityId.is_reader()); } diff --git a/test/blackbox/common/BlackboxTestsDiscovery.cpp b/test/blackbox/common/BlackboxTestsDiscovery.cpp index 2f65079d2c9..9d9ec76cb21 100644 --- a/test/blackbox/common/BlackboxTestsDiscovery.cpp +++ b/test/blackbox/common/BlackboxTestsDiscovery.cpp @@ -12,20 +12,30 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include + +#include +#include + #ifndef _WIN32 #include #endif // _WIN32 -#include +#include +#include +#include +#include +#include #include #include +#include #include + #include #include #include "BlackboxTests.hpp" +#include "DatagramInjectionTransport.hpp" #include "PubSubReader.hpp" #include "PubSubWriter.hpp" #include "PubSubWriterReader.hpp" @@ -2131,3 +2141,69 @@ TEST(Discovery, MultipleXMLProfileLoad) thr_reader.join(); thr_writer.join(); } + +//! Regression test for redmine issue 20641 +TEST(Discovery, discovery_cyclone_participant_with_custom_pid) +{ + using namespace eprosima::fastdds::dds; + using namespace eprosima::fastrtps::rtps; + + /* Custom participant listener to count number of discovered participants */ + class DiscoveryListener : public DomainParticipantListener + { + public: + + void on_participant_discovery( + DomainParticipant*, + ParticipantDiscoveryInfo&& info) override + { + if (ParticipantDiscoveryInfo::DISCOVERED_PARTICIPANT == info.status) + { + discovered_participants_++; + } + else if (ParticipantDiscoveryInfo::REMOVED_PARTICIPANT == info.status) + { + discovered_participants_--; + } + } + + uint8_t discovered_participants() const + { + return discovered_participants_; + } + + private: + + std::atomic discovered_participants_{0}; + }; + + /* Create a datagram injection transport */ + using eprosima::fastdds::rtps::DatagramInjectionTransportDescriptor; + using eprosima::fastdds::rtps::DatagramInjectionTransport; + auto low_level_transport = std::make_shared(); + auto transport = std::make_shared(low_level_transport); + + /* Disable builtin transport and add custom one */ + DomainParticipantQos participant_qos = PARTICIPANT_QOS_DEFAULT; + participant_qos.transport().use_builtin_transports = false; + participant_qos.transport().user_transports.clear(); + participant_qos.transport().user_transports.push_back(transport); + + /* Create participant with custom transport and listener */ + DiscoveryListener listener; + uint32_t domain_id = static_cast(GET_PID()) % 230; + DomainParticipantFactory* factory = DomainParticipantFactory::get_instance(); + DomainParticipant* participant = factory->create_participant(domain_id, participant_qos, &listener); + ASSERT_NE(nullptr, participant); + + /* Inject a Cyclone DDS Data(p) with a custom PID that we also use */ + auto receivers = transport->get_receivers(); + ASSERT_FALSE(receivers.empty()); + DatagramInjectionTransport::deliver_datagram_from_file(receivers, "datagrams/20641.bin"); + + /* Assert that the participant is discovered */ + ASSERT_EQ(listener.discovered_participants(), 1u); + + /* Clean up */ + factory->delete_participant(participant); +} diff --git a/test/blackbox/common/BlackboxTestsTransportUDP.cpp b/test/blackbox/common/BlackboxTestsTransportUDP.cpp index c45244a3cfc..e907d1afab1 100644 --- a/test/blackbox/common/BlackboxTestsTransportUDP.cpp +++ b/test/blackbox/common/BlackboxTestsTransportUDP.cpp @@ -13,9 +13,7 @@ // limitations under the License. #include -#include #include -#include #include #include @@ -518,29 +516,10 @@ TEST_P(TransportUDP, whitelisting_udp_localhost_alone) } } -void deliver_datagram_from_file( - const std::set& receivers, - const char* filename) -{ - std::basic_ifstream file(filename, std::ios::binary | std::ios::in); - - file.seekg(0, file.end); - size_t file_size = static_cast(file.tellg()); - file.seekg(0, file.beg); - - std::vector buf(file_size); - file.read(reinterpret_cast(buf.data()), file_size); - - eprosima::fastdds::rtps::Locator loc; - for (const auto& rec : receivers) - { - rec->OnDataReceived(buf.data(), static_cast(file_size), loc, loc); - } -} - TEST(TransportUDP, DatagramInjection) { using eprosima::fastdds::rtps::DatagramInjectionTransportDescriptor; + using eprosima::fastdds::rtps::DatagramInjectionTransport; auto low_level_transport = std::make_shared(); auto transport = std::make_shared(low_level_transport); @@ -552,8 +531,8 @@ TEST(TransportUDP, DatagramInjection) auto receivers = transport->get_receivers(); ASSERT_FALSE(receivers.empty()); - deliver_datagram_from_file(receivers, "datagrams/16784.bin"); - deliver_datagram_from_file(receivers, "datagrams/20140.bin"); + DatagramInjectionTransport::deliver_datagram_from_file(receivers, "datagrams/16784.bin"); + DatagramInjectionTransport::deliver_datagram_from_file(receivers, "datagrams/20140.bin"); } TEST(TransportUDP, MaliciousManipulatedDataOctetsToNextHeaderIgnore) diff --git a/test/blackbox/common/DatagramInjectionTransport.hpp b/test/blackbox/common/DatagramInjectionTransport.hpp index aa1cd4b0873..03ca287ad61 100644 --- a/test/blackbox/common/DatagramInjectionTransport.hpp +++ b/test/blackbox/common/DatagramInjectionTransport.hpp @@ -12,12 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include #include #include +#include using SenderResource = eprosima::fastrtps::rtps::SenderResource; @@ -112,6 +114,26 @@ class DatagramInjectionTransport : public ChainingTransport return ret_val; } + static void deliver_datagram_from_file( + const std::set& receivers, + const char* filename) + { + std::basic_ifstream file(filename, std::ios::binary | std::ios::in); + + file.seekg(0, file.end); + size_t file_size = static_cast(file.tellg()); + file.seekg(0, file.beg); + + std::vector buf(file_size); + file.read(reinterpret_cast(buf.data()), file_size); + + eprosima::fastdds::rtps::Locator loc; + for (const auto& rec : receivers) + { + rec->OnDataReceived(buf.data(), static_cast(file_size), loc, loc); + } + } + private: DatagramInjectionTransportDescriptor* parent_ = nullptr; diff --git a/test/blackbox/datagrams/20641.bin b/test/blackbox/datagrams/20641.bin new file mode 100644 index 0000000000000000000000000000000000000000..5c49c58ecc6c37d09748a60a30bf74012197bc46 GIT binary patch literal 384 zcmWFv2?%ClWE5Z&@PEmA@b}ly%(dd2j2sMa{C=iBz4#$hlr@HtfkA))3>X;>foPxr z0|PSy0}}%W0~d%51d$9CARz`mAdZg@D9TSxEiU#;%uNOIMS(cEGC3zdFEu_TH8(## zvp6HMAhj4M%)kc3K=qj^K$;zh4GkFlrm&kz;Rg0ISu3>wqxSffN?^=^N-78tNJ7`();oRzOGw2FV7X8yeWa_5%RD C7)D(H literal 0 HcmV?d00001