ASAN error in rclcpp_action test_server

[y-okumura-isp]

Bug report

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • build
• Version or commit hash:
  • 8d5af66
• DDS implementation:
  • FastDDS
• Client library (if applicable):
  • rclcpp_action

Steps to reproduce issue

Build with ASAN and run test_server.
In my environment, I got the following error (I deleted the irrelevant logs).

$ ./build-asan/rclcpp_action/test_server
Running main() from /src/ros2_rolling_asan/install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc
[==========] Running 37 tests from 3 test suites.
[----------] Global test environment set-up.
[----------] 20 tests from TestServer
[ RUN      ] TestServer.construction_and_destruction
[       OK ] TestServer.construction_and_destruction (112 ms)
[ RUN      ] TestServer.construction_and_destruction_after_node
=================================================================
==13282==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e0000011c0 at pc 0x7f494c8451e5 bp 0x7ffca1543260 sp 0x7ffca1543250
READ of size 4 at 0x60e0000011c0 thread T0
    #0 0x7f494c8451e4 in rcl_timer_fini /src/ros2_rolling_asan/src/ros2/rcl/rcl/src/rcl/timer.c:213
    #1 0x7f494d3c4554 in rcl_action_server_fini /src/ros2_rolling_asan/src/ros2/rcl/rcl_action/src/rcl_action/action_server.c:235
    #2 0x7f494d46886e in operator() /src/ros2_rolling_asan/src/ros2/rclcpp/rclcpp_action/src/server.cpp:92
    #8 0x7f494d478c87 in rclcpp_action::ServerBaseImpl::~ServerBaseImpl() /src/ros2_rolling_asan/src/ros2/rclcpp/rclcpp_action/src/server.cpp:37
    #11 0x7f494d469b52 in rclcpp_action::ServerBase::~ServerBase() /src/ros2_rolling_asan/src/ros2/rclcpp/rclcpp_action/src/server.cpp:128
    #20 0x55fa05f9ad90 in TestServer_construction_and_destruction_after_node_Test::TestBody() /src/ros2_rolling_asan/src/ros2/rclcpp/rclcpp_action/test/test_server.cpp:124
    #31 0x55fa0618b38b in main /src/ros2_rolling_asan/install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc:45
    #32 0x7f494c3aa0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #33 0x55fa05f99ccd in _start (/src/ros2_rolling_asan/build-asan/rclcpp_action/test_server+0xf3ccd)

0x60e0000011c0 is located 0 bytes inside of 160-byte region [0x60e0000011c0,0x60e000001260)
freed by thread T0 here:
    #0 0x7f494d5f7025 in operator delete(void*, unsigned long) (/lib/x86_64-linux-gnu/libasan.so.5+0x111025)
    #6 0x7f494cea182d in rclcpp::Clock::~Clock() /src/ros2_rolling_asan/src/ros2/rclcpp/rclcpp/src/rclcpp/clock.cpp:64
    #14 0x7f494d478c77 in rclcpp_action::ServerBaseImpl::~ServerBaseImpl() /src/ros2_rolling_asan/src/ros2/rclcpp/rclcpp_action/src/server.cpp:37
    #17 0x7f494d469b52 in rclcpp_action::ServerBase::~ServerBase() /src/ros2_rolling_asan/src/ros2/rclcpp/rclcpp_action/src/server.cpp:128
    #26 0x55fa05f9ad90 in TestServer_construction_and_destruction_after_node_Test::TestBody() /src/ros2_rolling_asan/src/ros2/rclcpp/rclcpp_action/test/test_server.cpp:124
    #29 0x55fa0619f2fd in testing::Test::Run() /src/ros2_rolling_asan/install-asan/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2508

Expected behavior

No leak

Actual behavior

See above.

Additional information

It looks

• ServerBaseImpl::clock_ is freed in rclcpp_action::ServerBaseImpl::~ServerBaseImpl().
• Then ServerBaseImpl::action_server_ is deleted.
• But ServerBaseImpl::action_server_ depends on ServerBaseImpl::clock_, heap-use-after-free occurs.

ServerBase::ServerBase(...) {
  rcl_node_t * rcl_node = node_base->get_rcl_node_handle();
  rcl_clock_t * rcl_clock = pimpl_->clock_->get_clock_handle();

  // action_server gets rcl_clock 
  rcl_ret_t ret = rcl_action_server_init(
    pimpl_->action_server_.get(), rcl_node, rcl_clock, type_support, name.c_str(), &options);
}

As variables are destroyed in the reverse order of construction, it's good to declare clock_ before action_server_.

// ros2/rclcpp/rclcpp_action/src/server.cpp
class ServerBaseImpl
  // std::shared_ptr<rcl_action_server_t> action_server_;

   rclcpp::Clock::SharedPtr clock_;
 
   std::shared_ptr<rcl_action_server_t> action_server_;

By this modification, ASAN errors disappear in my enviroinment.
I'll send PR if it looks OK.