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

[GoesM]

this issue is mainly for adding ticket for #4005

Bug report

Required Info:

• Operating System:
  • Ubuntu 22.04
• ROS2 Version:
  • humble
• Version or commit hash:
  • the latest
• DDS implementation:
  • default

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.

navigation2/nav2_amcl/src/amcl_node.cpp

Lines 1126 to 1131 in 4fa12ac

	if (min_particles_ < 0) {
	RCLCPP_WARN(
	get_logger(), "You've set min_particles to be negative,"
	" this isn't allowed so it will be set to default value 500.");
	min_particles_ = 500;
	}

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]

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]

-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]

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