Skip to content

Commit d3c65aa

Browse files
Arithmetic overflow in fragment size calculations (#5512)
* Tests arithmetic overflow in fragment size calculations Signed-off-by: Eugenio Collado <[email protected]> * Refs #21814. Fix code in BaseWriter.cpp. Signed-off-by: Miguel Company <[email protected]> * Fix corner case overhead==max_data_size Signed-off-by: Eugenio Collado <[email protected]> * Refs #21814. Fix code in WriterHistory.cpp. Signed-off-by: Miguel Company <[email protected]> * Fix corner case overhead==final_high_mark_for_frag Signed-off-by: Eugenio Collado <[email protected]> * Uncrustify Signed-off-by: Eugenio Collado <[email protected]> * Fix log error message Signed-off-by: Eugenio Collado <[email protected]> * Fix test fragments not been dropped Signed-off-by: Eugenio Collado <[email protected]> * Fix corner case RTPSParticipantImpl max_data_size < overhead Signed-off-by: Eugenio Collado <[email protected]> * Test refactor for windows compilation Signed-off-by: Eugenio Collado <[email protected]> * Fix blackbox test Signed-off-by: Eugenio Collado <[email protected]> * Applied review suggestions Signed-off-by: EugenioCollado <[email protected]> --------- Signed-off-by: Eugenio Collado <[email protected]> Signed-off-by: Miguel Company <[email protected]> Signed-off-by: EugenioCollado <[email protected]> Co-authored-by: Miguel Company <[email protected]> (cherry picked from commit bfc5a53) Co-authored-by: EugenioCollado <[email protected]>
1 parent a00ea62 commit d3c65aa

File tree

6 files changed

+172
-17
lines changed

6 files changed

+172
-17
lines changed

src/cpp/rtps/history/WriterHistory.cpp

+10-3
Original file line numberDiff line numberDiff line change
@@ -484,9 +484,16 @@ void WriterHistory::set_fragments(
484484
// If inlineqos for related_sample_identity is required, then remove its size from the final fragment size.
485485
if (0 < inline_qos_size)
486486
{
487-
final_high_mark_for_frag -= (
488-
fastdds::dds::ParameterSerializer<Parameter_t>::PARAMETER_SENTINEL_SIZE +
489-
inline_qos_size);
487+
uint32_t overhead = fastdds::dds::ParameterSerializer<Parameter_t>::PARAMETER_SENTINEL_SIZE + inline_qos_size;
488+
constexpr uint32_t min_fragment_size = 4;
489+
if (final_high_mark_for_frag < (overhead + min_fragment_size))
490+
{
491+
final_high_mark_for_frag = min_fragment_size;
492+
}
493+
else
494+
{
495+
final_high_mark_for_frag -= overhead;
496+
}
490497
}
491498

492499
// If it is big data, fragment it.

src/cpp/rtps/participant/RTPSParticipantImpl.cpp

+8-6
Original file line numberDiff line numberDiff line change
@@ -2309,20 +2309,22 @@ uint32_t RTPSParticipantImpl::getMaxDataSize()
23092309
uint32_t RTPSParticipantImpl::calculateMaxDataSize(
23102310
uint32_t length)
23112311
{
2312-
uint32_t maxDataSize = length;
2313-
2312+
// RTPS header
2313+
uint32_t overhead = RTPSMESSAGE_HEADER_SIZE;
23142314
#if HAVE_SECURITY
23152315
// If there is rtps messsage protection, reduce max size for messages,
23162316
// because extra data is added on encryption.
23172317
if (security_attributes_.is_rtps_protected)
23182318
{
2319-
maxDataSize -= m_security_manager.calculate_extra_size_for_rtps_message();
2319+
overhead += m_security_manager.calculate_extra_size_for_rtps_message();
23202320
}
23212321
#endif // if HAVE_SECURITY
23222322

2323-
// RTPS header
2324-
maxDataSize -= RTPSMESSAGE_HEADER_SIZE;
2325-
return maxDataSize;
2323+
if (length <= overhead)
2324+
{
2325+
return 0;
2326+
}
2327+
return length - overhead;
23262328
}
23272329

23282330
bool RTPSParticipantImpl::networkFactoryHasRegisteredTransports() const

src/cpp/rtps/writer/BaseWriter.cpp

+15-7
Original file line numberDiff line numberDiff line change
@@ -189,30 +189,38 @@ uint32_t BaseWriter::calculate_max_payload_size(
189189
constexpr uint32_t heartbeat_message_length = 32;
190190

191191
uint32_t max_data_size = mp_RTPSParticipant->calculateMaxDataSize(datagram_length);
192-
193-
max_data_size -= info_dst_message_length +
192+
uint32_t overhead = info_dst_message_length +
194193
info_ts_message_length +
195194
data_frag_submessage_header_length +
196195
heartbeat_message_length;
197196

198-
//TODO(Ricardo) inlineqos in future.
199-
200197
#if HAVE_SECURITY
201198
if (getAttributes().security_attributes().is_submessage_protected)
202199
{
203-
max_data_size -= mp_RTPSParticipant->security_manager().calculate_extra_size_for_rtps_submessage(m_guid);
200+
overhead += mp_RTPSParticipant->security_manager().calculate_extra_size_for_rtps_submessage(m_guid);
204201
}
205202

206203
if (getAttributes().security_attributes().is_payload_protected)
207204
{
208-
max_data_size -= mp_RTPSParticipant->security_manager().calculate_extra_size_for_encoded_payload(m_guid);
205+
overhead += mp_RTPSParticipant->security_manager().calculate_extra_size_for_encoded_payload(m_guid);
209206
}
210207
#endif // if HAVE_SECURITY
211208

212209
#ifdef FASTDDS_STATISTICS
213-
max_data_size -= eprosima::fastdds::statistics::rtps::statistics_submessage_length;
210+
overhead += eprosima::fastdds::statistics::rtps::statistics_submessage_length;
214211
#endif // FASTDDS_STATISTICS
215212

213+
constexpr uint32_t min_fragment_size = 4;
214+
if ((overhead + min_fragment_size) > max_data_size)
215+
{
216+
auto min_datagram_length = overhead + min_fragment_size + 1 + (datagram_length - max_data_size);
217+
EPROSIMA_LOG_ERROR(RTPS_WRITER, "Datagram length '" << datagram_length << "' is too small." <<
218+
"At least " << min_datagram_length << " bytes are needed to send a message. Fixing fragments to " <<
219+
min_fragment_size << " bytes.");
220+
return min_fragment_size;
221+
}
222+
223+
max_data_size -= overhead;
216224
return max_data_size;
217225
}
218226

test/blackbox/common/DDSBlackboxTestsListeners.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -2984,7 +2984,7 @@ TEST(DDSStatus, sample_rejected_waitset)
29842984
.disable_heartbeat_piggyback(true)
29852985
.asynchronously(eprosima::fastdds::dds::PublishModeQosPolicyKind::ASYNCHRONOUS_PUBLISH_MODE)
29862986
.add_flow_controller_descriptor_to_pparams( // Be sure are sent in separate submessage each DATA.
2987-
eprosima::fastdds::rtps::FlowControllerSchedulerPolicy::FIFO, 100, 50)
2987+
eprosima::fastdds::rtps::FlowControllerSchedulerPolicy::FIFO, 300, 300) // Be sure the first message is processed before sending the second.
29882988
.init();
29892989

29902990
reader.history_kind(eprosima::fastdds::dds::KEEP_ALL_HISTORY_QOS)

test/unittest/rtps/history/CMakeLists.txt

+19
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,8 @@ set(TOPICPAYLOADPOOLTESTS_SOURCE
7676
${PROJECT_SOURCE_DIR}/src/cpp/utils/IPLocator.cpp
7777
${PROJECT_SOURCE_DIR}/src/cpp/utils/SystemInfo.cpp)
7878

79+
set(WRITERHISTORYTESTS_SOURCE WriterHistoryTests.cpp)
80+
7981
if(WIN32)
8082
add_definitions(-D_WIN32_WINNT=0x0601)
8183
endif()
@@ -164,3 +166,20 @@ target_link_libraries(TopicPayloadPoolTests
164166
GTest::gtest
165167
${CMAKE_DL_LIBS})
166168
gtest_discover_tests(TopicPayloadPoolTests)
169+
170+
171+
172+
add_executable(WriterHistoryTests ${WRITERHISTORYTESTS_SOURCE})
173+
target_compile_definitions(WriterHistoryTests PRIVATE
174+
BOOST_ASIO_STANDALONE
175+
ASIO_STANDALONE
176+
$<$<AND:$<NOT:$<BOOL:${WIN32}>>,$<STREQUAL:"${CMAKE_BUILD_TYPE}","Debug">>:__DEBUG>
177+
$<$<BOOL:${INTERNAL_DEBUG}>:__INTERNALDEBUG> # Internal debug activated.
178+
)
179+
target_link_libraries(WriterHistoryTests
180+
fastcdr
181+
fastdds
182+
foonathan_memory
183+
GTest::gtest
184+
${CMAKE_DL_LIBS})
185+
gtest_discover_tests(WriterHistoryTests)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
// Copyright 2020 Proyectos y Sistemas de Mantenimiento SL (eProsima).
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
#include <gtest/gtest.h>
16+
17+
#include <fastdds/rtps/RTPSDomain.hpp>
18+
#include <fastdds/rtps/participant/RTPSParticipant.hpp>
19+
#include <fastdds/rtps/writer/RTPSWriter.hpp>
20+
#include <fastdds/rtps/history/IPayloadPool.hpp>
21+
#include <fastdds/rtps/history/WriterHistory.hpp>
22+
23+
24+
namespace eprosima {
25+
namespace fastdds {
26+
namespace rtps {
27+
28+
using namespace testing;
29+
30+
#define MAX_MESSAGE_SIZE 300
31+
32+
void cache_change_fragment(
33+
uint32_t max_message_size,
34+
uint32_t inline_qos_length,
35+
bool expected_fragmentation)
36+
{
37+
uint32_t domain_id = 0;
38+
uint32_t initial_reserved_caches = 10;
39+
std::string max_message_size_str = std::to_string(max_message_size);
40+
41+
RTPSParticipantAttributes p_attr;
42+
p_attr.properties.properties().emplace_back("fastdds.max_message_size", max_message_size_str);
43+
RTPSParticipant* participant = RTPSDomain::createParticipant(
44+
domain_id, true, p_attr);
45+
46+
ASSERT_NE(participant, nullptr);
47+
48+
HistoryAttributes h_attr;
49+
h_attr.memoryPolicy = DYNAMIC_RESERVE_MEMORY_MODE;
50+
h_attr.initialReservedCaches = initial_reserved_caches;
51+
h_attr.payloadMaxSize = 250;
52+
WriterHistory* history = new WriterHistory(h_attr);
53+
54+
WriterAttributes w_attr;
55+
RTPSWriter* writer = RTPSDomain::createRTPSWriter(participant, w_attr, history);
56+
57+
ASSERT_NE(writer, nullptr);
58+
59+
CacheChange_t* change = history->create_change(ALIVE);
60+
if (expected_fragmentation)
61+
{
62+
change->serializedPayload.length = 3 * max_message_size;
63+
}
64+
else
65+
{
66+
change->serializedPayload.length = max_message_size / 3;
67+
}
68+
change->inline_qos.length = inline_qos_length;
69+
history->add_change(change);
70+
71+
auto result = change->getFragmentSize();
72+
std::cout << "Fragment size: " << result << std::endl;
73+
if (expected_fragmentation)
74+
{
75+
ASSERT_NE(result, 0);
76+
}
77+
else
78+
{
79+
ASSERT_EQ(result, 0);
80+
}
81+
}
82+
83+
/**
84+
* This test checks the get_max_allowed_payload_size() method of the BaseWriter class.
85+
* When setting the RTPS Participant Attribute property fastdds.max_message_size to a value lower than the
86+
* message overhead, if the method does not overflow the fragment size will be set.
87+
* If the max_message_size is big enough for the overhead, inline_qos and serializedPayload,
88+
* then no fragmentation will occur.
89+
*/
90+
TEST(WriterHistoryTests, get_max_allowed_payload_size_overflow)
91+
{
92+
cache_change_fragment(100, 0, true);
93+
cache_change_fragment(MAX_MESSAGE_SIZE, 0, false);
94+
}
95+
96+
/**
97+
* This test checks the fragment size calculation for a cache change depending on the inline qos length.
98+
* The change.serializedPayload.length is set to 3 times the max_allowed_payload_size, so the fragment size should always be set.
99+
* In case of an overflow in the attribute high_mark_for_frag_ the fragment size will not be set, which is an error.
100+
*/
101+
TEST(WriterHistoryTests, final_high_mark_for_frag_overflow)
102+
{
103+
for (uint32_t inline_qos_length = 0; inline_qos_length < MAX_MESSAGE_SIZE; inline_qos_length += 40)
104+
{
105+
cache_change_fragment(MAX_MESSAGE_SIZE, inline_qos_length, true);
106+
}
107+
}
108+
109+
} // namespace rtps
110+
} // namespace fastdds
111+
} // namespace eprosima
112+
113+
int main(
114+
int argc,
115+
char** argv)
116+
{
117+
testing::InitGoogleTest(&argc, argv);
118+
return RUN_ALL_TESTS();
119+
}

0 commit comments

Comments
 (0)