Skip to content

Commit

Permalink
Explicitly pass vendor ID to readFromCdrMessage (#4583)
Browse files Browse the repository at this point in the history 
* Refs #20641: Add regression test

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20641: Explicitely pass vendor ID to all readFromCdrMessage calls

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20641: Treat unkown vendor IDs as ours

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

---------

Signed-off-by: EduPonz <eduardoponz@eprosima.com>
  • Loading branch information
@EduPonz
EduPonz authored Mar 19, 2024
1 parent a146b23 commit c5cd225
Show file tree
Hide file tree
Showing 11 changed files with 187 additions and 52 deletions.
10 changes: 8 additions & 2 deletions src/cpp/rtps/builtin/data/ParticipantProxyData.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
35 changes: 28 additions & 7 deletions src/cpp/rtps/builtin/data/ReaderProxyData.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down
47 changes: 37 additions & 10 deletions src/cpp/rtps/builtin/data/WriterProxyData.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down
4 changes: 2 additions & 2 deletions src/cpp/rtps/builtin/discovery/endpoint/EDPSimpleListeners.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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)
{
Expand Down
2 changes: 1 addition & 1 deletion src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down
9 changes: 6 additions & 3 deletions src/cpp/rtps/participant/RTPSParticipantImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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());
}
Expand All @@ -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());
}
Expand Down
80 changes: 78 additions & 2 deletions test/blackbox/common/BlackboxTestsDiscovery.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,30 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include <gtest/gtest.h>

#include <atomic>
#include <thread>

#ifndef _WIN32
#include <stdlib.h>
#endif // _WIN32

#include <thread>
#include <gtest/gtest.h>

#include <fastdds/dds/domain/DomainParticipant.hpp>
#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
#include <fastdds/dds/domain/DomainParticipantListener.hpp>
#include <fastdds/dds/domain/qos/DomainParticipantQos.hpp>
#include <fastdds/rtps/attributes/ServerAttributes.h>
#include <fastdds/rtps/common/CDRMessage_t.h>
#include <fastdds/rtps/transport/UDPv4TransportDescriptor.h>
#include <fastrtps/xmlparser/XMLProfileManager.h>

#include <rtps/transport/test_UDPv4Transport.h>
#include <utils/SystemInfo.hpp>

#include "BlackboxTests.hpp"
#include "DatagramInjectionTransport.hpp"
#include "PubSubReader.hpp"
#include "PubSubWriter.hpp"
#include "PubSubWriterReader.hpp"
Expand Down Expand Up @@ -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<uint8_t> 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<UDPv4TransportDescriptor>();
auto transport = std::make_shared<DatagramInjectionTransportDescriptor>(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<uint32_t>(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);
}
Loading

0 comments on commit c5cd225

Please sign in to comment.