Skip to content

Use valid clock in case of issue in rcl_timer_init #795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
brawner merged 2 commits into from
Sep 9, 2020
Merged

Use valid clock in case of issue in rcl_timer_init #795

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a107029
Use valid clock in case of issue
brawner Sep 9, 2020
4ff7f9c
Cleanup
brawner Sep 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions rcl_action/src/rcl_action/action_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,13 @@ rcl_action_server_init(
PUBLISHER_INIT(feedback);
PUBLISHER_INIT(status);

// Initialize Timer
// Copy clock
action_server->impl->clock = *clock;
Copy link
Copy Markdown
Contributor

@sloretz sloretz Sep 10, 2020

Choose a reason for hiding this comment

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

@clalancette @brawner This appears to be the cause of the tf2_ros test_buffer_server test failing. The problem is rcl_clock_t can't be trivially copied because it stores time jump callbacks. Reverting would make the test failure go away, but I think a better solution would be to make the action server store a pointer to the clock, not the clock itself.

First the tf2_ros::Buffer creates a time jump callback. This causes clock->jump_callbacks to be allocated. Then the action server copies the clock (this line here). This means there are two rcl_clock_t instances pointing holding the same pointer in clock->jump_callbacks (let's call them OriginalClock and ASClock). Then the call to rcl_timer_init() below adds a clock jump callback to ASClock. Since this is a realloc, the old pointer is free'd and a new pointer is returned. ASClock->jump_callbacks points to newly allocated memory while OriginalClock->jump_callbacks still points to the memory that has now been freed. The test creates a tf2_ros::BufferServer instance with OriginalClock, which creates a timer that again adds a jump callback. It tries realloc using OriginalClock->jump_callbacks but that is a use-after-free error.

Copy link
Copy Markdown
Contributor Author

@brawner brawner Sep 10, 2020

Choose a reason for hiding this comment

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

@sloretz Thanks for bringing this to my attention and for the thorough writeup. I'll dig into it

Copy link
Copy Markdown
Contributor

@sloretz sloretz Sep 10, 2020

Choose a reason for hiding this comment

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

@brawner happy to. I have a branch locally that makes the action server use a pointer to the clock. Assuming tests pass locally, mind if I ping you later for a review?


// Initialize Timer
ret = rcl_timer_init(
&action_server->impl->expire_timer, clock, node->context, options->result_timeout.nanoseconds,
NULL, allocator);
&action_server->impl->expire_timer, &action_server->impl->clock, node->context,
options->result_timeout.nanoseconds, NULL, allocator);
if (RCL_RET_OK != ret) {
goto fail;
}
Expand All @@ -183,9 +186,6 @@ rcl_action_server_init(
goto fail;
}

// Copy clock
action_server->impl->clock = *clock;

// Copy action name
action_server->impl->action_name = rcutils_strdup(action_name, allocator);
if (NULL == action_server->impl->action_name) {
Expand Down