search

PX4 / PX4-Autopilot

PX4 Autopilot Software
https://px4.io
BSD 3-Clause "New" or "Revised" License
8.21k stars 13.38k forks source link

Mavlink: memory leak if already running or bad args #16877

Open lukegluke opened 3 years ago

lukegluke commented 3 years ago

Mavlink instance is allocated here https://github.com/PX4/PX4-Autopilot/blob/6bbb2faf8a0a67fc34fb15d616a97a281d6febab/src/modules/mavlink/mavlink_main.cpp#L2685

than task_main() of this instance is called, where instance is appended to _mavlink_instances so it can be normally free than https://github.com/PX4/PX4-Autopilot/blob/6bbb2faf8a0a67fc34fb15d616a97a281d6febab/src/modules/mavlink/mavlink_main.cpp#L2200-L2201 but task_main() may not reach this point if bad arguments https://github.com/PX4/PX4-Autopilot/blob/6bbb2faf8a0a67fc34fb15d616a97a281d6febab/src/modules/mavlink/mavlink_main.cpp#L2080-L2082 or serial already running/port already occupied https://github.com/PX4/PX4-Autopilot/blob/6bbb2faf8a0a67fc34fb15d616a97a281d6febab/src/modules/mavlink/mavlink_main.cpp#L2113-L2115 https://github.com/PX4/PX4-Autopilot/blob/6bbb2faf8a0a67fc34fb15d616a97a281d6febab/src/modules/mavlink/mavlink_main.cpp#L2128-L2130 So mavlink instance will not be correctly freed.

lukegluke commented 3 years ago

Hi @dagar, I suppose it could be fixed within your https://github.com/PX4/PX4-Autopilot/pull/16180

dagar commented 3 years ago

@lukegluke can you try https://github.com/PX4/PX4-Autopilot/pull/16830?

lukegluke commented 3 years ago

can you try #16830?

I don't see any significant difference regarding to the issue. Mavlink still allocated here https://github.com/PX4/PX4-Autopilot/blob/a9ef92b55e369b397aa39a4ab0579b16654b797a/src/modules/mavlink/mavlink_main.cpp#L2608 and get into mavlink_module_instances now in set_instance_id() https://github.com/PX4/PX4-Autopilot/blob/a9ef92b55e369b397aa39a4ab0579b16654b797a/src/modules/mavlink/mavlink_main.cpp#L226 and it almost the same place in task_main() as before https://github.com/PX4/PX4-Autopilot/blob/a9ef92b55e369b397aa39a4ab0579b16654b797a/src/modules/mavlink/mavlink_main.cpp#L2062 after possible returns on errors like this for example.

I suppose instance should be freed here on error return understanding somehow that task_main() doesn't reach "saving point" set_instance_id() https://github.com/PX4/PX4-Autopilot/blob/a9ef92b55e369b397aa39a4ab0579b16654b797a/src/modules/mavlink/mavlink_main.cpp#L2620

dagar commented 3 years ago

Thanks for checking, let's get it fixed.

lukegluke commented 2 years ago

@dagar if understand #19534 fixes this issue, isn't it? For now I don't have opportunity to check latest master.

dagar commented 2 years ago

@dagar if understand #19534 fixes this issue, isn't it? For now I don't have opportunity to check latest master.

Yes, although I'd like to take another pass at it later so it's a little more foolproof, I'm not convinced there aren't other edge cases.