[amcl] reset laser_scan_filter before reinit

ros-navigation / navigation2

ROS 2 Navigation Framework and System

https://nav2.org/

Other

2.31k stars 1.2k forks source link

[amcl] reset laser_scan_filter before reinit #4397

Closed GoesM closed 1 month ago

GoesM commented 1 month ago

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#4379
Primary OS tested on	Ubuntu
Robotic platform tested on	gazebo simulation
Does this PR contain AI generated software?	No

Description of contribution in a few bullet points

reset laser_scan_filter_ before reinit it
avoid UAF bugs mentioned in #4379

Future work that may be required in bullet points

For Maintainers:

[ ] Check that any new parameters added are updated in docs.nav2.org
[ ] Check that any significant change is added to the migration guide
[ ] Check that any new features OR changes to existing behaviors are reflected in the tuning guide
[ ] Check that any new functions have Doxygen added
[ ] Check that any new features have test coverage
[ ] Check that any new plugins is added to the plugins page
[ ] If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

SteveMacenski commented 1 month ago

Does this actually fix the issue? Isn't the laser scanner still able to process scans while parameters are changing? I thought you were going to reset it at the start of the callback and always reset the message filter

GoesM commented 1 month ago

I‘m so sorry for any confusion caused by my previous description.

The following statement may be more clear:

After such dynamic parameters change, because of the lack of scan_filter_.reset() before initMessageFilters() , the scan_filter_ is still running even after scan_sub_.reset().

BUT based on the definition of scan_filter_, it would access the freed pointer scan_sub_:

https://github.com/ros-navigation/navigation2/blob/7dc76da7cf166e9a1b9345c5d3e91544b1ca696b/nav2_amcl/src/amcl_node.cpp#L1515-L1519

so that , the use-after-free bug occurs.

So what I did was add this missing code.

SteveMacenski commented 1 month ago

Got it! It may have been that you described it right and I just jumped the gun (I've been known to do that) thinking I knew where you were going with that. Thanks!