Use valid clock in case of issue in rcl_timer_init #795
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Stephen Brawner <brawner@gmail.com>
clalancette
reviewed
Sep 9, 2020
8000
|
Cleaned up code to pass linters. |
|
Testing
|
clalancette
approved these changes
Sep 9, 2020
sloretz
reviewed
Sep 10, 2020
| @@ -170,10 +170,13 @@ rcl_action_server_init( | |||
| PUBLISHER_INIT(feedback); | |||
| PUBLISHER_INIT(status); | |||
|
|
|||
| // Initialize Timer | |||
| // Copy clock | |||
| action_server->impl->clock = *clock; | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@sloretz Thanks for bringing this to my attention and for the thorough writeup. I'll dig into it
There was a problem hiding this comment.
@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?
sloretz
pushed a commit
that referenced
this pull request
Sep 18, 2020
* Use valid clock in case of issue Signed-off-by: Stephen Brawner <brawner@gmail.com> * Cleanup Signed-off-by: Stephen Brawner <brawner@gmail.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
sloretz
added a commit
that referenced
this pull request
Sep 23, 2020
) Store reference to rcl_clock_t instead of copy (#797) (#805) * Use valid clock in case of issue in rcl_timer_init (#795) * Use valid clock in case of issue Signed-off-by: Stephen Brawner <brawner@gmail.com> * Cleanup Signed-off-by: Stephen Brawner <brawner@gmail.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Store reference to rcl_clock_t instead of copy (#797) rcl_clock_t can't be trivially copied because adding or removing clock jump callbacks to a copy will free the pointer held by the original instance. Signed-off-by: Shane Loretz<sloretz@openrobotics.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Co-authored-by: brawner <brawner@gmail.com>
skyegalaxy
pushed a commit
to irobot-ros/rcl
that referenced
this pull request
Mar 12, 2025
* basic ipc implementation from alsora/new_ipc_proposal Signed-off-by: alberto <alberto.soragna@gmail.com> better use of node_topic create subscription Signed-off-by: alberto <alberto.soragna@gmail.com> added intra process manager test Signed-off-by: alberto <alberto.soragna@gmail.com> fixed ring buffer and added test Signed-off-by: alberto <alberto.soragna@gmail.com> added intra process buffer test Signed-off-by: alberto <alberto.soragna@gmail.com> added intra process buffer test Signed-off-by: alberto <alberto.soragna@gmail.com> Signed-off-by: alberto <alberto.soragna@gmail.com> removed intra-process methods from subscription base Signed-off-by: alberto <alberto.soragna@gmail.com> using lock_guard instead of unique_lock, renamed var without camel case Signed-off-by: alberto <alberto.soragna@gmail.com> using unordered set and references in intra process manager Signed-off-by: alberto <alberto.soragna@gmail.com> subscription intra-process does not depend anymore on subscription, but has a copy of the callback Signed-off-by: alberto <alberto.soragna@gmail.com> changed buffer API to use rvo Signed-off-by: Alberto <alberto.soragna@gmail.com> avoid copying shared_ptr Signed-off-by: alberto <alberto.soragna@gmail.com> revert not needed changes to create_subscription Signed-off-by: alberto <alberto.soragna@gmail.com> updated tests according to new buffer APIs Signed-off-by: alberto <alberto.soragna@gmail.com> updated types in ring buffer implementation avoid using uint32_t Signed-off-by: alberto <alberto.soragna@gmail.com> using unique ptr for buffers in subscription_intra_process Signed-off-by: alberto <alberto.soragna@gmail.com> added missing std::move in subscription_intra_process constructor Signed-off-by: alberto <alberto.soragna@gmail.com> use consisting names for ring_buffer_implementation members Signed-off-by: alberto <alberto.soragna@gmail.com> addressing typos, one-liners and similar from ivanpauno review Signed-off-by: alberto <alberto.soragna@gmail.com> moved subscription_intra_process_base to its own files and moved non templated method from derived class Signed-off-by: alberto <alberto.soragna@gmail.com> removed forward declarations, fixed include subscription_intra_process_base Signed-off-by: alberto <alberto.soragna@gmail.com> removed member variable from do_intra_process_publish signature Signed-off-by: alberto <alberto.soragna@gmail.com> declare public before private in intra_process_manager_impl Signed-off-by: alberto <alberto.soragna@gmail.com> made matches_any_intra_process_publishers const Signed-off-by: alberto <alberto.soragna@gmail.com> using const reference in get_all_matching_publishers Signed-off-by: alberto <alberto.soragna@gmail.com> added deleter and alloc templates in intra_process_buffer Signed-off-by: alberto <alberto.soragna@gmail.com> added RCLCPP_WARN to intra_process_manager_impl Signed-off-by: alberto <alberto.soragna@gmail.com> passing context from node to subscription_intra_process Signed-off-by: alberto <alberto.soragna@gmail.com> using allocators in intra_process_manager Signed-off-by: alberto <alberto.soragna@gmail.com> use size_t instead of int in ring buffer indices Signed-off-by: alberto <alberto.soragna@gmail.com> creating buffer inside subscription_intra_process constructor Signed-off-by: alberto <alberto.soragna@gmail.com> fix lint errors Signed-off-by: alberto <alberto.soragna@gmail.com> throw error if trying to dequeue when buffer empty; remove duplicated methods in intra_process_buffer Signed-off-by: alberto <alberto.soragna@gmail.com> added todo for creating an rmw function for checking qos compatibility Signed-off-by: alberto <alberto.soragna@gmail.com> test fixes Signed-off-by: alberto <alberto.soragna@gmail.com> refactored intra_process_manager, removed ipm impl Signed-off-by: alberto <alberto.soragna@gmail.com> added mutex in intra_process_manager add_* methods Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> added allocator to intra_process_buffer Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> added invalid intra_process qos test for subscription Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> throw error if history size is 0 with keep last and ipc Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> using allocator when creating unique_ptr from shared_ptr Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> adding deleter template argument to intra_process buffer Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> fix linter Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> throw error with callbackT different from messageT Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> updated deleter template argument in subscription factory Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> Fix typo in test fixture tear down method name (ros2#787) Signed-off-by: Jacob Perron <jacob@openrobotics.org> Add free function for creating service clients (ros2#788) Equivalent to the free function for creating a service. Resolves ros2#768 Signed-off-by: Jacob Perron <jacob@openrobotics.org> Cmake infrastructure for creating components (ros2#784) *cmake macro to create components for libraries with multiple nodes Signed-off-by: Siddharth Kucheria <kucheria@usc.edu> Allow registering multiple on_parameters_set_callback (ros2#772) Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com> fix for multiple nodes not being recognized (ros2#790) Signed-off-by: Siddharth Kucheria <kucheria@usc.edu> Remove non-package from ament_target_dependencies() (ros2#793) Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> fix linter issue (ros2#795) Signed-off-by: Siddharth Kucheria <kucheria@usc.edu> Make TimeSource ignore use_sim_time events coming from other nodes. (ros2#799) Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> passing deleter template parameter Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> small fixes for failing tests Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> fixed imports in test_intra_process_manager Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> using RCLCPP_SMART_PTR_ALIASES_ONLY and RCLCPP_PUBLIC macros Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> added RCLCPP_PUBLIC macros and virtual destructor to sub intra_process base Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> added unique_ptr alias to macros Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> updated test_intra_process_manager.cpp Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> remove mock msgs from rclcpp (ros2#800) Signed-off-by: Karsten Knese <karsten@openrobotics.org> Add line break after first open paren in multiline function call (ros2#785) * Add line break after first open paren in multiline function call as per developer guide: https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#open-versus-cuddled-braces see ament/ament_lint#148 Signed-off-by: Dan Rose <dan@digilabs.io> Fix dedent when first function argument starts with a brace Signed-off-by: Dan Rose <dan@digilabs.io> Line break with multiline if condition Remove line breaks where allowed. Signed-off-by: Dan Rose <dan@digilabs.io> Fixup after rebase Signed-off-by: Dan Rose <dan@digilabs.io> Fixup again after reverting indent_paren_open_brace Signed-off-by: Dan Rose <dan@digilabs.io> * Revert comment spacing change, condense some lines Signed-off-by: Dan Rose <dan@digilabs.io> Adapt to '--ros-args ... [--]'-based ROS args extraction (ros2#816) * Use --ros-args to deal with node arguments in rclcpp. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Document implicit --ros-args flag in NodeOptions::arguments(). Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Add missing size_t to int cast. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Only add implicit --ros-args flag if not present already. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Add some rclcpp::NodeOptions test coverage. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Address peer review comments. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Please cpplint and uncrustify. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> Guard against making multiple result requests for a goal handle (ros2#808) This fixes a runtime error caused by a race condition when making consecutive requests for the result. Specifically, this happens if the user provides a result callback when sending a goal and then calls async_get_result shortly after. Resolves ros2#783 Signed-off-by: Jacob Perron <jacob@openrobotics.org> Explain return value of spin_until_future_complete (ros2#792) Signed-off-by: Dan Rose <dan@digilabs.io> Allow passing logger by const ref (ros2#820) Signed-off-by: Karsten Knese <karsten@openrobotics.org> Delete unnecessary call for get_node_by_group (ros2#823) Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com> Fix get_node_interfaces functions taking a pointer (ros2#821) Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com> add callback group as member variable and constructor arg (ros2#811) Signed-off-by: bpwilcox <bpwilcox@eng.ucsd.edu> remove callback group as member variable Wrap documentation examples in code blocks (ros2#830) This makes the code examples easier to read in the generated documentation. Signed-off-by: Jacob Perron <jacob@openrobotics.org> Crash in callback group pointer vector iterator (ros2#814) Signed-off-by: Guillaume Autran <gautran@clearpath.ai> add mutex in add/remove_node and wait_for_work to protect concurrent use/change of memory_strategy_ (ros2#837) Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com> Fix hang with timers in MultiThreadedExecutor (ros2#835) (ros2#836) Signed-off-by: Todd Malsbary <todd.malsbary@intel.com> Use of -r/--remap flags where appropriate. (ros2#834) Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> Force explicit --ros-args in NodeOptions::arguments(). (ros2#845) Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> Fail on invalid and unknown ROS specific arguments (ros2#842) * Fail on invalid and unknown ROS specific arguments. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Revert changes to utilities.hpp in rclcpp Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Fully revert change to utilities.hpp Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> Fix typo in deprecated warning. (r 7D53 os2#848) "it's" instead of its Signed-off-by: Luca Della Vedova <luca@openrobotics.org> Add throwing parameter name if parameter is not set (ros2#833) * added throwing parameter name if parameter is not set Signed-off-by: Alex <cvbn127@gmail.com> Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com> check valid timer handler 1st to reduce the time window for scan. (ros2#841) Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com> remove features and related code which were deprecated in dashing (ros2#852) Signed-off-by: William Woodall <william@osrfoundation.org> reset error message before setting a new one, embed the original one (ros2#854) Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com> restored virtual destructor in publisher_base Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com> * fixup a few things after rebase Signed-off-by: William Woodall <william@osrfoundation.org> * refactor some API's and get code compiling again Signed-off-by: William Woodall <william@osrfoundation.org> * docs and style changes (whitespace) Signed-off-by: William Woodall <william@osrfoundation.org> * move new intra process internals into experimental namespace Signed-off-by: William Woodall <william@osrfoundation.org> * uncrustify Signed-off-by: William Woodall <william@osrfoundation.org> * fix issues with LoanedMessages after rebase Signed-off-by: William Woodall <william@osrfoundation.org> * more fixups Signed-off-by: William Woodall <william@osrfoundation.org> * readd logic for avoiding in compatible QoS Signed-off-by: William Woodall <william@osrfoundation.org> * avoid an error when intra process is disabled Signed-off-by: William Woodall <william@osrfoundation.org> * change intra process to preserve pointer in cyclic_pipeline Signed-off-by: William Woodall <william@osrfoundation.org> * fix issue matching topics in intra process Signed-off-by: William Woodall <william@osrfoundation.org> * fix some issues with the tests after latest behavior change Signed-off-by: William Woodall <william@osrfoundation.org> * address review feedback Signed-off-by: William Woodall <william@osrfoundation.org> * fix the initialization order Signed-off-by: William Woodall <william@osrfoundation.org> * avoid possible loss of data warning Signed-off-by: William Woodall <william@osrfoundation.org> * more fixes related to initialization Signed-off-by: William Woodall <william@osrfoundation.org> * fix use of custom allocators Signed-off-by: William Woodall <william@osrfoundation.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This clock needs to be copied before rcl_timer_init in the case that
rcl_timer_initfails. Previously, if it failed before it was copied,rcl_timer_finiwould be called inrcl_action_server_finiand that would try to access a pointer owned and already free'd byrclcpp::Node.Signed-off-by: Stephen Brawner brawner@gmail.com