Skip to content

Guard intra-process message construction and adapted ROS conversion #3321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TinyGeek wants to merge 3 commits into
base: rolling
Choose a base branch
from
+374 −10
Open

Guard intra-process message construction and adapted ROS conversion #3321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f9cd4c
Fix intra-process message copy allocation leak
TinyGeek Oct 11, 2026
85b0e94
Guard intra-process message construction and adapted ROS conversion
TinyGeek Oct 15, 2026
e9e3a10
Merge upstream rolling into fix/ipc-copy-exception-leak
TinyGeek Oct 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions rclcpp/include/rclcpp/allocator/allocator_common.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include <cstring>
#include <memory>
#include <utility>

#include "rcl/allocator.h"

Expand Down Expand Up @@ -57,6 +58,20 @@ inline constexpr bool has_get_rcl_allocator_v =
template<typename T, typename Alloc>
using AllocRebind = typename std::allocator_traits<Alloc>::template rebind_traits<T>;

template<typename Traits, typename ... Args>
typename Traits::pointer
allocate_and_construct(typename Traits::allocator_type & allocator, Args &&... args)
{
auto ptr = Traits::allocate(allocator, 1);
try {
Traits::construct(allocator, ptr, std::forward<Args>(args)...);
} catch (...) {
Traits::deallocate(allocator, ptr, 1);
throw;
}
return ptr;
}

template<typename Alloc>
[[deprecated("Conversion of C++ allocators to C style is not valid, as the size on deallocate"
"can not be determined. This will be remove in future versions of ros.")]]
Expand Down
21 changes: 11 additions & 10 deletions rclcpp/include/rclcpp/experimental/intra_process_manager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <vector>
#include <typeinfo>

#include "rclcpp/allocator/allocator_common.hpp"
#include "rclcpp/allocator/allocator_deleter.hpp"
#include "rclcpp/experimental/buffers/intra_process_buffer.hpp"
#include "rclcpp/experimental/ros_message_intra_process_buffer.hpp"
Expand Down Expand Up @@ -627,10 +628,10 @@ class IntraProcessManager
} else {
// Copy the message since we have additional subscriptions to serve
Deleter deleter = message.get_deleter();

@ahcorde ahcorde Oct 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question (pre-existing, but the guard makes it visible): here the copy is allocated with allocator, and the guard deallocates with allocator, but on the success path ownership goes to a deleter copied from message that is never re-pointed. The ROS branch below does call allocator::set_allocator_for_deleter(&deleter, &allocator). With a stateful allocator whose instance differs from the one in message's deleter, wouldn't the success path free through a different allocator than the one that allocated? Should this branch call set_allocator_for_deleter too?

auto ptr = MessageAllocTraits::allocate(allocator, 1);
MessageAllocTraits::construct(allocator, ptr, *message);
auto ptr = allocator::allocate_and_construct<MessageAllocTraits>(allocator, *message);
MessageUniquePtr copied_message(ptr, deleter);

subscription->provide_intra_process_data(MessageUniquePtr(ptr, deleter));
subscription->provide_intra_process_data(std::move(copied_message));
}

continue;
Expand All @@ -651,12 +652,13 @@ class IntraProcessManager

if constexpr (rclcpp::TypeAdapter<MessageT, ROSMessageType>::is_specialized::value) {
ROSMessageTypeAllocator ros_message_alloc(allocator);
auto ptr = ROSMessageTypeAllocatorTraits::allocate(ros_message_alloc, 1);
ROSMessageTypeAllocatorTraits::construct(ros_message_alloc, ptr);
ROSMessageTypeDeleter deleter;
allocator::set_allocator_for_deleter(&deleter, &allocator);
rclcpp::TypeAdapter<MessageT, ROSMessageType>::convert_to_ros_message(*message, *ptr);
auto ptr = allocator::allocate_and_construct<ROSMessageTypeAllocatorTraits>(
ros_message_alloc);
auto ros_msg = std::unique_ptr<ROSMessageType, ROSMessageTypeDeleter>(ptr, deleter);
rclcpp::TypeAdapter<MessageT, ROSMessageType>::convert_to_ros_message(
*message, *ros_msg);
ros_message_subscription->provide_intra_process_message(std::move(ros_msg));
} else {
if constexpr (std::is_same<MessageT, ROSMessageType>::value) {
Expand All @@ -669,11 +671,10 @@ class IntraProcessManager
// Copy the message since we have additional subscriptions to serve
Deleter deleter = message.get_deleter();
allocator::set_allocator_for_deleter(&deleter, &allocator);
auto ptr = MessageAllocTraits::allocate(allocator, 1);
MessageAllocTraits::construct(allocator, ptr, *message);
auto ptr = allocator::allocate_and_construct<MessageAllocTraits>(allocator, *message);
MessageUniquePtr copied_message(ptr, deleter);

ros_message_subscription->provide_intra_process_message(
MessageUniquePtr(ptr, deleter));
ros_message_subscription->provide_intra_process_message(std::move(copied_message));
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions rclcpp/test/rclcpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,12 @@ ament_add_ros_isolated_gtest(test_parameter_map test_parameter_map.cpp)
if(TARGET test_parameter_map)
target_link_libraries(test_parameter_map ${PROJECT_NAME} rcl::rcl rcl_yaml_param_parser::rcl_yaml_param_parser rcutils::rcutils)
endif()
ament_add_ros_isolated_gtest(
test_intra_process_copy_exception test_intra_process_copy_exception.cpp)
if(TARGET test_intra_process_copy_exception)
target_link_libraries(test_intra_process_copy_exception ${PROJECT_NAME} std_msgs::std_msgs)
endif()

ament_add_ros_isolated_gtest(test_publisher test_publisher.cpp TIMEOUT 120)
ament_add_test_label(test_publisher mimick)
if(TARGET test_publisher)
Expand Down
Loading
Loading