From patchwork Thu Jul 1 23:07:39 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 12762 X-Patchwork-Delegate: laurent.pinchart@ideasonboard.com Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id D16C8C3223 for ; Thu, 1 Jul 2021 23:08:31 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id EA48C684F2; Fri, 2 Jul 2021 01:08:30 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="N+dQTRCG"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 022F3684E3 for ; Fri, 2 Jul 2021 01:08:26 +0200 (CEST) Received: from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 7002AA15; Fri, 2 Jul 2021 01:08:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1625180905; bh=HPotrqiJYhYzaqt76k73aSYoCoVjoc8I/E0b9rACjvA=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=N+dQTRCGWzir7wtlVyN0J+G7hLvWf8yXTCKs5oGbykoasw3gnWSnY6xiefOH+r6Ir JFKtnZfv13mNrVay6YVmfcfl/Cme+KCaJHGeP/MaDXMJkL+TUfy4yFOHcyIwKxAVRS mmB6e/2erujg6HrrtyPL2PKL1BAr2dR2iVfP8mf0= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Fri, 2 Jul 2021 02:07:39 +0300 Message-Id: <20210701230741.14320-2-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210701230741.14320-1-laurent.pinchart@ideasonboard.com> References: <20210701230741.14320-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 1/3] base: thread: Document the postMessage() function as thread-safe X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The Thread::postMessage() function is thread-safe, document it as such. Signed-off-by: Laurent Pinchart Reviewed-by: Kieran Bingham --- src/libcamera/base/thread.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp index c7c2d6b29d6a..7f79115222e8 100644 --- a/src/libcamera/base/thread.cpp +++ b/src/libcamera/base/thread.cpp @@ -526,6 +526,8 @@ EventDispatcher *Thread::eventDispatcher() * * If the \a receiver is not bound to this thread the behaviour is undefined. * + * \context This function is \threadsafe. + * * \sa exec() */ void Thread::postMessage(std::unique_ptr msg, Object *receiver) From patchwork Thu Jul 1 23:07:40 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 12763 X-Patchwork-Delegate: laurent.pinchart@ideasonboard.com Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 71A4EC3222 for ; Thu, 1 Jul 2021 23:08:32 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 452C9684FA; Fri, 2 Jul 2021 01:08:31 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="Us7I5Qkg"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 439D8684E4 for ; Fri, 2 Jul 2021 01:08:26 +0200 (CEST) Received: from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id D4142D89; Fri, 2 Jul 2021 01:08:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1625180906; bh=iWNAMCBnINoGWXKaO/8sOU6Au1BP2vvyHmdX6CATKbg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Us7I5QkgzXfwEv15SDMa9iz1BLTUZO69efkKKDUIgNr17fWGKrpb2Qj0ZXBGREU7E Dlcddu9W/Lhrp5cI4W45dDNLh8guB8Xr219eFzciU6Aq04PZIz6SyVvLtOAwQiDUJa 2ewOUg2+icsFQX+kyVgAf9DQesvgHxgYQza3BAZs= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Fri, 2 Jul 2021 02:07:40 +0300 Message-Id: <20210701230741.14320-3-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210701230741.14320-1-laurent.pinchart@ideasonboard.com> References: <20210701230741.14320-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 2/3] test: message: Test recursive Thread::dispatchMessages() calls X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The Thread::dispatchMessages() function needs to support recursive calls, for instance to allow flushing delivery of invoked methods. Add a corresponding test, which currently fails with a double free. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Kieran Bingham --- test/message.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/test/message.cpp b/test/message.cpp index eeea57feab76..7895cd7c2106 100644 --- a/test/message.cpp +++ b/test/message.cpp @@ -26,8 +26,8 @@ public: MessageReceived, }; - MessageReceiver() - : status_(NoMessage) + MessageReceiver(Object *parent = nullptr) + : Object(parent), status_(NoMessage) { } @@ -52,6 +52,45 @@ private: Status status_; }; +class RecursiveMessageReceiver : public Object +{ +public: + RecursiveMessageReceiver() + : child_(this), success_(false) + { + } + + bool success() const { return success_; } + +protected: + void message([[maybe_unused]] Message *msg) + { + if (msg->type() != Message::None) { + Object::message(msg); + return; + } + + child_.postMessage(std::make_unique(Message::None)); + + /* + * If the child has already received the message, something is + * wrong. + */ + if (child_.status() != MessageReceiver::NoMessage) + return; + + Thread::current()->dispatchMessages(Message::None); + + /* The child should now have received the message. */ + if (child_.status() == MessageReceiver::MessageReceived) + success_ = true; + } + +private: + MessageReceiver child_; + bool success_; +}; + class SlowMessageReceiver : public Object { protected: @@ -120,6 +159,29 @@ protected: delete slowReceiver; + this_thread::sleep_for(chrono::milliseconds(100)); + + /* + * Test recursive calls to Thread::dispatchMessages(). Messages + * should be delivered correctly, without crashes or memory + * leaks. Two messages need to be posted to ensure we don't only + * test the simple case of a queue containing a single message. + */ + RecursiveMessageReceiver *recursiveReceiver = new RecursiveMessageReceiver(); + recursiveReceiver->moveToThread(&thread_); + + recursiveReceiver->postMessage(std::make_unique(Message::None)); + recursiveReceiver->postMessage(std::make_unique(Message::UserMessage)); + + this_thread::sleep_for(chrono::milliseconds(10)); + + if (!recursiveReceiver->success()) { + cout << "Recursive message delivery failed" << endl; + return TestFail; + } + + delete recursiveReceiver; + return TestPass; } From patchwork Thu Jul 1 23:07:41 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 12764 X-Patchwork-Delegate: laurent.pinchart@ideasonboard.com Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 9886EC3224 for ; Thu, 1 Jul 2021 23:08:32 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 941AE684FD; Fri, 2 Jul 2021 01:08:31 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="L+C2ctZ5"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id A6E27684E3 for ; Fri, 2 Jul 2021 01:08:26 +0200 (CEST) Received: from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C3ADE53; Fri, 2 Jul 2021 01:08:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1625180906; bh=Glf15EbSOCrKKbh3HM/ycvOlUuHT+xYMVwy/up78d3E=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=L+C2ctZ5Jws6EgP6MJXWu0LFOnMLbAEFl6/SnbLLsNnpr5pWhwIohnVc3zKn59uUY psowLNuzwGutbQ5k7sMoYJJ6NDJiOZpsW5UaD+0M6kgA2WKc0yf6UDtd/YYu331JFQ VzAu8wCjp4Uf+EfenQg4TJTdlLNXrcIma+KsP/Rg= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Fri, 2 Jul 2021 02:07:41 +0300 Message-Id: <20210701230741.14320-4-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210701230741.14320-1-laurent.pinchart@ideasonboard.com> References: <20210701230741.14320-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls to dispatchMessages() X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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. We need to keep track of the first non-null entry in the posted messages list to restrict the deletion to the Bug: https://bugs.libcamera.org/show_bug.cgi?id=26 Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Tested-by: David Plowman Reviewed-by: Kieran Bingham --- src/libcamera/base/thread.cpp | 43 +++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp index 7f79115222e8..9eb488d46db8 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 in all cases + * delivery of messages in the order they have been posted. */ void Thread::dispatchMessages(Message::Type type) { ASSERT(data_ == ThreadData::current()); + ++data_->messages_.recursion_; + MutexLocker locker(data_->messages_.mutex_); std::list> &messages = data_->messages_.list_; - for (auto iter = messages.begin(); iter != messages.end(); ) { - std::unique_ptr &msg = *iter; - - if (!msg) { - iter = data_->messages_.list_.erase(iter); + for (std::unique_ptr &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 = 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; + } + } } /**