From a2034754892a3108c89e2b0fd70fa27e26bcce9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beat=20K=C3=BCng?= Date: Tue, 5 Nov 2019 11:09:05 +0100 Subject: [PATCH] BlockingList: fix unsafe getLockGuard() API getLockGuard relies on copy elision to work correctly, which the compiler is not required to do (only with C++17). If no copy elision happens, the mutex ends up being unlocked twice, and the CS is executed with the mutex unlocked. The patch also ensures that the same pattern cannot be used again. --- platforms/common/px4_work_queue/WorkQueueManager.cpp | 6 +++--- src/include/containers/BlockingList.hpp | 2 +- src/include/containers/LockGuard.hpp | 3 +++ 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/platforms/common/px4_work_queue/WorkQueueManager.cpp b/platforms/common/px4_work_queue/WorkQueueManager.cpp index 3569238bad..77cc48103b 100644 --- a/platforms/common/px4_work_queue/WorkQueueManager.cpp +++ b/platforms/common/px4_work_queue/WorkQueueManager.cpp @@ -70,7 +70,7 @@ FindWorkQueueByName(const char *name) return nullptr; } - auto lg = _wq_manager_wqs_list->getLockGuard(); + LockGuard lg{_wq_manager_wqs_list->mutex()}; // search list for (WorkQueue *wq : *_wq_manager_wqs_list) { @@ -304,7 +304,7 @@ WorkQueueManagerStop() // first ask all WQs to stop if (_wq_manager_wqs_list != nullptr) { { - auto lg = _wq_manager_wqs_list->getLockGuard(); + LockGuard lg{_wq_manager_wqs_list->mutex()}; // ask all work queues (threads) to stop // NOTE: not currently safe without all WorkItems stopping first @@ -348,7 +348,7 @@ WorkQueueManagerStatus() const size_t num_wqs = _wq_manager_wqs_list->size(); PX4_INFO_RAW("\nWork Queue: %-1zu threads RATE INTERVAL\n", num_wqs); - auto lg = _wq_manager_wqs_list->getLockGuard(); + LockGuard lg{_wq_manager_wqs_list->mutex()}; size_t i = 0; for (WorkQueue *wq : *_wq_manager_wqs_list) { diff --git a/src/include/containers/BlockingList.hpp b/src/include/containers/BlockingList.hpp index 0a68f783f3..a44207748c 100644 --- a/src/include/containers/BlockingList.hpp +++ b/src/include/containers/BlockingList.hpp @@ -80,7 +80,7 @@ public: List::clear(); } - LockGuard getLockGuard() { return LockGuard{_mutex}; } + pthread_mutex_t &mutex() { return _mutex; } private: diff --git a/src/include/containers/LockGuard.hpp b/src/include/containers/LockGuard.hpp index b5528d1b00..d6670c036a 100644 --- a/src/include/containers/LockGuard.hpp +++ b/src/include/containers/LockGuard.hpp @@ -44,6 +44,9 @@ public: pthread_mutex_lock(&_mutex); } + LockGuard(const LockGuard &other) = delete; + LockGuard &operator=(const LockGuard &other) = delete; + ~LockGuard() { pthread_mutex_unlock(&_mutex);