Px4-autopilot: Memory leak in uORB teardown

[jkflying]

Describe the bug
In the uORBDeviceMaster memory is allocated for the device path and handed to the node, but the node doesn't take ownership of said memory.

Allocated, and if all is successful handed to DeviceNode:
https://github.com/PX4/Firmware/blob/e23e3d7baed6871841911a3aa19812ddb1547b6b/src/modules/uORB/uORBDeviceMaster.cpp#L99

DeviceNode gives it to CDev:
https://github.com/PX4/Firmware/blob/e23e3d7baed6871841911a3aa19812ddb1547b6b/src/modules/uORB/uORBDeviceNode.cpp#L60

which won''t delete it on destruction
https://github.com/PX4/Firmware/blob/e23e3d7baed6871841911a3aa19812ddb1547b6b/src/lib/cdev/CDev.cpp#L62

This isn't an issue on the FCU in real flights, but I'm trying to set up valgrind to find problems in tests, and this is one of them that shows up. It also prevents tearing down and setting up the uORB framework in between unit tests, each test will need to be run in its own process.

[bkueng]

We can delete it in DeviceNode's destructor, we just need to make sure CDev::~CDev() doesn't access it anymore.

Generally with uORB tear-down we need to ensure that no other thread is running anymore that potentially calls an uORB API.

[jkflying]

I tried this patch, but then sitl-uorb test started failing with a "double free or corruption":

diff --cc src/modules/uORB/uORBDeviceNode.cpp
index d6aad04,d6aad04..d047ce9
--- a/src/modules/uORB/uORBDeviceNode.cpp
+++ b/src/modules/uORB/uORBDeviceNode.cpp
@@@ -61,7 -61,7 +61,8 @@@ uORB::DeviceNode::DeviceNode(const stru
        _meta(meta),
        _instance(instance),
        _priority(priority),
--      _queue_size(queue_size)
++      _queue_size(queue_size),
++      _devname(path)
  {
  }

@@@ -70,6 -70,6 +71,11 @@@ uORB::DeviceNode::~DeviceNode(
        if (_data != nullptr) {
                delete[] _data;
        }
++
++      if (_devname != nullptr) {
++              free((void *)_devname);
++              _devname = nullptr;
++      }
  }

  int
diff --cc src/modules/uORB/uORBDeviceNode.hpp
index ef0a7b0,ef0a7b0..228bcd4
--- a/src/modules/uORB/uORBDeviceNode.hpp
+++ b/src/modules/uORB/uORBDeviceNode.hpp
@@@ -56,7 -56,7 +56,7 @@@ class uORB::DeviceNode : public cdev::C
  public:
        DeviceNode(const struct orb_metadata *meta, const uint8_t instance, const char *path, uint8_t priority,
                   uint8_t queue_size = 1);
--      ~DeviceNode();
++      virtual ~DeviceNode();

        // no copy, assignment, move, move assignment
        DeviceNode(const DeviceNode &) = delete;
@@@ -281,6 -281,6 +281,7 @@@ private
        bool _published{false};  /**< has ever data been published */
        uint8_t _queue_size; /**< maximum number of elements in the queue */
        int8_t _subscriber_count{0};
++      const char *_devname; /**< keep this to delete at teardown, since CDev doesn't */

        px4_task_t _publisher{0}; /**< if nonzero, current publisher. Only used inside the advertise call.
                                                We allow one publisher to have an open file descriptor at the same time. *

So clearly there's more to this story. Maybe in the tests we're passing in literals somewhere?

[bkueng]

Did you see https://github.com/PX4/Firmware/blob/e23e3d7baed6871841911a3aa19812ddb1547b6b/src/modules/uORB/uORBDeviceMaster.cpp#L137?

[jkflying]

Did you see

Awesome, that fixes it :+1: Dunno how I missed that...

I'll add the fix to https://github.com/PX4/Firmware/pull/12521

[dagar]

Each device node knows enough to construct the path on demand. We might be able to structure this to ditch the dynamic allocation entirely.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.