From c3c30e5d4fabc6beffc98c82d219a4fe410f8a28 Mon Sep 17 00:00:00 2001 From: Jukka Laitinen Date: Tue, 24 Aug 2021 12:54:49 +0300 Subject: [PATCH] Fix memory corruption when work queue is being deleted When the last WorkItem is deleted, it is removed from a work queue and the queue is being stopped. But, the queue itself might get deleted in the middle, in a higher priority thread than where the WorkItem deletion was performed from If the WorkQueue::Detach accesses the member variables after this, there is memory corruption This happens in particular when launching i2c or spi devices in I2CSPIDriverBase::module_start: - The "initializer" is deleted when the instance is not found and the iterator while loop continues. - The workqueue is deleted in the middle of "initializer" deletion when the WorkQueueRunner returns. This prevents deletion of the WorkQueue before the Detach has been finished, in the specific case that the ::Detach triggers the deletion Signed-off-by: Jukka Laitinen --- .../px4_work_queue/WorkQueue.hpp | 1 + platforms/common/px4_work_queue/WorkQueue.cpp | 22 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/platforms/common/include/px4_platform_common/px4_work_queue/WorkQueue.hpp b/platforms/common/include/px4_platform_common/px4_work_queue/WorkQueue.hpp index d1ad917cdd..f2a66bb898 100644 --- a/platforms/common/include/px4_platform_common/px4_work_queue/WorkQueue.hpp +++ b/platforms/common/include/px4_platform_common/px4_work_queue/WorkQueue.hpp @@ -96,6 +96,7 @@ private: IntrusiveQueue _q; px4_sem_t _process_lock; + px4_sem_t _exit_lock; const wq_config_t &_config; BlockingList _work_items; px4::atomic_bool _should_exit{false}; diff --git a/platforms/common/px4_work_queue/WorkQueue.cpp b/platforms/common/px4_work_queue/WorkQueue.cpp index 48b88c7290..fa32017912 100644 --- a/platforms/common/px4_work_queue/WorkQueue.cpp +++ b/platforms/common/px4_work_queue/WorkQueue.cpp @@ -59,11 +59,20 @@ WorkQueue::WorkQueue(const wq_config_t &config) : px4_sem_init(&_process_lock, 0, 0); px4_sem_setprotocol(&_process_lock, SEM_PRIO_NONE); + + px4_sem_init(&_exit_lock, 0, 1); + px4_sem_setprotocol(&_exit_lock, SEM_PRIO_NONE); } WorkQueue::~WorkQueue() { + work_lock(); + + // Synchronize with ::Detach + px4_sem_wait(&_exit_lock); + px4_sem_destroy(&_exit_lock); + px4_sem_destroy(&_process_lock); work_unlock(); @@ -83,11 +92,14 @@ bool WorkQueue::Attach(WorkItem *item) } work_unlock(); + return false; } void WorkQueue::Detach(WorkItem *item) { + bool exiting = false; + work_lock(); _work_items.remove(item); @@ -96,11 +108,21 @@ void WorkQueue::Detach(WorkItem *item) // shutdown, no active WorkItems PX4_DEBUG("stopping: %s, last active WorkItem closing", _config.name); + // Deletion of this work queue might happen right after request_stop or + // SignalWorkerThread. Use a separate lock to prevent premature deletion + px4_sem_wait(&_exit_lock); + exiting = true; request_stop(); SignalWorkerThread(); } work_unlock(); + + // In case someone is deleting this wq already, signal + // that it is now allowed + if (exiting) { + px4_sem_post(&_exit_lock); + } } void WorkQueue::Add(WorkItem *item)