From 410b8660a257777570c3b4681a4a4542a3a3dd60 Mon Sep 17 00:00:00 2001 From: jparisu Date: Tue, 18 Oct 2022 12:18:34 +0200 Subject: [PATCH 1/8] Refs #15965: Avoid calling StopAll in RTPSDomain before DomainParticipantFactory Signed-off-by: jparisu --- test/blackbox/common/BlackboxTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/blackbox/common/BlackboxTests.cpp b/test/blackbox/common/BlackboxTests.cpp index c852a08be98..24522507cf2 100644 --- a/test/blackbox/common/BlackboxTests.cpp +++ b/test/blackbox/common/BlackboxTests.cpp @@ -79,7 +79,7 @@ class BlackboxEnvironment : public ::testing::Environment { //Log::Reset(); eprosima::fastdds::dds::Log::KillThread(); - eprosima::fastrtps::rtps::RTPSDomain::stopAll(); + // eprosima::fastrtps::rtps::RTPSDomain::stopAll(); } }; From 9bab8b50b9bb804664a6ad0ec449c772d56cdfd1 Mon Sep 17 00:00:00 2001 From: jparisu Date: Tue, 18 Oct 2022 12:22:31 +0200 Subject: [PATCH 2/8] Refs #15965: Fix race condition in ParticipantProxyPhysicalData Signed-off-by: jparisu --- .../rtps/participant/ParticipantDiscoveryInfo.h | 7 ++++++- .../common/DDSBlackboxTestsDiscovery.cpp | 16 ++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h b/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h index 55cd8821b9c..6558f284b0a 100644 --- a/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h +++ b/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h @@ -57,7 +57,12 @@ struct ParticipantDiscoveryInfo //! Status DISCOVERY_STATUS status; - //! Participant discovery info + /** + * @brief Participant discovery info + * + * @todo This is a reference to an object that could be deleted, thus it should not be a reference + * (intraprocess case -> BlackboxTests_DDS_PIM.DDSDiscovery.ParticipantProxyPhysicalData). + */ const ParticipantProxyData& info; }; diff --git a/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp b/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp index 7772c891b85..b6b5106460e 100644 --- a/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp +++ b/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp @@ -423,14 +423,18 @@ TEST(DDSDiscovery, ParticipantProxyPhysicalData) ParticipantDiscoveryInfo&& info) { std::unique_lock lck(*mtx_); - static_cast(participant); - if (nullptr != remote_participant_info) + if (info.status == + eprosima::fastrtps::rtps::ParticipantDiscoveryInfo::DISCOVERY_STATUS::DISCOVERED_PARTICIPANT) { - delete remote_participant_info; + static_cast(participant); + if (nullptr != remote_participant_info) + { + delete remote_participant_info; + } + remote_participant_info = new ParticipantDiscoveryInfo(info); + found_->store(true); + cv_->notify_one(); } - remote_participant_info = new ParticipantDiscoveryInfo(info); - found_->store(true); - cv_->notify_one(); } ParticipantDiscoveryInfo* remote_participant_info; From de85a5f8f5a61f26ee59d9ab6bea139d51872592 Mon Sep 17 00:00:00 2001 From: jparisu Date: Tue, 18 Oct 2022 15:04:31 +0200 Subject: [PATCH 3/8] Refs #15965: Modify ChainingTransport to hold uniqueptrs Signed-off-by: jparisu --- .../fastdds/rtps/transport/ChainingTransport.h | 18 +++++++++++++++++- src/cpp/rtps/transport/ChainingTransport.cpp | 5 +++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/include/fastdds/rtps/transport/ChainingTransport.h b/include/fastdds/rtps/transport/ChainingTransport.h index 8864e6e18ce..78600617efe 100644 --- a/include/fastdds/rtps/transport/ChainingTransport.h +++ b/include/fastdds/rtps/transport/ChainingTransport.h @@ -16,6 +16,7 @@ #define _FASTDDS_RTPS_TRANSPORT_CHAININGTRANSPORT_H_ #include +#include #include "TransportInterface.h" #include "ChainingTransportDescriptor.h" @@ -26,6 +27,21 @@ namespace rtps { class ChainingReceiverResource; +/** + * @brief Deleter for a ChainingReceiverResource + * + * @note this is required to create a \c unique_ptr of a \c ChainingReceiverResource as it is + * an incomplete class for this header. + */ +struct ChainingReceiverResourceDeleter +{ + void operator()(ChainingReceiverResource *p); +}; + +//! Type of the \c unique_ptr of a \c ChainingReceiverResource . +using ChainingReceiverResourceReferenceType = + std::unique_ptr; + /** * This is the base class for chaining adapter transports. * - Directly proxies all operations except Send and Receive @@ -340,7 +356,7 @@ class ChainingTransport : public TransportInterface private: - std::map receiver_resources_; + std::map receiver_resources_; }; } // namespace rtps diff --git a/src/cpp/rtps/transport/ChainingTransport.cpp b/src/cpp/rtps/transport/ChainingTransport.cpp index b4380647c25..bf89a2c59df 100644 --- a/src/cpp/rtps/transport/ChainingTransport.cpp +++ b/src/cpp/rtps/transport/ChainingTransport.cpp @@ -20,6 +20,11 @@ namespace eprosima { namespace fastdds { namespace rtps { +void ChainingReceiverResourceDeleter::operator()(ChainingReceiverResource *p) +{ + delete p; +} + bool ChainingTransport::OpenInputChannel( const fastrtps::rtps::Locator_t& loc, TransportReceiverInterface* receiver_interface, From 43276f510cebdb5db81bd71748f0025458081ff3 Mon Sep 17 00:00:00 2001 From: jparisu Date: Mon, 24 Oct 2022 09:24:50 +0200 Subject: [PATCH 4/8] Refs #15965: Remove SystemInfo from sources which creates twice same variable Signed-off-by: jparisu --- test/unittest/dds/participant/CMakeLists.txt | 5 ++--- test/unittest/dds/participant/ParticipantTests.cpp | 13 ++++++++++++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/test/unittest/dds/participant/CMakeLists.txt b/test/unittest/dds/participant/CMakeLists.txt index 07a27e9e27e..14c20e3bed0 100644 --- a/test/unittest/dds/participant/CMakeLists.txt +++ b/test/unittest/dds/participant/CMakeLists.txt @@ -17,9 +17,8 @@ configure_file(${CMAKE_CURRENT_SOURCE_DIR}/../profiles/test_xml_profiles.xml COPYONLY) set(PARTICIPANTTESTS_SOURCE - ParticipantTests.cpp - ${PROJECT_SOURCE_DIR}/src/cpp/rtps/builtin/discovery/participant/ServerAttributes.cpp - ${PROJECT_SOURCE_DIR}/src/cpp/utils/SystemInfo.cpp) + ParticipantTests.cpp + ) if(WIN32) add_definitions(-D_WIN32_WINNT=0x0601) diff --git a/test/unittest/dds/participant/ParticipantTests.cpp b/test/unittest/dds/participant/ParticipantTests.cpp index 2f479acc281..bb7f7540614 100644 --- a/test/unittest/dds/participant/ParticipantTests.cpp +++ b/test/unittest/dds/participant/ParticipantTests.cpp @@ -274,6 +274,17 @@ class BarType std::array message_; }; +int process_id() +{ +#if defined(__cplusplus_winrt) + return (int)GetCurrentProcessId(); +#elif defined(_WIN32) + return (int)_getpid(); +#else + return (int)getpid(); +#endif // platform selection +} + TEST(ParticipantTests, DomainParticipantFactoryGetInstance) { @@ -721,7 +732,7 @@ void set_environment_file( std::string get_environment_filename() { std::ostringstream name; - name << "environment_file_" << SystemInfo::instance().process_id() << ".json"; + name << "environment_file_" << process_id() << ".json"; std::string fname = name.str(); // 'touch' the file std::ofstream f(fname); From 791928a523991dbb100185bfa1d22200fc64c516 Mon Sep 17 00:00:00 2001 From: jparisu Date: Mon, 31 Oct 2022 16:13:13 +0100 Subject: [PATCH 5/8] Refs #15965: apply suggestionsg Signed-off-by: jparisu --- test/blackbox/common/BlackboxTests.cpp | 2 +- test/unittest/dds/participant/ParticipantTests.cpp | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/test/blackbox/common/BlackboxTests.cpp b/test/blackbox/common/BlackboxTests.cpp index 24522507cf2..0e83432997d 100644 --- a/test/blackbox/common/BlackboxTests.cpp +++ b/test/blackbox/common/BlackboxTests.cpp @@ -79,7 +79,7 @@ class BlackboxEnvironment : public ::testing::Environment { //Log::Reset(); eprosima::fastdds::dds::Log::KillThread(); - // eprosima::fastrtps::rtps::RTPSDomain::stopAll(); + // Please, do not remove RTPSDomain before DomainParticipantFactory } }; diff --git a/test/unittest/dds/participant/ParticipantTests.cpp b/test/unittest/dds/participant/ParticipantTests.cpp index bb7f7540614..371a6bbebeb 100644 --- a/test/unittest/dds/participant/ParticipantTests.cpp +++ b/test/unittest/dds/participant/ParticipantTests.cpp @@ -274,6 +274,9 @@ class BarType std::array message_; }; +// NOTE: This function is duplicated from SystemInfo because it is not in the API and could not be added to test +// compilation as that file is already compiled and linked, and doing such thing is wrong and would make a kitten cry. +// (it duplicates an instantiated variable 'environment_file_' and so provoke a double free). int process_id() { #if defined(__cplusplus_winrt) From 27a1fc37bf0b1b0e396713bf88d3f21a2b6ac4ad Mon Sep 17 00:00:00 2001 From: jparisu Date: Fri, 4 Nov 2022 09:54:14 +0100 Subject: [PATCH 6/8] Refs #15965: add small fixes Signed-off-by: jparisu --- .github/workflows/asan.yml | 2 +- test/unittest/dds/participant/CMakeLists.txt | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/asan.yml b/.github/workflows/asan.yml index 527d20df474..e7f3a2f8615 100644 --- a/.github/workflows/asan.yml +++ b/.github/workflows/asan.yml @@ -5,7 +5,7 @@ on: pull_request: push: branches: - - main + - master schedule: - cron: '0 1 * * *' diff --git a/test/unittest/dds/participant/CMakeLists.txt b/test/unittest/dds/participant/CMakeLists.txt index 14c20e3bed0..77964da913b 100644 --- a/test/unittest/dds/participant/CMakeLists.txt +++ b/test/unittest/dds/participant/CMakeLists.txt @@ -16,8 +16,11 @@ configure_file(${CMAKE_CURRENT_SOURCE_DIR}/../profiles/test_xml_profiles.xml ${CMAKE_CURRENT_BINARY_DIR}/test_xml_profiles.xml COPYONLY) +# NOTE: having here ServerAttributes.cpp is dangerous as this .cpp is already compiled. +# However, I found no way to do it work, so here it stays. set(PARTICIPANTTESTS_SOURCE ParticipantTests.cpp + ${PROJECT_SOURCE_DIR}/src/cpp/rtps/builtin/discovery/participant/ServerAttributes.cpp ) if(WIN32) From 84548f08fc3a474a6d4f0a4d01320c0241c7ddad Mon Sep 17 00:00:00 2001 From: jparisu Date: Fri, 4 Nov 2022 09:55:41 +0100 Subject: [PATCH 7/8] Refs #15965: uncrustify Signed-off-by: jparisu --- .../participant/ParticipantDiscoveryInfo.h | 44 ++++++++++++------- .../rtps/transport/ChainingTransport.h | 5 ++- src/cpp/rtps/transport/ChainingTransport.cpp | 3 +- .../common/DDSBlackboxTestsDiscovery.cpp | 2 +- .../dds/participant/ParticipantTests.cpp | 1 - 5 files changed, 34 insertions(+), 21 deletions(-) diff --git a/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h b/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h index 6558f284b0a..f9201e8fce5 100644 --- a/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h +++ b/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h @@ -28,9 +28,9 @@ namespace fastrtps { namespace rtps { /** -* Class ParticipantDiscoveryInfo with discovery information of the Participant. -* @ingroup RTPS_MODULE -*/ + * Class ParticipantDiscoveryInfo with discovery information of the Participant. + * @ingroup RTPS_MODULE + */ struct ParticipantDiscoveryInfo { //!Enum DISCOVERY_STATUS, four different status for discovered participants. @@ -39,20 +39,24 @@ struct ParticipantDiscoveryInfo enum RTPS_DllAPI DISCOVERY_STATUS #else enum DISCOVERY_STATUS -#endif +#endif // if defined(_WIN32) { DISCOVERED_PARTICIPANT, CHANGED_QOS_PARTICIPANT, REMOVED_PARTICIPANT, DROPPED_PARTICIPANT - }; + } - ParticipantDiscoveryInfo(const ParticipantProxyData& data) + ParticipantDiscoveryInfo( + const ParticipantProxyData& data) : status(DISCOVERED_PARTICIPANT) , info(data) - {} + { + } - virtual ~ParticipantDiscoveryInfo() {} + virtual ~ParticipantDiscoveryInfo() + { + } //! Status DISCOVERY_STATUS status; @@ -75,9 +79,14 @@ struct ParticipantAuthenticationInfo UNAUTHORIZED_PARTICIPANT }; - ParticipantAuthenticationInfo() : status(UNAUTHORIZED_PARTICIPANT) {} + ParticipantAuthenticationInfo() + : status(UNAUTHORIZED_PARTICIPANT) + { + } - ~ParticipantAuthenticationInfo() {} + ~ParticipantAuthenticationInfo() + { + } //! Status AUTHENTICATION_STATUS status; @@ -86,15 +95,18 @@ struct ParticipantAuthenticationInfo GUID_t guid; }; -inline bool operator==(const ParticipantAuthenticationInfo& l, const ParticipantAuthenticationInfo& r) +inline bool operator ==( + const ParticipantAuthenticationInfo& l, + const ParticipantAuthenticationInfo& r) { return l.status == r.status && - l.guid == r.guid; + l.guid == r.guid; } -#endif -} -} -} +#endif // if HAVE_SECURITY + +} // namespace rtps +} // namespace fastrtps +} // namespace eprosima #endif // _FASTDDS_RTPS_PARTICIPANT_PARTICIPANTDISCOVERYINFO_H__ diff --git a/include/fastdds/rtps/transport/ChainingTransport.h b/include/fastdds/rtps/transport/ChainingTransport.h index 78600617efe..1793182b71b 100644 --- a/include/fastdds/rtps/transport/ChainingTransport.h +++ b/include/fastdds/rtps/transport/ChainingTransport.h @@ -35,12 +35,13 @@ class ChainingReceiverResource; */ struct ChainingReceiverResourceDeleter { - void operator()(ChainingReceiverResource *p); + void operator ()( + ChainingReceiverResource* p); }; //! Type of the \c unique_ptr of a \c ChainingReceiverResource . using ChainingReceiverResourceReferenceType = - std::unique_ptr; + std::unique_ptr; /** * This is the base class for chaining adapter transports. diff --git a/src/cpp/rtps/transport/ChainingTransport.cpp b/src/cpp/rtps/transport/ChainingTransport.cpp index bf89a2c59df..dea45890bc5 100644 --- a/src/cpp/rtps/transport/ChainingTransport.cpp +++ b/src/cpp/rtps/transport/ChainingTransport.cpp @@ -20,7 +20,8 @@ namespace eprosima { namespace fastdds { namespace rtps { -void ChainingReceiverResourceDeleter::operator()(ChainingReceiverResource *p) +void ChainingReceiverResourceDeleter::operator ()( + ChainingReceiverResource* p) { delete p; } diff --git a/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp b/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp index b6b5106460e..c53231740ad 100644 --- a/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp +++ b/test/blackbox/common/DDSBlackboxTestsDiscovery.cpp @@ -424,7 +424,7 @@ TEST(DDSDiscovery, ParticipantProxyPhysicalData) { std::unique_lock lck(*mtx_); if (info.status == - eprosima::fastrtps::rtps::ParticipantDiscoveryInfo::DISCOVERY_STATUS::DISCOVERED_PARTICIPANT) + eprosima::fastrtps::rtps::ParticipantDiscoveryInfo::DISCOVERY_STATUS::DISCOVERED_PARTICIPANT) { static_cast(participant); if (nullptr != remote_participant_info) diff --git a/test/unittest/dds/participant/ParticipantTests.cpp b/test/unittest/dds/participant/ParticipantTests.cpp index 371a6bbebeb..387e405fac8 100644 --- a/test/unittest/dds/participant/ParticipantTests.cpp +++ b/test/unittest/dds/participant/ParticipantTests.cpp @@ -288,7 +288,6 @@ int process_id() #endif // platform selection } - TEST(ParticipantTests, DomainParticipantFactoryGetInstance) { DomainParticipantFactory* factory = DomainParticipantFactory::get_instance(); From c33bfda2b007d2f4b0f29d64197bb6b3e71b6bcd Mon Sep 17 00:00:00 2001 From: jparisu Date: Fri, 4 Nov 2022 12:29:29 +0100 Subject: [PATCH 8/8] Refs #15965: fix typo Signed-off-by: jparisu --- include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h b/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h index f9201e8fce5..e444bba6063 100644 --- a/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h +++ b/include/fastdds/rtps/participant/ParticipantDiscoveryInfo.h @@ -45,7 +45,7 @@ struct ParticipantDiscoveryInfo CHANGED_QOS_PARTICIPANT, REMOVED_PARTICIPANT, DROPPED_PARTICIPANT - } + }; ParticipantDiscoveryInfo( const ParticipantProxyData& data)