Skip to content

Commit e69a5b6

Browse files
clalancetteKarsten1987
authored andcommitted
Fix UBSAN warnings in any_subscription_callback. (#1551)
When running the rclcpp tests under UBSAN, it complained that the AnySubscriptionCallback tests were using an uninitialized allocator. Reviewing the code there, it also looks like the AnySubscriptionCallback constructor wasn't checking for a null allocator. Fix this by doing 3 things: 1. Add a check for a null allocator in the AnySubscriptionCallback constructor. 2. Add a new test to test_any_subscription_callback that tests the new nullptr handling. 3. Fix the test fixture to initialize the allocator before trying to call the AnySubscriptionCallback constructor. Signed-off-by: Chris Lalancette <[email protected]>
1 parent 3f3d0f9 commit e69a5b6

File tree

2 files changed

+19
-1
lines changed

2 files changed

+19
-1
lines changed

rclcpp/include/rclcpp/any_subscription_callback.hpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,9 @@ class AnySubscriptionCallback
6565
const_shared_ptr_callback_(nullptr), const_shared_ptr_with_info_callback_(nullptr),
6666
unique_ptr_callback_(nullptr), unique_ptr_with_info_callback_(nullptr)
6767
{
68+
if (allocator == nullptr) {
69+
throw std::runtime_error("invalid allocator");
70+
}
6871
message_allocator_ = std::make_shared<MessageAlloc>(*allocator.get());
6972
allocator::set_allocator_for_deleter(&message_deleter_, message_allocator_.get());
7073
}

rclcpp/test/rclcpp/test_any_subscription_callback.cpp

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ class TestAnySubscriptionCallback : public ::testing::Test
2626
{
2727
public:
2828
TestAnySubscriptionCallback()
29-
: any_subscription_callback_(allocator_) {}
29+
: allocator_(std::make_shared<std::allocator<void>>()),
30+
any_subscription_callback_(allocator_) {}
3031
void SetUp()
3132
{
3233
msg_shared_ptr_ = std::make_shared<test_msgs::msg::Empty>();
@@ -45,6 +46,20 @@ class TestAnySubscriptionCallback : public ::testing::Test
4546
rclcpp::MessageInfo message_info_;
4647
};
4748

49+
void construct_with_null_allocator()
50+
{
51+
// We need to wrap this in a function because `EXPECT_THROW` is a macro, and thinks
52+
// that the comma in here splits macro arguments, not the template arguments.
53+
rclcpp::AnySubscriptionCallback<
54+
test_msgs::msg::Empty, std::allocator<void>> any_subscription_callback_(nullptr);
55+
}
56+
57+
TEST(AnySubscription, null_allocator) {
58+
EXPECT_THROW(
59+
construct_with_null_allocator(),
60+
std::runtime_error);
61+
}
62+
4863
TEST_F(TestAnySubscriptionCallback, construct_destruct) {
4964
}
5065

0 commit comments

Comments
 (0)