[Aaravanand00]

Description

This PR implements the proper transfer of ownership between the RMW and the rcl layer for loaned messages, resolving a safety gap where loaned memory could be overwritten by the middleware if a user stored the shared_ptr for asynchronous processing.

Previously, rclcpp::Executor::execute_subscription unconditionally returned the loaned message immediately after the callback execution. With this change, the rcl_return_loaned_message_from_subscription is now tied to the destructor of the shared_ptr provided to the user.

• Created a custom deleter inside Subscription::handle_loaned_message which captures the rcl_subscription_t and safely returns the loaned message when the reference count drops to zero.
• Added a std::mutex (loaned_message_mutex_) in SubscriptionBase to ensure thread-safe execution of rcl_return and rcl_take.
• Removed the immediate rcl_return from executor.cpp.
• Removed the obsolete RCLCPP_WARN_ONCE about shared_ptr callbacks being unsafe for loaned messages.

Fixes #3119

Is this user-facing behavior change?

Yes, advanced users relying on zero-copy transport can now safely retain a std::shared_ptr to the loaned message. The underlying RMW memory is not returned to the pool until all references to the pointer are released.

Did you use Generative AI?

Yes, I used Antigravity (Google DeepMind agentic coding assistant) to help implement the shared_ptr custom deleter pattern and the thread-safe mutex logic across subscription_base.hpp/cpp, subscription.hpp, and executor.cpp.

Additional Information

This fix is based on the feedback from @jmachowinski to properly transfer ownership back and forth instead of introducing a new is_data_valid API. It also addresses the TODO(clalancette) in subscription_base.cpp about handling this with a custom deleter and proper locking.

[Aaravanand00]

@fujitatomoya ptal...

[fujitatomoya]

@Aaravanand00 thakns for working on the PR. can you see my review comments that need to be addressed.

@jmachowinski can you have a look at this? looks like this is what you are asking for?

[Aaravanand00]

@fujitatomoya Done I have moved the shared_ptr construction to the very top of handle_loaned_message before the intra-process check to fix the leak. I also removed the invalid handle_loaned_message test from test_subscription.cpp as it causes UB with a loaning RMW...

[clalancette]

First of all, thank you for going in and fixing this long-standing bug, it is much appreciated.

I found two potential issues, please take a look and tell me what you thin.

[Aaravanand00]

First of all, thank you for going in and fixing this long-standing bug, it is much appreciated.

I found two potential issues, please take a look and tell me what you thin.

@clalancette I agree with your points and IMO you are absolutely right on both issues :-

1. Exposing the lock directly definitely leaks implementation details and could lead to misuse. Handling the lock internally inside SubscriptionBase is a much cleaner and safer approach.
2. You are spot on about the callback lifetime. Since the subscription_handle_ can now outlive SubscriptionBase, the RMW could potentially trigger a callback on a destroyed object (Use-After-Free).

I have updated the pr to address both -->

1. Replaced the mutex getter with a take_loaned_message() API on SubscriptionBase to handle the locking internally.
2. Added this->clear_on_new_message_callback() in the SubscriptionBase destructor to explicitly unregister the callback and prevent the lifetime issue.
   let me know is this looks good?????...

[Aaravanand00]

@clalancette @fujitatomoya if all looks good then merge this...

[github-actions]

ABI Compliance Check

✅ Verdict: compatible

Library	Verdict	Summary
libcomponent_manager.so	✅ compatible	No ABI changes detected.
librclcpp.so	✅ compatible	No ABI 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 @ 1ee868d

(empty report — no differences printed by abidiff)

✅ librclcpp.so — full abidiff report

Compared:

• Base: lib-base/librclcpp.so
• Head: lib-pr/librclcpp.so @ 1ee868d

Functions changes summary: 0 Removed (41 filtered out), 0 Changed (85 filtered out), 0 Added (22 filtered out) functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

✅ librclcpp_action.so — full abidiff report

Compared:

• Base: lib-base/librclcpp_action.so
• Head: lib-pr/librclcpp_action.so @ 1ee868d

(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 @ 1ee868d

(empty report — no differences printed by abidiff)

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

[clalancette]

We are down to a few nits inline now, though the destructor change is enough for me to ask for one more round of fixes.

I will note that the loaned_message_mutex (and hence take_loaned_message) is probably unnecessary; rmw_take_loaned_message says that it is thread-safe, and the only implementation (rmw_fastrtps) makes that so. Documentation for rcl_take_loaned_message does not have that guarantee, though in practice it is thread-safe. I'd lean towards removing the mutex, but if you think we should keep it, I'm OK with that too. Just let me know which way you decide.

[Aaravanand00]

We are down to a few nits inline now, though the destructor change is enough for me to ask for one more round of fixes.

I will note that the loaned_message_mutex (and hence take_loaned_message) is probably unnecessary; rmw_take_loaned_message says that it is thread-safe, and the only implementation (rmw_fastrtps) makes that so. Documentation for rcl_take_loaned_message does not have that guarantee, though in practice it is thread-safe. I'd lean towards removing the mutex, but if you think we should keep it, I'm OK with that too. Just let me know which way you decide.

check now all looks good???

[clalancette]

Looks good to me now. Thank you for all of the iteration. I'll run CI on it next.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Linux-rhel
• Windows