Address issue #716 by zero initializing pointers and freeing memory

[brawner]

During initialization of rcl_action_client, pointers were not explicitly zero initialized which was only an issue if rcl_action_client_init failed at some point and then tried to cleanup in its fail block. This was only pointed out in an error on Windows debug, as an unitialized pointer was checked for null. This exact sort of initialization failure was added in a unit test in #663. Furthermore, because the action_client was invalid in the fail block, cleanup would subsequently fail.

Example of windows Debug failure
https://ci.ros2.org/view/nightly/job/nightly_win_deb/1682/testReport/(root)/projectroot/test_action_client_2/

This PR zero initialized rcl_action_client_impl_t once it's allocated and adds a _rcl_action_client_fini_impl and uses that for cleanup in rcl_action_client_init instead.

Fixes #716

I checked these changes with test_action_client and valgrind, which resulted only in the unrelated error below.

==1282598== HEAP SUMMARY:
==1282598==     in use at exit: 38,207 bytes in 115 blocks
==1282598==   total heap usage: 75,175 allocs, 75,060 frees, 32,579,914 bytes allocated
==1282598== 
==1282598== 560 (120 direct, 440 indirect) bytes in 5 blocks are definitely lost in loss record 20 of 25
==1282598==    at 0x48417F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1282598==    by 0x518011A: __default_allocate (allocator.c:35)
==1282598==    by 0x55978AB: rmw_allocate (allocators.c:29)
==1282598==    by 0x5597CA1: rmw_wait_set_allocate (allocators.c:133)
==1282598==    by 0x60BE148: rmw_fastrtps_shared_cpp::__rmw_create_wait_set(char const*, rmw_context_t*, unsigned long) (rmw_wait_set.cpp:38)
==1282598==    by 0x6050E01: node_listener(rmw_context_t*) (listener_thread.cpp:131)
==1282598==    by 0x6053110: void std::__invoke_impl<void, void (*)(rmw_context_t*), rmw_context_t*>(std::__invoke_other, void (*&&)(rmw_context_t*), rmw_context_t*&&) (invoke.h:60)
==1282598==    by 0x6052F86: std::__invoke_result<void (*)(rmw_context_t*), rmw_context_t*>::type std::__invoke<void (*)(rmw_context_t*), rmw_context_t*>(void (*&&)(rmw_context_t*), rmw_context_t*&&) (invoke.h:95)
==1282598==    by 0x6052E0E: void std::thread::_Invoker<std::tuple<void (*)(rmw_context_t*), rmw_context_t*> >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) (thread:244)
==1282598==    by 0x6052D24: std::thread::_Invoker<std::tuple<void (*)(rmw_context_t*), rmw_context_t*> >::operator()() (thread:251)
==1282598==    by 0x6052CD1: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(rmw_context_t*), rmw_context_t*> > >::_M_run() (thread:195)
==1282598==    by 0x527BCB3: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==1282598== 
==1282598== LEAK SUMMARY:
==1282598==    definitely lost: 120 bytes in 5 blocks
==1282598==    indirectly lost: 440 bytes in 5 blocks
==1282598==      possibly lost: 0 bytes in 0 blocks
==1282598==    still reachable: 37,647 bytes in 105 blocks
==1282598==         suppressed: 0 bytes in 0 blocks
==1282598== Reachable blocks (those to which a pointer was found) are not shown.
==1282598== To see them, rerun with: --leak-check=full --show-leak-kinds=all

Signed-off-by: Stephen Brawner brawner@gmail.com

[brawner]

Debug build --up-to rcl_action

• Linux
• Linux-aarch64
• macOS
• Windows

Standard build --up-to rcl_action

• Linux
• Linux-aarch64
• macOS
• Windows

[brawner]

Addresses #716

[fujitatomoya]

This might be overwritten by https://github.com/ros2/rcl/pull/717/files?file-filters%5B%5D=.c#diff-1f30210b7875b9b39020ed2b9c12bd78L173

[brawner]

Are there any specific circumstances you are worried about? Setting impl to get the zero_initialized values should put the object into a known state, which helps ensure cleanup occurs predictably. Otherwise if there is a double error, then it can be helpful for the message to be set twice, so that the log file has a record of the sequence of events that failed.

[fujitatomoya]

Actually no, but there's been discussion about this, #713 (comment). I do not think that this is the problem but status. I'd think it might be better to share.

[brawner]

I got the go-ahead privately from @jacobperron to merge with Dirk's approval