Skip to content

Commit 31ece6f

Browse files
Catch exception when constructing a payload pool Node (#6460)
* Refs #25475. Regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #25475. Capture exception when constructing PayloadNode. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #25475. Bonus: avoid using sizeof(NodeInfo). Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #25475. Fix uncrustify. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
1 parent bc607cc commit 31ece6f

3 files changed

Lines changed: 53 additions & 2 deletions

File tree

src/cpp/rtps/history/TopicPayloadPool.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,14 @@ TopicPayloadPool::PayloadNode* TopicPayloadPool::allocate(
180180
TopicPayloadPool::PayloadNode* TopicPayloadPool::do_allocate(
181181
uint32_t size)
182182
{
183-
PayloadNode* payload = new (std::nothrow) PayloadNode(size);
183+
PayloadNode* payload = nullptr;
184+
try
185+
{
186+
payload = new PayloadNode(size);
187+
}
188+
catch (const std::bad_alloc&)
189+
{
190+
}
184191

185192
if (payload != nullptr)
186193
{

src/cpp/rtps/history/TopicPayloadPool.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ class TopicPayloadPool : public ITopicPayloadPool
147147
}
148148
else
149149
{
150-
buffer = (octet*)calloc(size + sizeof(NodeInfo) - 1, sizeof(octet));
150+
buffer = (octet*)calloc(size + data_offset, sizeof(octet));
151151
}
152152

153153
if (buffer == nullptr)

test/unittest/rtps/history/TopicPayloadPoolTests.cpp

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <rtps/history/TopicPayloadPool.hpp>
1818
#include <fastdds/rtps/common/CacheChange.hpp>
1919

20+
#include <limits>
2021
#include <tuple>
2122

2223
using namespace eprosima::fastdds::rtps;
@@ -483,6 +484,49 @@ TEST(TopicPayloalPoolTests, dynamic_reusable_memory_zero_size)
483484
do_dynamic_topic_payload_pool_zero_size_test(config);
484485
}
485486

487+
//! Regression test for redmine issue #25475
488+
//! When the allocation of a new payload node fails, the pool must report the
489+
//! failure gracefully (returning false) instead of letting the std::bad_alloc
490+
//! thrown by the PayloadNode constructor propagate out of get_payload.
491+
//! Might require to be run in a constrained environment (i.e. container with limited memory)
492+
//! to actually trigger the allocation failure, but will not fail if the allocation succeeds.
493+
TEST(TopicPayloalPoolTests, bad_alloc_on_get_payload_is_handled)
494+
{
495+
// An infinite history so that the pool is allowed to allocate on demand
496+
// (otherwise allocate() would short-circuit on the maximum pool size).
497+
PoolConfig config{ DYNAMIC_RESERVE_MEMORY_MODE, 128, 0, 0 };
498+
499+
std::unique_ptr<ITopicPayloadPool> pool = TopicPayloadPool::get(config);
500+
ASSERT_NE(pool, nullptr);
501+
pool->reserve_history(config, false);
502+
503+
// Request a payload so large that the underlying calloc is expected to fail,
504+
// making the PayloadNode constructor throw std::bad_alloc.
505+
constexpr uint32_t huge_size = (std::numeric_limits<uint32_t>::max)();
506+
507+
CacheChange_t change;
508+
bool result = true;
509+
510+
// The pool must never let the exception escape.
511+
EXPECT_NO_THROW(result = pool->get_payload(huge_size, change.serializedPayload));
512+
513+
if (result)
514+
{
515+
// On a platform able to satisfy such a large allocation, release it.
516+
EXPECT_EQ(huge_size, change.serializedPayload.max_size);
517+
pool->release_payload(change.serializedPayload);
518+
}
519+
else
520+
{
521+
// On allocation failure the payload must be left in a clean state.
522+
EXPECT_EQ(change.serializedPayload.data, nullptr);
523+
EXPECT_EQ(change.serializedPayload.max_size, 0u);
524+
EXPECT_EQ(change.serializedPayload.payload_owner, nullptr);
525+
}
526+
527+
pool->release_history(config, false);
528+
}
529+
486530
#ifdef INSTANTIATE_TEST_SUITE_P
487531
#define GTEST_INSTANTIATE_TEST_MACRO(x, y, z) INSTANTIATE_TEST_SUITE_P(x, y, z)
488532
#else

0 commit comments

Comments
 (0)