From 9a72531910270ac352972c124e4b7e1915a004e7 Mon Sep 17 00:00:00 2001 From: Yueshan Date: Thu, 25 Jun 2026 00:03:26 +0800 Subject: [PATCH 1/5] Implement payload release in move assignment Signed-off-by: Yueshan --- src/cpp/rtps/common/SerializedPayload.cpp | 9 ++ test/unittest/rtps/common/CMakeLists.txt | 12 ++ .../rtps/common/SerializedPayloadTests.cpp | 131 ++++++++++++++++++ 3 files changed, 152 insertions(+) create mode 100644 test/unittest/rtps/common/SerializedPayloadTests.cpp diff --git a/src/cpp/rtps/common/SerializedPayload.cpp b/src/cpp/rtps/common/SerializedPayload.cpp index 98ea4738373..436348e0de9 100644 --- a/src/cpp/rtps/common/SerializedPayload.cpp +++ b/src/cpp/rtps/common/SerializedPayload.cpp @@ -30,6 +30,15 @@ SerializedPayload_t& SerializedPayload_t::operator = ( return *this; } + if (payload_owner != nullptr) + { + payload_owner->release_payload(*this); + } + else if (data != nullptr) + { + free(data); + } + encapsulation = other.encapsulation; length = other.length; data = other.data; diff --git a/test/unittest/rtps/common/CMakeLists.txt b/test/unittest/rtps/common/CMakeLists.txt index f7376fcf2ff..b0707a058d3 100644 --- a/test/unittest/rtps/common/CMakeLists.txt +++ b/test/unittest/rtps/common/CMakeLists.txt @@ -45,6 +45,8 @@ if(ANDROID) endif() +set(SERIALIZEDPAYLOADTESTS_SOURCE SerializedPayloadTests.cpp + ${PROJECT_SOURCE_DIR}/src/cpp/rtps/common/SerializedPayload.cpp) set(SEQUENCENUMBERTESTS_SOURCE SequenceNumberTests.cpp) set(PORTPARAMETERSTESTS_SOURCE PortParametersTests.cpp ${PROJECT_SOURCE_DIR}/src/cpp/fastdds/core/Time_t.cpp @@ -100,6 +102,16 @@ target_link_libraries(GuidUtilsTests ) gtest_discover_tests(GuidUtilsTests) +add_executable(SerializedPayloadTests ${SERIALIZEDPAYLOADTESTS_SOURCE}) +target_compile_definitions(SerializedPayloadTests PRIVATE + $<$>,$>:__DEBUG> + $<$:__INTERNALDEBUG> # Internal debug activated. + ) +target_include_directories(SerializedPayloadTests PRIVATE + ${PROJECT_SOURCE_DIR}/include ${PROJECT_BINARY_DIR}/include) +target_link_libraries(SerializedPayloadTests GTest::gtest) +gtest_discover_tests(SerializedPayloadTests) + add_executable(SequenceNumberTests ${SEQUENCENUMBERTESTS_SOURCE}) target_compile_definitions(SequenceNumberTests PRIVATE $<$>,$>:__DEBUG> diff --git a/test/unittest/rtps/common/SerializedPayloadTests.cpp b/test/unittest/rtps/common/SerializedPayloadTests.cpp new file mode 100644 index 00000000000..ff839373099 --- /dev/null +++ b/test/unittest/rtps/common/SerializedPayloadTests.cpp @@ -0,0 +1,131 @@ +// Copyright 2026 Proyectos y Sistemas de Mantenimiento SL (eProsima). +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include + +#include + +#include + +using namespace eprosima::fastdds::rtps; + +class TestingPayloadPool : public IPayloadPool +{ +public: + + ~TestingPayloadPool() override = default; + + bool get_payload( + uint32_t size, + SerializedPayload_t& payload) override + { + payload.data = static_cast(calloc(size, sizeof(octet))); + if (payload.data == nullptr) + { + return false; + } + + payload.payload_owner = this; + payload.max_size = size; + payload.length = size; + return true; + } + + bool get_payload( + const SerializedPayload_t& data, + SerializedPayload_t& payload) override + { + if (!get_payload(data.length, payload)) + { + return false; + } + + payload.copy(&data); + return true; + } + + bool release_payload( + SerializedPayload_t& payload) override + { + ++release_payload_calls; + EXPECT_EQ(this, payload.payload_owner); + + free(payload.data); + payload.data = nullptr; + payload.payload_owner = nullptr; + payload.length = 0; + payload.max_size = 0; + return true; + } + + size_t release_payload_calls = 0; +}; + +/*! + * @fn TEST(SerializedPayload, MoveAssignmentOperatorReleasesCurrentPayload) + * @brief This test checks that move assignment operator releases an already owned destination payload. + */ +TEST(SerializedPayload, MoveAssignmentOperatorReleasesCurrentPayload) +{ + TestingPayloadPool payload_pool; + SerializedPayload_t destination; + ASSERT_TRUE(payload_pool.get_payload(4, destination)); + + SerializedPayload_t source(8); + source.length = 8; + + destination = std::move(source); + + EXPECT_EQ(1u, payload_pool.release_payload_calls); + EXPECT_EQ(8u, destination.length); + EXPECT_EQ(8u, destination.max_size); + EXPECT_NE(nullptr, destination.data); + EXPECT_EQ(nullptr, destination.payload_owner); + EXPECT_EQ(0u, source.length); + EXPECT_EQ(0u, source.max_size); + EXPECT_EQ(nullptr, source.data); +} + +/*! + * @fn TEST(SerializedPayload, MoveAssignmentOperatorReleasesCurrentUnownedPayload) + * @brief This test checks that move assignment operator releases an already allocated destination payload. + */ +TEST(SerializedPayload, MoveAssignmentOperatorReleasesCurrentUnownedPayload) +{ + SerializedPayload_t destination(4); + ASSERT_NE(nullptr, destination.data); + + SerializedPayload_t source(8); + source.length = 8; + + destination = std::move(source); + + EXPECT_EQ(8u, destination.length); + EXPECT_EQ(8u, destination.max_size); + EXPECT_NE(nullptr, destination.data); + EXPECT_EQ(nullptr, destination.payload_owner); + EXPECT_EQ(0u, source.length); + EXPECT_EQ(0u, source.max_size); + EXPECT_EQ(nullptr, source.data); +} + +int main( + int argc, + char** argv) +{ + testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} From 338fc7bfec03e16e5b314f1b33236e21bf263f7b Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Mon, 29 Jun 2026 16:32:30 +0200 Subject: [PATCH 2/5] Use no-op pool in MessageReceiver. Signed-off-by: Miguel Company --- src/cpp/rtps/messages/MessageReceiver.cpp | 50 +++++++++++++++++------ 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/src/cpp/rtps/messages/MessageReceiver.cpp b/src/cpp/rtps/messages/MessageReceiver.cpp index f1be51957c5..e666ec976fe 100644 --- a/src/cpp/rtps/messages/MessageReceiver.cpp +++ b/src/cpp/rtps/messages/MessageReceiver.cpp @@ -47,6 +47,38 @@ namespace eprosima { namespace fastdds { namespace rtps { +struct NoOpPayloadPool : public IPayloadPool +{ + bool get_payload( + uint32_t size, + SerializedPayload_t& payload) override + { + static_cast(payload); + static_cast(size); + return false; + } + + bool get_payload( + const SerializedPayload_t& data, + SerializedPayload_t& payload) override + { + static_cast(data); + static_cast(payload); + return false; + } + + bool release_payload( + SerializedPayload_t& payload) override + { + if (payload.payload_owner == this) + { + payload.data = nullptr; + return true; + } + return false; + } +}; + MessageReceiver::MessageReceiver( RTPSParticipantImpl* participant, uint32_t rec_buffer_size) @@ -796,6 +828,7 @@ bool MessageReceiver::proc_Submsg_Data( //FOUND THE READER. //We ask the reader for a cachechange to store the information. + NoOpPayloadPool no_op_pool; CacheChange_t ch; ch.kind = ALIVE; ch.writerGUID.guidPrefix = source_guid_prefix_; @@ -846,6 +879,7 @@ bool MessageReceiver::proc_Submsg_Data( ch.inline_qos.length = inlineQosSize; ch.inline_qos.encapsulation = endiannessFlag ? PL_CDR_LE : PL_CDR_BE; ch.inline_qos.pos = 0; + ch.inline_qos.payload_owner = &no_op_pool; } if (dataFlag || keyFlag) @@ -872,6 +906,7 @@ bool MessageReceiver::proc_Submsg_Data( ch.serializedPayload.length = payload_size; ch.serializedPayload.max_size = payload_size; ch.serializedPayload.is_serialized_key = keyFlag; + ch.serializedPayload.payload_owner = &no_op_pool; msg->pos = next_pos; } else @@ -897,16 +932,6 @@ bool MessageReceiver::proc_Submsg_Data( //Look for the correct reader to add the change process_data_message_function_(readerID, ch, was_decoded); - IPayloadPool* payload_pool = ch.serializedPayload.payload_owner; - if (payload_pool) - { - payload_pool->release_payload(ch.serializedPayload); - } - - //TODO(Ricardo) If an exception is thrown (ex, by fastcdr), these lines are not executed -> segmentation fault - ch.serializedPayload.data = nullptr; - ch.inline_qos.data = nullptr; - EPROSIMA_LOG_INFO(RTPS_MSG_IN, IDSTRING "Sub Message DATA processed"); return true; } @@ -960,6 +985,7 @@ bool MessageReceiver::proc_Submsg_DataFrag( //FOUND THE READER. //We ask the reader for a cachechange to store the information. + NoOpPayloadPool no_op_pool; CacheChange_t ch; ch.kind = ALIVE; ch.writerGUID.guidPrefix = source_guid_prefix_; @@ -1024,6 +1050,7 @@ bool MessageReceiver::proc_Submsg_DataFrag( ch.inline_qos.length = inlineQosSize; ch.inline_qos.encapsulation = endiannessFlag ? PL_CDR_LE : PL_CDR_BE; ch.inline_qos.pos = 0; + ch.inline_qos.payload_owner = &no_op_pool; } uint32_t payload_size; @@ -1047,6 +1074,7 @@ bool MessageReceiver::proc_Submsg_DataFrag( ch.serializedPayload.length = payload_size; ch.serializedPayload.max_size = payload_size; ch.serializedPayload.is_serialized_key = keyFlag; + ch.serializedPayload.payload_owner = &no_op_pool; ch.setFragmentSize(fragmentSize); msg->pos = next_pos; @@ -1071,8 +1099,6 @@ bool MessageReceiver::proc_Submsg_DataFrag( << associated_readers_.size()); process_data_fragment_message_function_(readerID, ch, sampleSize, fragmentStartingNum, fragmentsInSubmessage, was_decoded); - ch.serializedPayload.data = nullptr; - ch.inline_qos.data = nullptr; EPROSIMA_LOG_INFO(RTPS_MSG_IN, IDSTRING "Sub Message DATA_FRAG processed"); From e0443fcba814fcef9223b6ec0cbac90c4861b108 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Mon, 29 Jun 2026 16:42:02 +0200 Subject: [PATCH 3/5] Reset payload data pointer when failing in read_from_shared_history. Signed-off-by: Miguel Company --- src/cpp/rtps/DataSharing/ReaderPool.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cpp/rtps/DataSharing/ReaderPool.hpp b/src/cpp/rtps/DataSharing/ReaderPool.hpp index d676c98f190..08f24a48863 100644 --- a/src/cpp/rtps/DataSharing/ReaderPool.hpp +++ b/src/cpp/rtps/DataSharing/ReaderPool.hpp @@ -255,6 +255,7 @@ class ReaderPool : public DataSharingPayloadPool if (check == c_SequenceNumber_Unknown || check != cache_change.sequenceNumber) { // data override while processing + cache_change.serializedPayload.data = nullptr; return false; } From ee08af61b4a5ad886cbefdd8d2767e5fdcbe7950 Mon Sep 17 00:00:00 2001 From: Yueshan Date: Sun, 5 Jul 2026 21:45:08 +0800 Subject: [PATCH 4/5] Fix SerializedPayload_t move constructor reading uninitialized members Signed-off-by: Yueshan --- .../fastdds/rtps/common/SerializedPayload.hpp | 19 ++++++++++++++++++- src/cpp/rtps/common/SerializedPayload.cpp | 8 ++++++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/include/fastdds/rtps/common/SerializedPayload.hpp b/include/fastdds/rtps/common/SerializedPayload.hpp index c856f23a3bd..663fd887a1f 100644 --- a/include/fastdds/rtps/common/SerializedPayload.hpp +++ b/include/fastdds/rtps/common/SerializedPayload.hpp @@ -103,10 +103,27 @@ struct FASTDDS_EXPORTED_API SerializedPayload_t const SerializedPayload_t& other) = delete; //!Move constructor + //!Directly moves the fields from \c other instead of calling the move assignment operator, + //!since the latter assumes the destination is already constructed (it may release the + //!currently owned payload). Using the move assignment operator here would read + //!uninitialized members of the newly constructed object. SerializedPayload_t( SerializedPayload_t&& other) noexcept + : encapsulation(other.encapsulation) + , length(other.length) + , data(other.data) + , max_size(other.max_size) + , pos(other.pos) + , payload_owner(other.payload_owner) + , is_serialized_key(other.is_serialized_key) { - *this = std::move(other); + other.encapsulation = CDR_BE; + other.length = 0; + other.data = nullptr; + other.max_size = 0; + other.pos = 0; + other.payload_owner = nullptr; + other.is_serialized_key = false; } //!Move operator diff --git a/src/cpp/rtps/common/SerializedPayload.cpp b/src/cpp/rtps/common/SerializedPayload.cpp index 436348e0de9..4940e1d184a 100644 --- a/src/cpp/rtps/common/SerializedPayload.cpp +++ b/src/cpp/rtps/common/SerializedPayload.cpp @@ -32,7 +32,9 @@ SerializedPayload_t& SerializedPayload_t::operator = ( if (payload_owner != nullptr) { - payload_owner->release_payload(*this); + auto state = payload_owner->release_payload(*this); + assert(state); + payload_owner = nullptr; } else if (data != nullptr) { @@ -62,7 +64,9 @@ SerializedPayload_t::~SerializedPayload_t() { if (payload_owner != nullptr) { - payload_owner->release_payload(*this); + auto state = payload_owner->release_payload(*this); + assert(state); + payload_owner = nullptr; } this->empty(); } From 9b5b0e643e5d909e15db342c9e8490cc6fe25a0b Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Mon, 6 Jul 2026 08:10:21 +0200 Subject: [PATCH 5/5] Avoid unused variable warning Signed-off-by: Miguel Company --- src/cpp/rtps/common/SerializedPayload.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/cpp/rtps/common/SerializedPayload.cpp b/src/cpp/rtps/common/SerializedPayload.cpp index 4940e1d184a..0675251551c 100644 --- a/src/cpp/rtps/common/SerializedPayload.cpp +++ b/src/cpp/rtps/common/SerializedPayload.cpp @@ -32,8 +32,9 @@ SerializedPayload_t& SerializedPayload_t::operator = ( if (payload_owner != nullptr) { - auto state = payload_owner->release_payload(*this); - assert(state); + bool success = payload_owner->release_payload(*this); + static_cast(success); + assert(success); payload_owner = nullptr; } else if (data != nullptr) @@ -64,8 +65,9 @@ SerializedPayload_t::~SerializedPayload_t() { if (payload_owner != nullptr) { - auto state = payload_owner->release_payload(*this); - assert(state); + bool success = payload_owner->release_payload(*this); + static_cast(success); + assert(success); payload_owner = nullptr; } this->empty();