Do not clear entities callbacks on destruction #2002

mauropasse · 2022-08-24T16:16:44Z

Removing these not-necessary clearings of listener callbacks, since the objects are being destroyed anyway.

This also fixes a bug (use-after-free) happening during services destruction:

Lines 35 to 38 in 3d69031

    
           ServiceBase::~ServiceBase() 
        
           { 
        
             clear_on_new_request_callback(); 
        
           }

On detail:

ServiceBase::~ServiceBase()
{
  // Here, implicitly, the deleter of service_handle_ was being called (*),
  // freeing the service listener (**):
  // ~service_handle_
  
  // Then, the following was called, which tries to access the freed mem (thus seg-fault) (***)
  clear_on_new_request_callback();
}

(*)

rclcpp/rclcpp/include/rclcpp/service.hpp

Lines 316 to 327 in 3d69031

    
           service_handle_ = std::shared_ptr<rcl_service_t>( 
        
             new rcl_service_t, [handle = node_handle_, service_name](rcl_service_t * service) 
        
             { 
        
               if (rcl_service_fini(service, handle.get()) != RCL_RET_OK) { 
        
                 RCLCPP_ERROR( 
        
                   rclcpp::get_node_logger(handle.get()).get_child("rclcpp"), 
        
                   "Error in destruction of rcl service handle: %s", 
        
                   rcl_get_error_string().str); 
        
                 rcl_reset_error(); 
        
               } 
        
               delete service; 
        
             });

(**) https://github.com/ros2/rmw_fastrtps/blob/dbee45ec67104db51b96da9693466f7a14e2f000/rmw_fastrtps_shared_cpp/src/rmw_service.cpp#L132

(***) https://github.com/ros2/rmw_fastrtps/blob/dbee45ec67104db51b96da9693466f7a14e2f000/rmw_fastrtps_shared_cpp/src/rmw_service.cpp#L170

FYI @alsora

Signed-off-by: Mauro Passerino mpasserino@irobot.com

Removing these clearings since they were not necessary, since the objects are being destroyed anyway. Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

fujitatomoya · 2022-08-25T17:39:42Z

rclcpp/src/rclcpp/service.cpp

@@ -32,10 +32,6 @@ ServiceBase::ServiceBase(std::shared_ptr<rcl_node_t> node_handle)
  node_logger_(rclcpp::get_node_logger(node_handle_.get()))
 {}

-ServiceBase::~ServiceBase()
-{
-  clear_on_new_request_callback();


std::shared_ptr<rcl_service_t> service_handle_ will be deleted after clear_on_new_request_callback(), right? All member variables are alive in the body of the destructor. So this is something like optimization?

Yes, usually that's the order. But I found a strange case with lifecycle nodes destruction, where it happens in the opposite order!
Anyway, even if the order were correct, IMO there is no real point in clearing those callbacks.

i think that this should not be called here, because it might generate the exception in dtor.

see,

https://github.com/ros2/rmw/blob/rolling/rmw/include/rmw/rmw.h#L3118-L3131

if the callback is NULL, as specification, it is supposed to return RMW_RET_INVALID_ARGUMENT.
and eventually rclcpp will generate rclcpp::exceptions::throw_from_rcl_error.

test is pass cz rmw_fastrtps always return RCL_RET_OK. (this is not correct behavior based on the doc.)

if we use RTI Coneext DDS, the test fails as following, because it returns RMW_RET_UNSUPPORTED.

[ RUN ] TestService.on_new_request_callback RTI Connext DDS Non-commercial license is for academic, research, evaluation and personal use only. USE FOR COMMERCIAL PURPOSES IS PROHIBITED. See RTI_LICENSE.TXT for terms. Download free tools at rti.com/ncl. License issued to Non-Commercial User license@rti.com For non-production use only. Expires on 00-jan-00 See www.rti.com for more information. [ERROR] [1661535719.598830456] [rmw_connextdds]: rmw_service_set_on_new_request_callback not implemented unknown file: Failure C++ exception with description "failed to set the on new request callback for service: rmw_service_set_on_new_request_callback not implemented, at /root/ros2_ws/colcon_ws/src/ros2/rmw_connextdds/rmw_connextdds_common/src/common/rmw_listener.cpp:45" thrown in the test body. [ FAILED ] TestService.on_new_request_callback (315 ms)

after all, i think after this PR is merged, we need to re-check the listener rmw implementation and extend test for all rmw tier 1 implementation. what do you think?

Good, you found a better reason for removing these calls! And yes I agree, tests should be extended for these calls to return the other possible values too. When I found some time I'll add them. You have any other ideas for testing?

I think we can manage the following with different PR,

test all tier 1 rmw implementations with listener APIs for rclcpp (rclpy does not support?)

bugfix rmw implementations if necessary, i think rmw_connextdds is the correct implementation.

test would be required to adjust accordingly.

fujitatomoya · 2022-08-26T17:54:11Z

rclcpp/src/rclcpp/service.cpp

@@ -32,10 +32,6 @@ ServiceBase::ServiceBase(std::shared_ptr<rcl_node_t> node_handle)
  node_logger_(rclcpp::get_node_logger(node_handle_.get()))
 {}

-ServiceBase::~ServiceBase()
-{
-  clear_on_new_request_callback();


i think that this should not be called here, because it might generate the exception in dtor.

see,

https://github.com/ros2/rmw/blob/rolling/rmw/include/rmw/rmw.h#L3118-L3131

if the callback is NULL, as specification, it is supposed to return RMW_RET_INVALID_ARGUMENT.
and eventually rclcpp will generate rclcpp::exceptions::throw_from_rcl_error.

test is pass cz rmw_fastrtps always return RCL_RET_OK. (this is not correct behavior based on the doc.)

if we use RTI Coneext DDS, the test fails as following, because it returns RMW_RET_UNSUPPORTED.

[ RUN ] TestService.on_new_request_callback RTI Connext DDS Non-commercial license is for academic, research, evaluation and personal use only. USE FOR COMMERCIAL PURPOSES IS PROHIBITED. See RTI_LICENSE.TXT for terms. Download free tools at rti.com/ncl. License issued to Non-Commercial User license@rti.com For non-production use only. Expires on 00-jan-00 See www.rti.com for more information. [ERROR] [1661535719.598830456] [rmw_connextdds]: rmw_service_set_on_new_request_callback not implemented unknown file: Failure C++ exception with description "failed to set the on new request callback for service: rmw_service_set_on_new_request_callback not implemented, at /root/ros2_ws/colcon_ws/src/ros2/rmw_connextdds/rmw_connextdds_common/src/common/rmw_listener.cpp:45" thrown in the test body. [ FAILED ] TestService.on_new_request_callback (315 ms)

after all, i think after this PR is merged, we need to re-check the listener rmw implementation and extend test for all rmw tier 1 implementation. what do you think?

alsora · 2022-08-30T11:10:17Z

CI

Linux
Linux-aarch64
Windows

fujitatomoya · 2022-09-18T22:33:38Z

@mauropasse can you check the CI failure? i think those two are unrelated, but need to double-check.

Needed since QOSEventHandlerBase does not own the pub/sub listeners. So the QOSEventHandler can be destroyed while the corresponding listeners are still alive, so we need to clear these callbacks. Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

mauropasse · 2022-09-20T12:10:08Z

@fujitatomoya I reverted the changes for clearing callbacks on ~QOSEventHandlerBase.
This object is created when creating a Publisher/Subscriber, so it's destructor will be called before the rmw publisher/subscribers/etc are destroyed.
So the events callbacks need to be cleared on QOSEventHandlerBase destructor. No risk of rmw entities destroyed before, since they are created prior of this object.

alsora · 2022-09-20T12:12:17Z

Linux
Linux-aarch64
Windows

mauropasse · 2022-09-20T14:57:09Z

I'll add a comment about why this clearing is needed here, and not in the other entities, but I'll wait for all green CI.

Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

fujitatomoya · 2022-09-22T21:53:09Z

@alsora can you review this? i am good to go with it.

alsora · 2022-09-22T23:30:49Z

Looks good to me.
We have been testing this PR for the last couple of days and it seems to fix the problem.
I'm going to go ahead and merge it.

fujitatomoya · 2022-09-22T23:46:01Z

@alsora thanks!

* Do not clear entities callbacks on destruction Removing these clearings since they were not necessary, since the objects are being destroyed anyway. Signed-off-by: Mauro Passerino <mpasserino@irobot.com> * Fix CI Signed-off-by: Mauro Passerino <mpasserino@irobot.com> * Restore clear_on_ready_callback on ~QOSEventHandlerBase Needed since QOSEventHandlerBase does not own the pub/sub listeners. So the QOSEventHandler can be destroyed while the corresponding listeners are still alive, so we need to clear these callbacks. Signed-off-by: Mauro Passerino <mpasserino@irobot.com> * Add coment on clearing callback for QoS event Signed-off-by: Mauro Passerino <mpasserino@irobot.com> Signed-off-by: Mauro Passerino <mpasserino@irobot.com> Co-authored-by: Mauro Passerino <mpasserino@irobot.com>

Mauro Passerino added 2 commits August 24, 2022 15:08

Do not clear entities callbacks on destruction

ee87778

Removing these clearings since they were not necessary, since the objects are being destroyed anyway. Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

Fix CI

86e3b86

Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

fujitatomoya reviewed Aug 25, 2022

View reviewed changes

fujitatomoya approved these changes Aug 26, 2022

View reviewed changes

Add coment on clearing callback for QoS event

40597d7

Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

This was referenced Sep 22, 2022

callback can be NULL to clear in Listener APIs. ros2/rmw#332

Merged

add NULL check before accessing object. ros2/rmw_cyclonedds#423

Merged

alsora merged commit 145933b into ros2:rolling Sep 22, 2022

mauropasse deleted the mauro/not-clear-callbacks-on-destruction branch September 23, 2022 09:35

mauropasse mentioned this pull request Oct 10, 2022

Fix data race, declare rclcpp callbacks before the rcl entities #2024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not clear entities callbacks on destruction #2002

Do not clear entities callbacks on destruction #2002

mauropasse commented Aug 24, 2022

fujitatomoya Aug 25, 2022

mauropasse Aug 26, 2022

fujitatomoya Aug 26, 2022

mauropasse Aug 30, 2022

fujitatomoya Aug 30, 2022

fujitatomoya Aug 26, 2022

alsora commented Aug 30, 2022

fujitatomoya commented Sep 18, 2022

mauropasse commented Sep 20, 2022

alsora commented Sep 20, 2022

mauropasse commented Sep 20, 2022

fujitatomoya commented Sep 22, 2022

alsora commented Sep 22, 2022

fujitatomoya commented Sep 22, 2022

	ServiceBase::~ServiceBase()
	{
	clear_on_new_request_callback();
	}

	service_handle_ = std::shared_ptr<rcl_service_t>(
	new rcl_service_t, [handle = node_handle_, service_name](rcl_service_t * service)
	{
	if (rcl_service_fini(service, handle.get()) != RCL_RET_OK) {
	RCLCPP_ERROR(
	rclcpp::get_node_logger(handle.get()).get_child("rclcpp"),
	"Error in destruction of rcl service handle: %s",
	rcl_get_error_string().str);
	rcl_reset_error();
	}
	delete service;
	});

Do not clear entities callbacks on destruction #2002

Do not clear entities callbacks on destruction #2002

Conversation

mauropasse commented Aug 24, 2022

fujitatomoya Aug 25, 2022

Choose a reason for hiding this comment

mauropasse Aug 26, 2022

Choose a reason for hiding this comment

fujitatomoya Aug 26, 2022

Choose a reason for hiding this comment

mauropasse Aug 30, 2022

Choose a reason for hiding this comment

fujitatomoya Aug 30, 2022

Choose a reason for hiding this comment

fujitatomoya Aug 26, 2022

Choose a reason for hiding this comment

alsora commented Aug 30, 2022

fujitatomoya commented Sep 18, 2022

mauropasse commented Sep 20, 2022

alsora commented Sep 20, 2022

mauropasse commented Sep 20, 2022

fujitatomoya commented Sep 22, 2022

alsora commented Sep 22, 2022

fujitatomoya commented Sep 22, 2022