Skip to content

Commit 2da4ffc

Browse files
Arithmetic overflow in fragment size calculations (#5464)
* 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)
1 parent 28552ce commit 2da4ffc

File tree

5 files changed

+143
-10
lines changed

5 files changed

+143
-10
lines changed

src/cpp/rtps/history/WriterHistory.cpp

+10-3
Original file line numberDiff line numberDiff line change
@@ -320,9 +320,16 @@ void WriterHistory::set_fragments(
320320
// If inlineqos for related_sample_identity is required, then remove its size from the final fragment size.
321321
if (0 < inline_qos_size)
322322
{
323-
final_high_mark_for_frag -= (
324-
fastdds::dds::ParameterSerializer<Parameter_t>::PARAMETER_SENTINEL_SIZE +
325-
inline_qos_size);
323+
uint32_t overhead = fastdds::dds::ParameterSerializer<Parameter_t>::PARAMETER_SENTINEL_SIZE + inline_qos_size;
324+
constexpr uint32_t min_fragment_size = 4;
325+
if (final_high_mark_for_frag < (overhead + min_fragment_size))
326+
{
327+
final_high_mark_for_frag = min_fragment_size;
328+
}
329+
else
330+
{
331+
final_high_mark_for_frag -= overhead;
332+
}
326333
}
327334

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

src/cpp/rtps/participant/RTPSParticipantImpl.cpp

+8-6
Original file line numberDiff line numberDiff line change
@@ -2390,20 +2390,22 @@ uint32_t RTPSParticipantImpl::getMaxDataSize()
23902390
uint32_t RTPSParticipantImpl::calculateMaxDataSize(
23912391
uint32_t length)
23922392
{
2393-
uint32_t maxDataSize = length;
2394-
2393+
// RTPS header
2394+
uint32_t overhead = RTPSMESSAGE_HEADER_SIZE;
23952395
#if HAVE_SECURITY
23962396
// If there is rtps messsage protection, reduce max size for messages,
23972397
// because extra data is added on encryption.
23982398
if (security_attributes_.is_rtps_protected)
23992399
{
2400-
maxDataSize -= m_security_manager.calculate_extra_size_for_rtps_message();
2400+
overhead += m_security_manager.calculate_extra_size_for_rtps_message();
24012401
}
24022402
#endif // if HAVE_SECURITY
24032403

2404-
// RTPS header
2405-
maxDataSize -= RTPSMESSAGE_HEADER_SIZE;
2406-
return maxDataSize;
2404+
if (length <= overhead)
2405+
{
2406+
return 0;
2407+
}
2408+
return length - overhead;
24072409
}
24082410

24092411
bool RTPSParticipantImpl::networkFactoryHasRegisteredTransports() const

test/blackbox/common/DDSBlackboxTestsListeners.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -2942,7 +2942,7 @@ TEST(DDSStatus, sample_rejected_waitset)
29422942
.disable_heartbeat_piggyback(true)
29432943
.asynchronously(eprosima::fastdds::dds::PublishModeQosPolicyKind::ASYNCHRONOUS_PUBLISH_MODE)
29442944
.add_throughput_controller_descriptor_to_pparams( // Be sure are sent in separate submessage each DATA.
2945-
eprosima::fastdds::rtps::FlowControllerSchedulerPolicy::FIFO, 100, 50)
2945+
eprosima::fastdds::rtps::FlowControllerSchedulerPolicy::FIFO, 300, 300)
29462946
.init();
29472947

29482948
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
@@ -52,6 +52,8 @@ set(TOPICPAYLOADPOOLTESTS_SOURCE
5252
${PROJECT_SOURCE_DIR}/src/cpp/rtps/common/Time_t.cpp
5353
${PROJECT_SOURCE_DIR}/src/cpp/utils/SystemInfo.cpp)
5454

55+
set(WRITERHISTORYTESTS_SOURCE WriterHistoryTests.cpp)
56+
5557
if(WIN32)
5658
add_definitions(-D_WIN32_WINNT=0x0601)
5759
endif()
@@ -121,9 +123,26 @@ target_link_libraries(TopicPayloadPoolTests
121123
${CMAKE_DL_LIBS})
122124
add_gtest(TopicPayloadPoolTests SOURCES ${TOPICPAYLOADPOOLTESTS_SOURCE})
123125

126+
127+
add_executable(WriterHistoryTests ${WRITERHISTORYTESTS_SOURCE})
128+
target_compile_definitions(WriterHistoryTests PRIVATE
129+
BOOST_ASIO_STANDALONE
130+
ASIO_STANDALONE
131+
$<$<AND:$<NOT:$<BOOL:${WIN32}>>,$<STREQUAL:"${CMAKE_BUILD_TYPE}","Debug">>:__DEBUG>
132+
$<$<BOOL:${INTERNAL_DEBUG}>:__INTERNALDEBUG> # Internal debug activated.
133+
)
134+
target_link_libraries(WriterHistoryTests
135+
fastcdr
136+
fastrtps
137+
foonathan_memory
138+
GTest::gtest
139+
${CMAKE_DL_LIBS})
140+
add_gtest(WriterHistoryTests SOURCES ${WRITERHISTORYTESTS_SOURCE})
141+
124142
if(ANDROID)
125143
set_property(TARGET ReaderHistoryTests PROPERTY CROSSCOMPILING_EMULATOR "adb;shell;cd;${CMAKE_CURRENT_BINARY_DIR};&&")
126144
set_property(TARGET BasicPoolsTests PROPERTY CROSSCOMPILING_EMULATOR "adb;shell;cd;${CMAKE_CURRENT_BINARY_DIR};&&")
127145
set_property(TARGET CacheChangePoolTests PROPERTY CROSSCOMPILING_EMULATOR "adb;shell;cd;${CMAKE_CURRENT_BINARY_DIR};&&")
128146
set_property(TARGET CacheChangeTests PROPERTY CROSSCOMPILING_EMULATOR "adb;shell;cd;${CMAKE_CURRENT_BINARY_DIR};&&")
147+
set_property(TARGET WriterHistoryTests PROPERTY CROSSCOMPILING_EMULATOR "adb;shell;cd;${CMAKE_CURRENT_BINARY_DIR};&&")
129148
endif()
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
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 <fastrtps/rtps/RTPSDomain.h>
18+
#include <fastrtps/rtps/participant/RTPSParticipant.h>
19+
#include <fastrtps/rtps/writer/RTPSWriter.h>
20+
#include <fastrtps/rtps/history/WriterHistory.h>
21+
22+
23+
namespace eprosima {
24+
namespace fastrtps {
25+
namespace rtps {
26+
27+
using namespace testing;
28+
29+
#define MAX_MESSAGE_SIZE 300
30+
31+
void cache_change_fragment(
32+
uint32_t max_message_size,
33+
uint32_t inline_qos_length,
34+
bool expected_fragmentation)
35+
{
36+
uint32_t domain_id = 0;
37+
uint32_t initial_reserved_caches = 10;
38+
std::string max_message_size_str = std::to_string(max_message_size);
39+
40+
RTPSParticipantAttributes p_attr;
41+
p_attr.properties.properties().emplace_back("fastdds.max_message_size", max_message_size_str);
42+
RTPSParticipant* participant = RTPSDomain::createParticipant(
43+
domain_id, true, p_attr);
44+
45+
ASSERT_NE(participant, nullptr);
46+
47+
HistoryAttributes h_attr;
48+
h_attr.memoryPolicy = DYNAMIC_RESERVE_MEMORY_MODE;
49+
h_attr.initialReservedCaches = initial_reserved_caches;
50+
h_attr.payloadMaxSize = 250;
51+
WriterHistory* history = new WriterHistory(h_attr);
52+
53+
WriterAttributes w_attr;
54+
RTPSWriter* writer = RTPSDomain::createRTPSWriter(participant, w_attr, history);
55+
56+
ASSERT_NE(writer, nullptr);
57+
58+
CacheChange_t* change = writer->new_change(ALIVE);
59+
if (expected_fragmentation)
60+
{
61+
change->serializedPayload.length = 3 * max_message_size;
62+
}
63+
else
64+
{
65+
change->serializedPayload.length = max_message_size / 3;
66+
}
67+
change->inline_qos.length = inline_qos_length;
68+
history->add_change(change);
69+
70+
auto result = change->getFragmentSize();
71+
std::cout << "Fragment size: " << result << std::endl;
72+
if (expected_fragmentation)
73+
{
74+
ASSERT_NE(result, 0);
75+
}
76+
else
77+
{
78+
ASSERT_EQ(result, 0);
79+
}
80+
}
81+
82+
/**
83+
* This test checks the fragment size calculation for a cache change depending on the inline qos length.
84+
* The change.serializedPayload.length is set to 3 times the max_allowed_payload_size, so the fragment size should always be set.
85+
* In case of an overflow in the attribute high_mark_for_frag_ the fragment size will not be set, which is an error.
86+
*/
87+
TEST(WriterHistoryTests, final_high_mark_for_frag_overflow)
88+
{
89+
for (uint32_t inline_qos_length = 0; inline_qos_length < MAX_MESSAGE_SIZE; inline_qos_length += 40)
90+
{
91+
cache_change_fragment(MAX_MESSAGE_SIZE, inline_qos_length, true);
92+
}
93+
}
94+
95+
} // namespace rtps
96+
} // namespace fastdds
97+
} // namespace eprosima
98+
99+
int main(
100+
int argc,
101+
char** argv)
102+
{
103+
testing::InitGoogleTest(&argc, argv);
104+
return RUN_ALL_TESTS();
105+
}

0 commit comments

Comments
 (0)