ros-navigation / navigation2

ROS 2 Navigation Framework and System
https://nav2.org/
Other
2.51k stars 1.27k forks source link

heap-buffer-overflow bug caused by user misconfiguration (amcl:min_particles=a negative value) #4339

Closed GoesM closed 4 months ago

GoesM commented 4 months ago

this issue is mainly for adding ticket for https://github.com/ros-navigation/navigation2/issues/4005

Bug report

Required Info:

Steps to reproduce issue

Here is our launch command:

source install/setup.bash
source /opt/ros/humble/setup.bash
export TURTLEBOT3_MODEL=waffle
export GAZEBO_MODEL_PATH=$GAZEBO_MODEL_PATH:/opt/ros/humble/share/turtlebot3_gazebo/models
ros2 launch nav2_bringup tb3_simulation_launch.py params_file:=my_nav2_params.yaml

there's only one difference between my_nav2_params.yaml and defaulted nav2_params.yaml:

#my_nav2_params.yaml
......
nav2_amcl
      ......
    max_particles: 2000
    min_particles: -67897767946
    ......

Expected behavior

no bug occurs

Actual behavior

face to the asan report:

=================================================================
==150964==ERROR: AddressSanitizer: calloc parameters overflow: count * size (-1829840926 * 72) cannot be represented in type size_t (thread T0)
    #0 0x6468d0e16538 in __interceptor_calloc (/home/***/nav2_humble/install/nav2_amcl/lib/nav2_amcl/amcl+0xa9538) (BuildId: 3867e1c4deb9f2b10f5a588dd0fac0b28cac6c97)
    #1 0x71f8cc77b837 in pf_kdtree_alloc (/home/***/nav2_humble/install/nav2_amcl/lib/libpf_lib.so+0x9837) (BuildId: 5f790c1d486efe88d68d8730614daf5dc67b5248)

==150964==HINT: if you don't care about these errors you may set allocator_may_return_null=1
SUMMARY: AddressSanitizer: calloc-overflow (/home/***/nav2_humble/install/nav2_amcl/lib/nav2_amcl/amcl+0xa9538) (BuildId: 3867e1c4deb9f2b10f5a588dd0fac0b28cac6c97) in __interceptor_calloc
==150964==ABORTING

Additional information

It seems that here's already a check for the negative value, however it doesn't work actually.

https://github.com/ros-navigation/navigation2/blob/4fa12acec79a08f472d9d04ac08997db47f2a398/nav2_amcl/src/amcl_node.cpp#L1126-L1131

And if the value of min_particles is less than max_particles, min_particles's value should not affect the pf_alloc() function. So it's very odd to me .

I guess there may be a value change during the getparameter() , and the detail of why this check doesn't work needs to be checked.

GoesM commented 4 months ago

additional information

I insert log to confirm what happened after getparamer() as following:

  get_parameter("max_particles", max_particles_);
  get_parameter("min_particles", min_particles_);
std::cerr << "-------------------------------------------------------" << std::endl;
std::cerr << "max_particles:" << max_particles_ << std::endl;
std::cerr << "min_particles:" << min_particles_ << std::endl;
std::cerr << "-------------------------------------------------------" << std::endl;

and I get the log following:

[amcl-7] -------------------------------------------------------
[amcl-7] max_particles:2000
[amcl-7] min_particles:821708790
[amcl-7] -------------------------------------------------------

it's quiet strange that the get_parameter() read a wrong value.

SteveMacenski commented 4 months ago

-67897767946

That is out of bounds of what an int can represents (+/-2147483647), so my guess is that its a wrap around issue. That is undefined behavior so getting a weird number seems to track for me.

GoesM commented 4 months ago

uh of course you're right. I missed such situation. ^_^