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

[Laurent Pinchart]

Hi Kieran,

On Fri, Jul 09, 2021 at 01:36:13PM +0100, Kieran Bingham wrote:
> On 05/07/2021 13:33, Jacopo Mondi wrote:
> > On Mon, Jul 05, 2021 at 03:12:45PM +0300, Laurent Pinchart wrote:
> >> On Mon, Jul 05, 2021 at 12:48:11PM +0200, Jacopo Mondi wrote:
> >>> 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
> 
> to the .... ?

:-)

I'll drop the whole sentence, it's a leftover of a previous more complex
implementation, the current version doesn't keep track of the first
non-null entry.

> >>>>
> >>>> 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.
> 
> "in all cases delivery of" sounds odd.
> 
> Perhaps
> 
> "It guarantees delivery of messages in the order they have been posted
> in all cases".

I'll use this wording.

> Or
> 
> "In all cases, it guarantees 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 ?
> >>
> >> The function is documented as not being thread-safe. This is because it
> >> must be called from the thread created by the Thread class (see the
> >> assert at the beginning that enforces that). While it can be called
> >> recursively, there will never be two concurrent calls, so no race
> >> condition when accessing the recursion_ variable. This is unlike the
> >> messages list that needs to be protected by a lock because other threads
> >> can post messages to the list concurrently to
> >> Thread::dispatchMessages().
> 
> Sounds good, and Thread looks well asserted in most cases to guarantee
> rules are adhered to.
> 
> >>>> +	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 ?
> >>
> >> The function takes a message type parameter to restrict it to
> >> dispatching messages of a given type. The list may thus not be empty, it
> >> may still contain messages of other types.
> > 
> > So it is the other way around actually, if the message has been
> > dispatched, we erase it. Sorry, I read the condition wrong, thanks for
> > explaining.
> > 
> > A little note, you could:
> > 
> >         if (--data_->messages_.recursion_)
> >                 return;
> > 
> > And save one indentation level.
> > 
> > Apart from this:
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> With comments addressed if and as required:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >>>>  }
> >>>>
> >>>>  /**

-- 
Regards,

Laurent Pinchart