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

[Kieran Bingham]

Hi Laurent,


On 05/07/2021 13:33, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Mon, Jul 05, 2021 at 03:12:45PM +0300, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> 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 .... ?


>>>>
>>>> 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".

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>


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