[libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls to dispatchMessages()

[Jacopo Mondi]

Hi Laurent,

On Fri, Jul 02, 2021 at 02:07:41AM +0300, Laurent Pinchart wrote:
> 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 <laurent.pinchart at ideasonboard.com>
> ---
>  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<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.
> +	 */

is it paranoid to think accesses to recursions_ should be serialized ?

> +	if (!--data_->messages_.recursion_) {
> +		for (auto iter = messages.begin(); iter != messages.end(); ) {
> +			if (!*iter)
> +				iter = messages.erase(iter);
> +			else
> +				++iter;
> +		}
> +	}

Can null messages end up in the list ? can't you just clear() the
whole list ?

Thanks
  j

>  }
>
>  /**
> --
> Regards,
>
> Laurent Pinchart
>