[libcamera-devel] [PATCH 2/3] test: message: Test recursive Thread::dispatchMessages() calls

[Kieran Bingham]

Hi Laurent,

Replying here to follow existing discussions...


On 05/07/2021 13:48, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Mon, Jul 05, 2021 at 02:38:11PM +0200, Jacopo Mondi wrote:
>> On Mon, Jul 05, 2021 at 03:16:32PM +0300, Laurent Pinchart wrote:
>>> On Mon, Jul 05, 2021 at 01:08:29PM +0200, Jacopo Mondi wrote:
>>>> On Fri, Jul 02, 2021 at 02:07:40AM +0300, Laurent Pinchart wrote:
>>>>> 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 <laurent.pinchart at ideasonboard.com>
>>>>> ---
>>>>>  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>(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>(Message::None));
>>>>> +		recursiveReceiver->postMessage(std::make_unique<Message>(Message::UserMessage));
>>>>
>>>> I'm not sure why two messages are necessary, also considering that
>>>> messages != Message::None are ignored...
>>>>
>>>> If my understanding is correct, a single postMessage() triggers a
>>>> dispatcher->interrupt() which triggers a dispatchMessages() as the receiver
>>>> runs on a different thread and can be interrupted. The message is then
>>>> dispatched and handled by the execution of RecursiveMessageReceiver::message()
>>>> call.
>>>>
>>>> From there we post a message to the child and go throuh the same path if not
>>>> that the child runs on the same thread and we have to dispatch
>>>> messages forcefully, causing a recursive call to dispatchMessages() on
>>>> the same thread of execution.
>>>>
>>>> Am I missing something or is the second message not required ?
>>>
>>> The second message is needed to ensure we don't only test a case where
>>> the queue has a single message. The test doesn't fail in that case, due
>>> to the fact that the internal iter variable in
>>> Thread::dispatchMessages() is incremented before dispatching the current
>>> message. If the list contains a single message, iter will thus be the
>>> end() of the list after being incremented, which will not cause the
>>> outer dispatchMessage() call to fail with use-after-free (the .end()
>>> iterator is not invalidated by an erase operation on a std::list).
>>
>> Oh I see, thanks for clarifying this!

Are there any further edge cases here? I.e. should we post three
messages to ensure that they all clean up iteratively?

I am only suggesting this in case, conceptually there are two overlaps -
where three messages would ensure an overlap for the both the beginning
and end of message two for instance..


<message one>
      <message two>
              <message three>


>>>>> +
>>>>> +		this_thread::sleep_for(chrono::milliseconds(10));
>>>>> +
>>>>> +		if (!recursiveReceiver->success()) {
>>>>> +			cout << "Recursive message delivery failed" << endl;
>>>>> +			return TestFail;
>>
>> Doesn't this leak recursiveReceiver ?
>> Can it be allocated on the stack or wrapped in a unique_ptr<> ?
> 
> You're right. I initially thought it was just in an error path in a
> test, but that matters too. I'll use a unique_ptr.
> 
>> With this fixed
>>
>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Yup, I'll go along with that.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>



>>
>>>>> +		}
>>>>> +
>>>>> +		delete recursiveReceiver;
>>>>> +
>>>>>  		return TestPass;
>>>>>  	}
>>>>>
>