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

[Laurent Pinchart]

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!
> 
> > > > +
> > > > +		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>
> 
> > > > +		}
> > > > +
> > > > +		delete recursiveReceiver;
> > > > +
> > > >  		return TestPass;
> > > >  	}
> > > >

-- 
Regards,

Laurent Pinchart