-
Notifications
You must be signed in to change notification settings - Fork 162
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.
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
SIGABRT parsing yaml config file (double free detected) #553
Comments
If a parameter file is malformed, it might actually be safer to fail instead of letting rcl_init continue. Passing incorrect values to a node (e.g. defaults) could be bad for an application, resulting in unexpected behavior. We should still fix the crash and instead return an appropriate error code. |
Yes, I agree with SIGABRT preferably with a stderr line preceding it would be the safe bet, or std::exception with what() filled in. |
I tried to reproduce the issue, and the double free only occurs when using the deprecated syntax e.g.
If we use the recommended syntax then I don't see the double free:
And we get a more informative error message:
I also noticed that the double free does not occur in Dashing. The bug must have been introduced when the |
The function assumes that the structure has already been initialized, so it should be the callers responsibility to finalize it. Otherwise, this may lead to a double free as reported in #553. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
See #556 for a fix. |
Thanks @jacobperron for the fix! Regarding the deprecated syntax, this bug surfaced using python launch file description...perhaps that is using the old syntax and should be updated to use the new? |
@guru-florida Are you able to share a launch file that I can use to reproduce the issue? |
Nevermind, I guess this is the fix for the deprecation warning you were seeing: ros2/launch_ros#106 It will be backported to Eloquent and should be fixed in the next patch release: ros2/launch_ros#115 |
* Don't finalize parameters struct in rcl_parse_yaml_file function The function assumes that the structure has already been initialized, so it should be the callers responsibility to finalize it. Otherwise, this may lead to a double free as reported in #553. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Return an error code if there's a malformed parameters file This restores behavior that was lost when the '__params:=' syntax was deprecated in #495. An extra guard was also added when finalizing the parameters struct. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Refactor comment Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add regression test Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Provide detailed error message Also warn about deprecated usage, even if there is an error parsing parameters file. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This was fixed by #556. |
Bug report
rclcpp::init(...) causing SIGABRT while parsing config yaml that is well formed yaml but not valid ROS2 parameters format. For example, if yaml has only a single underscore in ros__parameters.
Steps to reproduce issue
remove an underscore from ros__parameters. Or have a yaml without node-name, such as just:
joints: 17
Expected behavior
Should produce an error saying the yaml file is malformed, or at least not crash but node would not receive parameters.
Actual behavior
Program produces the following output:
Additional information
Using
--cmake-args -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-fsanitize=address"
produces this output:The text was updated successfully, but these errors were encountered: