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

[Laurent Pinchart]

Hi Kieran,

On Fri, Jul 09, 2021 at 10:57:23AM +0100, Kieran Bingham wrote:
> On 05/07/2021 13:48, Laurent Pinchart wrote:
> > 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>
> 

It's a good question. While tests are meant to verify that the
implementation is comformant with the API, they embed some knowledge of
expected implementation errors. In this case, I'm not sure what issue
you think three messages would catch. I'm also not sure what you mean by
overlap.

> >>>>> +
> >>>>> +		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;
> >>>>>  	}
> >>>>>

-- 
Regards,

Laurent Pinchart