Skip to content

Commit

Permalink
Fix memory issues in destructors in uORB manager and CDev
Browse files Browse the repository at this point in the history
  • Loading branch information
jkflying committed Aug 8, 2019
1 parent 8d10ed0 commit 6727250
Show file tree
Hide file tree
Showing 7 changed files with 57 additions and 8 deletions.
23 changes: 23 additions & 0 deletions src/lib/cdev/CDev.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -395,4 +395,27 @@ CDev::remove_poll_waiter(px4_pollfd_struct_t *fds)
return -EINVAL;
}

int CDev::unregister_driver_and_memory()
{
int retval = PX4_OK;

if (_registered) {
unregister_driver(_devname);
_registered = false;

} else {
retval = -ENODEV;
}

if (_devname != nullptr) {
free((void *)_devname);
_devname = nullptr;

} else {
retval = -ENODEV;
}

return retval;
}

} // namespace cdev
11 changes: 11 additions & 0 deletions src/lib/cdev/CDev.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,17 @@ class __EXPORT CDev

px4_sem_t _lock; /**< lock to protect access to all class members (also for derived classes) */


/**
* First, unregisters the driver. Next, free the memory for the devname,
* in case it was expected to have ownership. Sets devname to nullptr.
*
* This is only needed if the ownership of the devname was passed to the CDev, otherwise ~CDev handles it.
*
* @return PX4_OK on success, -ENODEV if the devname is already nullptr
*/
int unregister_driver_and_memory();

private:
const char *_devname{nullptr}; /**< device node name */

Expand Down
7 changes: 2 additions & 5 deletions src/modules/uORB/uORBDeviceMaster.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,10 @@ uORB::DeviceMaster::advertise(const struct orb_metadata *meta, int *instance, in
return -ENOMEM;
}

/* construct the new node */
/* construct the new node, passing the ownership of path to it */
uORB::DeviceNode *node = new uORB::DeviceNode(meta, group_tries, devpath, priority);

/* if we didn't get a device, that's bad */
/* if we didn't get a device, that's bad, free the path too */
if (node == nullptr) {
free((void *)devpath);
return -ENOMEM;
Expand Down Expand Up @@ -133,9 +133,6 @@ uORB::DeviceMaster::advertise(const struct orb_metadata *meta, int *instance, in
}
}

/* also discard the name now */
free((void *)devpath);

} else {
// add to the node map;.
_node_list.add(node);
Expand Down
3 changes: 2 additions & 1 deletion src/modules/uORB/uORBDeviceNode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@

#include "uORBDeviceNode.hpp"

#include "uORBDeviceNode.hpp"
#include "uORBUtils.hpp"
#include "uORBManager.hpp"

Expand Down Expand Up @@ -70,6 +69,8 @@ uORB::DeviceNode::~DeviceNode()
if (_data != nullptr) {
delete[] _data;
}

CDev::unregister_driver_and_memory();
}

int
Expand Down
2 changes: 1 addition & 1 deletion src/modules/uORB/uORBDeviceNode.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class uORB::DeviceNode : public cdev::CDev, public ListNode<uORB::DeviceNode *>
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;
Expand Down
11 changes: 11 additions & 0 deletions src/modules/uORB/uORBManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,17 @@ bool uORB::Manager::initialize()
return _Instance != nullptr;
}

bool uORB::Manager::terminate()
{
if (_Instance != nullptr) {
delete _Instance;
_Instance = nullptr;
return true;
}

return false;
}

uORB::Manager::Manager()
{
#ifdef ORB_USE_PUBLISHER_RULES
Expand Down
8 changes: 7 additions & 1 deletion src/modules/uORB/uORBManager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ class uORB::Manager
*/
static bool initialize();

/**
* Terminate the singleton. Call this after everything else.
* @return true on success
*/
static bool terminate();

/**
* Method to get the singleton instance for the uORB::Manager.
* Make sure initialize() is called first.
Expand Down Expand Up @@ -416,7 +422,7 @@ class uORB::Manager

private: //class methods
Manager();
~Manager();
virtual ~Manager();

#ifdef ORB_COMMUNICATOR
/**
Expand Down

0 comments on commit 6727250

Please sign in to comment.