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

[Laurent Pinchart]

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

> > +
> > +		this_thread::sleep_for(chrono::milliseconds(10));
> > +
> > +		if (!recursiveReceiver->success()) {
> > +			cout << "Recursive message delivery failed" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		delete recursiveReceiver;
> > +
> >  		return TestPass;
> >  	}
> >

-- 
Regards,

Laurent Pinchart