Skip to content

Address issue #716 by zero initializing pointers and freeing memory#717

Merged
brawner merged 1 commit intomasterfrom
brawner/rcl_action-fix-716
Jul 16, 2020
Merged

Address issue #716 by zero initializing pointers and freeing memory#717
brawner merged 1 commit intomasterfrom
brawner/rcl_action-fix-716

Conversation

@brawner
Copy link
Copy Markdown
Contributor

@brawner brawner commented Jul 15, 2020 •
edited
Loading

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
Address issue 716 by zero initializing pointers and freeing memory
fbb7441 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Copy link
Copy Markdown
Contributor Author

brawner commented Jul 15, 2020 •
edited
Loading

Debug build --up-to rcl_action

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Standard build --up-to rcl_action

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@brawner
Copy link
Copy Markdown
Contributor Author

brawner commented Jul 15, 2020

Addresses #716

@brawner brawner mentioned this pull request Jul 15, 2020
Closed
@brawner brawner requested a review from jacobperron July 15, 2020 23:17
fujitatomoya
fujitatomoya reviewed Jul 16, 2020
View reviewed changes
rcl_action/src/rcl_action/action_client.c
// Copy action client name and options.
action_client->impl->action_name = rcutils_strdup(action_name, allocator);
if (NULL == action_client->impl->action_name) {
RCL_SET_ERROR_MSG("failed to duplicate action name");
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@brawner brawner Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor Author

brawner commented Jul 16, 2020

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

@brawner brawner merged commit 1a7f737 into master Jul 16, 2020
@delete-merged-branch delete-merged-branch bot deleted the brawner/rcl_action-fix-716 branch July 16, 2020 23:46
brawner added a commit that referenced this pull request Oct 5, 2020
@brawner
Address issue 716 by zero initializing pointers and freeing memory (#717
397ddeb 
)

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner mentioned this pull request Oct 5, 2020
Merged
brawner added a commit that referenced this pull request Oct 5, 2020
@brawner
Address issue 716 by zero initializing pointers and freeing memory (#717
c4e690c 
) (#820)

Signed-off-by: Stephen Brawner <brawner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fujitatomoya fujitatomoya fujitatomoya left review comments

@jacobperron jacobperron Awaiting requested review from jacobperron

+1 more reviewer

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test_action_client failing on Windows debug

4 participants

@brawner @dirk-thomas @fujitatomoya @jacobperron