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

[Jacopo Mondi]

Hi Laurent,

On Mon, Jul 05, 2021 at 03:16:32PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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!

> > > +
> > > +		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<> ?

With this fixed

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

> > > +		}
> > > +
> > > +		delete recursiveReceiver;
> > > +
> > >  		return TestPass;
> > >  	}
> > >
>
> --
> Regards,
>
> Laurent Pinchart