Fix loaned message ownership transfer by returning loans via shared_ptr custom deleter

[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.

[clalancette]

I'm not a fan of exposing implementations details like this, particularly around locks. It just makes it much harder to see how the locking is used and expected to be used. I'm going to suggest that we change the implementation to a take_loaned_message API here on SubscriptionBase which holds the lock internally, removing the need for this.

[clalancette]

I think we have a lifetime issue with sub_handle now if a user holds onto the message shared pointer. The deleter captures both sub_handle and loan_mutex shared pointers, so if SubscriptionBase gets destructed, those are both still valid until the user drops it. That is fine.

However, the comments in

rclcpp/rclcpp/include/rclcpp/subscription_base.hpp

Lines 583 to 589 in 249154a

	// It is important to declare on_new_message_callback_ before
	// subscription_handle_, so on destruction the subscription is
	// destroyed first. Otherwise, the rmw subscription callback
	// would point briefly to a destroyed function.
	std::function<void(size_t)> on_new_message_callback_{nullptr};
	// Declare subscription_handle_ after callback
	std::shared_ptr<rcl_subscription_t> subscription_handle_;

explicitly say that subscription_handle_ to be destroyed before on_new_message_callback so we don't end up in a situation where the callback can fire on a destroyed subscription_handle_. That is now violated because the subscription_handle_ can outlive the SubscriptionBase. We can probably get away with explicitly clearing the on_new_message_callback in the SubscriptionBase destructor to make sure this doesn't happen.