Fix for use-after-free that causes unpredicable crashes on serialization

SteveMacenski / slam_toolbox

Slam Toolbox for lifelong mapping and localization in potentially massive maps with ROS

GNU Lesser General Public License v2.1

1.68k stars 525 forks source link

Fix for use-after-free that causes unpredicable crashes on serialization #455

Closed jaredohmni closed 2 years ago

jaredohmni commented 3 years ago

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#393
Primary OS tested on	Ubuntu
Robotic platform tested on	Ohmni robots

Description of contribution in a few bullet points

Explicitly set m_pPoseResponse to NULL in the ScanMatcher constructor (following existing convention there which is why not using nullptr)
Set m_pPoseResponse to nullptr after delete[] is called to avoid keeping a stale reference to a free'd block
Modify logic in ScanMatcher::serialize to basically serialize to and from some dummy data that we new and free, since the serialized data is not relevant for operation anyways.

Description of documentation updates required from your changes

None required.

Future work that may be required in bullet points

None.

SteveMacenski commented 3 years ago

I'm happy with this, can you do some testing and get back to me that its all OK? Then I'll merge and backport to all the distributions

jaredohmni commented 2 years ago

Things look good, been running stably for us across our fleet for the last week. But caveat is that we are mainly testing the saving side. We are saving the posegraph data so that we can run validation tests to validate switching over to slam_toolbox localization, but for now we are still using AMCL so are not deserializing posegraphs across our fleet. If someone has historical posegraphs that they deserialize from regularly, would be great to get some extra feedback to make sure things are 100% stable there as well.

SteveMacenski commented 2 years ago

Do you not have any posegraphs generated before this change to check for backwards compatibility?

Have you tested much with deserialization (or actually not at all)?

I don't suspect to see any issues there, I think if you tried a couple dozen times at deserializing an old file without an issue, that would be enough for me.

jaredohmni commented 2 years ago

Yeah, we've run about 20 deserializations by hand with posegraph/data files made before and after the change and have observed no issues. I concur that given the restricted scope of changes issues will be unlikely 👍

SteveMacenski commented 2 years ago

OK, give me a day to get the time to backport and I'll merge this all. Thanks for the help!!