summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorLaurent Pinchart <laurent.pinchart@ideasonboard.com>2021-07-02 01:30:41 +0300
committerLaurent Pinchart <laurent.pinchart@ideasonboard.com>2021-07-11 17:42:02 +0300
commitda9b6bb196e0165342a414657edfc5aaf165baa5 (patch)
tree82dddf1faf7e571f1a7781a96f70b0d92b60115b
parent1f7f7a72ed47a4a9cbfcda9dc26f490d5ef7b1af (diff)
base: thread: Fix recursive calls to dispatchMessages()
There are use cases for calling the dispatchMessages() function recursively, from within a message handler. This can be used, for instance, to force delivery of messages posted to a thread concurrently to stopping the thread. This currently causes access, in the outer dispatchMessages() call, to iterators that have been invalidated by erasing list elements in the recursive call, leading to undefined behaviour (most likely double-free or other crashes). Fix it by only erasing messages from the list at the end of the outer call, identified using a recursion counter. Bug: https://bugs.libcamera.org/show_bug.cgi?id=26 Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Tested-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
-rw-r--r--src/libcamera/base/thread.cpp43
1 files changed, 33 insertions, 10 deletions
diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
index 7f791152..1232f895 100644
--- a/src/libcamera/base/thread.cpp
+++ b/src/libcamera/base/thread.cpp
@@ -126,6 +126,11 @@ public:
* \brief Protects the \ref list_
*/
Mutex mutex_;
+ /**
+ * \brief The recursion level for recursive Thread::dispatchMessages()
+ * calls
+ */
+ unsigned int recursion_ = 0;
};
/**
@@ -595,30 +600,34 @@ void Thread::removeMessages(Object *receiver)
* Messages shall only be dispatched from the current thread, typically within
* the thread from the run() function. Calling this function outside of the
* thread results in undefined behaviour.
+ *
+ * This function is not thread-safe, but it may be called recursively in the
+ * same thread from an object's message handler. It guarantees delivery of
+ * messages in the order they have been posted in all cases.
*/
void Thread::dispatchMessages(Message::Type type)
{
ASSERT(data_ == ThreadData::current());
+ ++data_->messages_.recursion_;
+
MutexLocker locker(data_->messages_.mutex_);
std::list<std::unique_ptr<Message>> &messages = data_->messages_.list_;
- for (auto iter = messages.begin(); iter != messages.end(); ) {
- std::unique_ptr<Message> &msg = *iter;
-
- if (!msg) {
- iter = data_->messages_.list_.erase(iter);
+ for (std::unique_ptr<Message> &msg : messages) {
+ if (!msg)
continue;
- }
- if (type != Message::Type::None && msg->type() != type) {
- ++iter;
+ if (type != Message::Type::None && msg->type() != type)
continue;
- }
+ /*
+ * Move the message, setting the entry in the list to null. It
+ * will cause recursive calls to ignore the entry, and the erase
+ * loop at the end of the function to delete it from the list.
+ */
std::unique_ptr<Message> message = std::move(msg);
- iter = data_->messages_.list_.erase(iter);
Object *receiver = message->receiver_;
ASSERT(data_ == receiver->thread()->data_);
@@ -629,6 +638,20 @@ void Thread::dispatchMessages(Message::Type type)
message.reset();
locker.lock();
}
+
+ /*
+ * If the recursion level is 0, erase all null messages in the list. We
+ * can't do so during recursion, as it would invalidate the iterator of
+ * the outer calls.
+ */
+ if (!--data_->messages_.recursion_) {
+ for (auto iter = messages.begin(); iter != messages.end(); ) {
+ if (!*iter)
+ iter = messages.erase(iter);
+ else
+ ++iter;
+ }
+ }
}
/**