Skip to content

Commit 726c05f

Browse files
committed
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> (cherry picked from commit 31ece6f)
1 parent bb485c1 commit 726c05f

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
@@ -189,7 +189,14 @@ TopicPayloadPool::PayloadNode* TopicPayloadPool::allocate(
189189
TopicPayloadPool::PayloadNode* TopicPayloadPool::do_allocate(
190190
uint32_t size)
191191
{
192-
PayloadNode* payload = new (std::nothrow) PayloadNode(size);
192+
PayloadNode* payload = nullptr;
193+
try
194+
{
195+
payload = new PayloadNode(size);
196+
}
197+
catch (const std::bad_alloc&)
198+
{
199+
}
193200

194201
if (payload != nullptr)
195202
{

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
@@ -16,6 +16,7 @@
1616

1717
#include <rtps/history/TopicPayloadPool.hpp>
1818

19+
#include <limits>
1920
#include <tuple>
2021

2122
using namespace eprosima::fastrtps::rtps;
@@ -488,6 +489,49 @@ TEST(TopicPayloalPoolTests, dynamic_reusable_memory_zero_size)
488489
do_dynamic_topic_payload_pool_zero_size_test(config);
489490
}
490491

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

0 commit comments

Comments
 (0)