[TinyGeek]

Description

When IntraProcessManager::add_owned_msg_to_buffers() copies an owning message, a throwing constructor must release the newly allocated storage. The same manager also constructs a ROS message for TypeAdapter delivery before calling user conversion code; if construction or conversion throws, that storage remains allocated. On the unmodified Jazzy and Rolling sources, a throwing TypeAdapter conversion leaks one 32-byte ROS message in the tested intra-process path.

This update adds allocator::allocate_and_construct<Traits>() beside AllocRebind to deallocate storage when construction throws, and uses it at the manager's three allocation sites. In the TypeAdapter branch, the completed ROS message enters its existing unique_ptr before convert_to_ros_message() runs, so conversion exceptions destroy and deallocate it. The copy-delivery and tracepoint flow remains the same. Existing public method signatures and class layouts are unchanged; the helper is an additive template in an installed header.

Refs #3320

Is this user-facing behavior change?

Yes, on the exception paths: the existing exception still propagates, while the unfinished allocation or completed adapted ROS message is cleaned up. Normal owning delivery and the last subscriber's original message are preserved.

Did you use Generative AI?

Yes. OpenAI Codex CLI (model gpt-6-sol) assisted with the helper, manager changes, regression tests, local sanitizer runs, and this description.

Assisted-by: Codex:gpt-6-sol

Additional Information

• The original two copy-constructor regression cases failed before the fix and pass after it. The updated registered test contains 7 cases, including helper construction failure, adapted ROS construction and conversion failures, and an exception from provide_intra_process_message() after ownership transfer.
• Jazzy and Rolling source-built related CTest targets: 4/4 passed on each distribution. The 7-case test also passed with ASan/UBSan/LSan in both isolated Fast DDS rootfs environments.
• The same out-of-tree TypeAdapter publisher → ROS-message subscription reproducer leaked 32 bytes before the update. With the updated Rolling and Jazzy sources, 10/10 intra-process runs on each distribution propagated the expected conversion exception without an LSan finding.
• copyright and cpplint passed on both distributions. uncrustify initially requested one template-spacing change and passed after that change.
• After merging current Rolling (0e5589a), a same-rootfs Debug (-g -Og, BUILD_TESTING=OFF) abidiff against that upstream revision exited 4: two added weak allocate_and_construct instantiations, with no removed or changed functions or variables. This local comparison does not replace the upstream ABI workflow. An earlier supplemental Jazzy build's abidiff exited 12 for one removed and one added weak ParameterValue::get symbol outside the changed code; Jazzy ABI was not rechecked after the latest changes, so Jazzy ABI compatibility is not claimed.
• Other allocation sites and the existing stateful allocator/deleter mismatch remain outside this PR. The broader issue can stay open while their scope is discussed.

[ahcorde]

Thanks for tackling #3320! The guard fixes the two copy sites in add_owned_msg_to_buffers, but the same allocate-then-construct leak exists in a few more places, so I don't think this fully closes the issue yet.

TypeAdapter branch is still unguarded (intra_process_manager.hpp ~L654-662, outside this diff): ROSMessageTypeAllocatorTraits::allocate + construct, and then convert_to_ros_message (user code), all run before the unique_ptr takes ownership. If the default constructor or the conversion throws, the storage leaks (and in the conversion case the constructed message is never destroyed either). Could this branch get the same treatment plus a test case?

Same pattern elsewhere:

• rclcpp/include/rclcpp/experimental/buffers/intra_process_buffer.hpp (~L193 add_shared_impl, ~L239 consume_unique, ~L303)
• rclcpp/include/rclcpp/any_subscription_callback.hpp (~L499, ~L521)
• rclcpp/include/rclcpp/publisher.hpp (~L558, ~L567, ~L583)

With the CopyMessage type from this test, e.g. a shared publish into a UniquePtr buffer, or consume_unique on a shared buffer, still leaks one message per throw.

[ahcorde]

Rather than inlining this guard (here and again at L679-685, where the two copies already differ slightly), what about adding one exception-safe helper next to AllocRebind in rclcpp/allocator/allocator_common.hpp, something like allocate_and_construct<Traits>(alloc, args...) that deallocates if construction throws? That would let all the other sites listed in the review summary be fixed the same way, without copying this 6-line block around.

[ahcorde]

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?

[ahcorde]

nit: could this include the granular headers it actually uses (rclcpp/node.hpp, rclcpp/experimental/intra_process_manager.hpp, the intra-process buffer headers, ...) instead of the umbrella rclcpp/rclcpp.hpp? We're moving rclcpp tests away from the umbrella header to cut compile time.

[ahcorde]

nit: std::size_t is used here without #include <cstddef>; it only compiles via transitive includes.

[ahcorde]

The cases cover only plain data buffers and ROS-message buffers. Could you add:

• a TypeAdapter publisher → ROS-message subscription case (convert_to_ros_message throwing), which would catch the remaining leak in that branch, and
• a case where provide_intra_process_* throws after the guard has released ownership, so a future reordering of release() vs. constructing copied_message is caught?

[TinyGeek]

@ahcorde Thanks for the detailed review and the helper suggestion. I found the leak documented in #3320 while investigating #1860, which I set aside when the optimization grew too broad. After your review, I reproduced the remaining TypeAdapter failure and related exception-safety failures at 13 other allocation sites on unmodified Jazzy and Rolling. I haven't changed this PR yet.

I understand allocate_and_construct<Traits>(alloc, args...) as a helper that deallocates if construction throws, then returns the constructed pointer. In the TypeAdapter branch, I would immediately put that pointer in the existing unique_ptr before calling convert_to_ros_message(), so a conversion exception also destroys the object. Is that the ownership boundary you had in mind, or did you intend the helper to return an owning object?

Would you be comfortable keeping #3321 focused on IntraProcessManager, the helper, and the requested regression tests, with the other sites addressed in follow-up PRs? I also reproduced the stateful allocator/deleter mismatch on the normal path. Should that be included here or tracked separately?

I can share the reproducers and sanitizer logs, and adjust Fixes #3320 once we agree on the scope.

I've also noted the two include-related comments and both requested regression cases, including the throwing provide_intra_process_* path. I'll address those when updating the PR after we agree on the scope.

[fujitatomoya]

i think this fixes the part of the original problem.
saying that TypeAdapter publisher to ROS-message subscription still leaks on throw?

[TinyGeek]

i think this fixes the part of the original problem. saying that TypeAdapter publisher to ROS-message subscription still leaks on throw?

Yes. I replicated this with a real TypeAdapter publisher and a ROS-message intra-process subscription with a convert_to_ros_message() implementation that throws.

ROS message allocation in IntraProcessManager::add_owned_msg_to_buffers() before calling the converter. Total subscriptions: 1 Intra-process subscriptions: 1 LeakSanitizer reports a 32-byte leak on both the unmodified Jazzy/Rolling and the current PR-patched code.

The leak reproduces in 10/10 runs on each of the unmodified distributions and 9/10 runs on each of the patched distributions. The two non-leaking runs reported 2 total subscriptions / 1 intra-process subscripton, which made the publisher take a different path.

This TypeAdapter branch is not covered by the current copy-constructor guards. I have the standalone reproducer, sanitizer logs and conditions under which it was run.

[TinyGeek]

@ahcorde @fujitatomoya
The update introduces the suggested allocate_and_construct helper and ensures the constructed ROS message is owned by its unique_ptr before convert_to_ros_message() runs.
I've also added regression coverage for construction, conversion, and delivery exceptions, and addressed the include-related comments.
Validation:

• Jazzy and Rolling: 4/4 related CTests passed.
• All 7 regression cases passed under ASan/UBSan/LSan on both distributions.
• The separate TypeAdapter publisher → ROS-message subscription reproducer completed 10/10 runs on each distribution without LSan findings.
• Rolling abidiff exited 0. PR CI is still pending.
  I changed Fixes Copy constructor exception leaks intra-process message allocation #3320 to Refs Copy constructor exception leaks intra-process message allocation #3320 because other allocation sites remain. The existing stateful allocator/deleter mismatch is also unchanged. I'd prefer addressing those separately, but I'm happy to adjust the scope based on your feedback.
  Could you take another look at the helper's ownership boundary, the TypeAdapter branch, and the proposed scope when you have a chance?
  Thanks again!

[github-actions]

ABI Compliance Check

❌ Verdict: incompatible

Library	Verdict	Summary
libcomponent_manager.so	✅ compatible	No ABI changes detected.
librclcpp.so	❌ incompatible	ABI-incompatible changes detected.
librclcpp_action.so	✅ compatible	No ABI changes detected.
librclcpp_lifecycle.so	✅ compatible	No ABI changes detected.

✅ libcomponent_manager.so — full abidiff report

Compared:

• Base: lib-base/libcomponent_manager.so
• Head: lib-pr/libcomponent_manager.so @ 85b0e94

(empty report — no differences printed by abidiff)

❌ librclcpp.so — full abidiff report

Compared:

• Base: lib-base/librclcpp.so
• Head: lib-pr/librclcpp.so @ 85b0e94

Functions changes summary: 1 Removed (41 filtered out), 0 Changed (79 filtered out), 2 Added (19 filtered out) functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 Removed function:

  [D] 'method rclcpp::WaitResult<rclcpp::WaitSetTemplate<rclcpp::wait_set_policies::SequentialSynchronization, rclcpp::wait_set_policies::DynamicStorage> >::~WaitResult()'    {_ZN6rclcpp10WaitResultINS_15WaitSetTemplateINS_17wait_set_policies25SequentialSynchronizationENS2_14DynamicStorageEEEED2Ev, aliases _ZN6rclcpp10WaitResultINS_15WaitSetTemplateINS_17wait_set_policies25SequentialSynchronizationENS2_14DynamicStorageEEEED1Ev}

2 Added functions:

  [A] 'function std::allocator_traits<std::allocator<rcl_interfaces::msg::ParameterEvent_<std::allocator<void> > > >::pointer rclcpp::allocator::allocate_and_construct<std::allocator_traits<std::allocator<rcl_interfaces::msg::ParameterEvent_<std::allocator<void> > > >, rcl_interfaces::msg::ParameterEvent_<std::allocator<void> >&>(std::allocator_traits<std::allocator<rcl_interfaces::msg::ParameterEvent_<std::allocator<void> > > >::allocator_type&)'    {_ZN6rclcpp9allocator22allocate_and_constructISt16allocator_traitsISaIN14rcl_interfaces3msg15ParameterEvent_ISaIvEEEEEJRS7_EEENT_7pointerERNSB_14allocator_typeEDpOT0_}
  [A] 'function std::allocator_traits<std::allocator<rosgraph_msgs::msg::Clock_<std::allocator<void> > > >::pointer rclcpp::allocator::allocate_and_construct<std::allocator_traits<std::allocator<rosgraph_msgs::msg::Clock_<std::allocator<void> > > >, rosgraph_msgs::msg::Clock_<std::allocator<void> >&>(std::allocator_traits<std::allocator<rosgraph_msgs::msg::Clock_<std::allocator<void> > > >::allocator_type&)'    {_ZN6rclcpp9allocator22allocate_and_constructISt16allocator_traitsISaIN13rosgraph_msgs3msg6Clock_ISaIvEEEEEJRS7_EEENT_7pointerERNSB_14allocator_typeEDpOT0_}

✅ librclcpp_action.so — full abidiff report

Compared:

• Base: lib-base/librclcpp_action.so
• Head: lib-pr/librclcpp_action.so @ 85b0e94

(empty report — no differences printed by abidiff)

✅ librclcpp_lifecycle.so — full abidiff report

Compared:

• Base: lib-base/librclcpp_lifecycle.so
• Head: lib-pr/librclcpp_lifecycle.so @ 85b0e94

(empty report — no differences printed by abidiff)

_{Updated for commit 85b0e94 · check run · suppressions: /home/runner/work/_temp/ros2-abi-suppressions.txt}

[TinyGeek]

@ahcorde @fujitatomoya
I've pushed an update merging the latest Rolling to address the WaitResult ABI discrepancy.