Skip to content
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

Jump to bottom

SIGABRT parsing yaml config file (double free detected) #553

Closed
guru-florida opened this issue Dec 22, 2019 · 8 comments
Closed

SIGABRT parsing yaml config file (double free detected) #553

guru-florida opened this issue Dec 22, 2019 · 8 comments
Assignees
@jacobperron
Labels
bug Something isn't working

Comments

@guru-florida
Copy link

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.

  • Operating System: Ubuntu 19.04 on x64 and also reproduced on RPI4 using Ubuntu 19
  • Installation type: source
  • Version or commit hash: eloquent branch
  • DDS implementation: default
  • Client library (if applicable): rclcpp

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:

free(): double free detected in tcache 2
Aborted (core dumped)

Additional information

Using --cmake-args -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-fsanitize=address" produces this output:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==24262==ERROR: AddressSanitizer: SEGV on unknown address 0x00002dfffff1 (pc 0x7f3437322962 bp 0x00002dfffff1 sp 0x7ffecf4d24c0 T0)
==24262==The signal is caused by a WRITE memory access.
    #0 0x7f3437322961  (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x28961)
    #1 0x7f3437406014 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10c014)
    #2 0x7f3436d0a6de in __default_deallocate (/opt/ros2/eloquent/install/rcutils/lib/librcutils.so+0x46de)
    #3 0x7f34367308e8 in rcl_yaml_node_struct_fini (/opt/ros2/eloquent/install/rcl_yaml_param_parser/lib/librcl_yaml_param_parser.so+0x28e8)
    #4 0x7f343676ab8e in rcl_arguments_fini (/opt/ros2/eloquent/install/rcl/lib/librcl.so+0xfb8e)
    #5 0x7f3436769ab0 in rcl_parse_arguments (/opt/ros2/eloquent/install/rcl/lib/librcl.so+0xeab0)
    #6 0x7f3436772f19 in rcl_init (/opt/ros2/eloquent/install/rcl/lib/librcl.so+0x17f19)
    #7 0x7f34370f5d69 in rclcpp::Context::init(int, char const* const*, rclcpp::InitOptions const&) (/opt/ros2/eloquent/install/rclcpp/lib/librclcpp.so+0x3d9d69)
    #8 0x7f3437212fa7 in rclcpp::init(int, char const* const*, rclcpp::InitOptions const&) (/opt/ros2/eloquent/install/rclcpp/lib/librclcpp.so+0x4f6fa7)
    #9 0x55c93cebce1a in main /home/guru/src/humanoid/ros2/humanoid/src/lss_joint_publisher/src/lss_joint_states.cpp:96
    #10 0x7f3436943b6a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26b6a)
    #11 0x55c93ceba619 in _start (/home/guru/src/humanoid/ros2/humanoid/build/lss_joint_publisher/lss_joint_states+0xf619)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x28961)
@jacobperron jacobperron added the bug Something isn't working label Jan 3, 2020
@jacobperron
Copy link
Member

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.

@guru-florida
Copy link
Author

Yes, I agree with SIGABRT preferably with a stderr line preceding it would be the safe bet, or std::exception with what() filled in.

@jacobperron
Copy link
Member

I tried to reproduce the issue, and the double free only occurs when using the deprecated syntax e.g.

    ros2 run demo_nodes_cpp talker __params:=bogus_config.yaml

If we use the recommended syntax then I don't see the double free:

    ros2 run demo_nodes_cpp talker --ros-args --params-file bogus_config.yaml

And we get a more informative error message:

'Couldn't parse params file: '--params-file bogus_config.yaml'. Error: Cannot have a value before ros__parameters at line 7 ...

I also noticed that the double free does not occur in Dashing. The bug must have been introduced when the __params syntax was deprecated in Eloquent. Since the deprecated syntax has been removed since Eloquent, this bug is specific to Eloquent.

jacobperron added a commit that referenced this issue Jan 17, 2020
@jacobperron
Don't finalize parameters struct in rcl_parse_yaml_file function
686d426 
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>
@jacobperron jacobperron mentioned this issue Jan 17, 2020
Merged
@jacobperron
Copy link
Member

See #556 for a fix.

@guru-florida
Copy link
Author

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?

@jacobperron
Copy link
Member

@guru-florida Are you able to share a launch file that I can use to reproduce the issue?

@jacobperron
Copy link
Member

jacobperron commented Jan 17, 2020 •
edited
Loading

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

jacobperron added a commit that referenced this issue Jan 28, 2020
@jacobperron
Fix behavior when provided a malformed parameters file (#556)
b6f6492 
* 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>
@jacobperron
Copy link
Member

This was fixed by #556.

@s-hillerk s-hillerk mentioned this issue Sep 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jacobperron jacobperron

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jacobperron @guru-florida