[libcamera-devel] [PATCH] libcamera: pipeline: simple: Fix crash when storing timestamp in metadata

[Laurent Pinchart]

Hi Jacopo,

On Thu, Jun 17, 2021 at 10:10:21AM +0200, Jacopo Mondi wrote:
> On Thu, Jun 17, 2021 at 02:44:22AM +0300, Laurent Pinchart wrote:
> > Commit 922833f774f6 ("libcamera: simple: Report sensor timestamp")
> > unconditionally tries to access the request through the capture buffer
> > to store the capture timestamp in the metadata. This causes a null
> > pointer dereference when using a converter, as the capture buffers are
> > free-wheeling in that case, and not associated with a request.
> >
> > Fix this by getting the request from the user-facing buffer, which can
> > be the capture buffer when no converter is used.
> >
> > Fixes: 922833f774f6 ("libcamera: simple: Report sensor timestamp")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> My bad, my understanding of the converters in the simple pipeline is
> limited. Thanks for fixing

No worries. I missed it during review too :-)

> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 36fd9b852b33..e63d0bfd2fcb 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -1127,14 +1127,28 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)
> >  	}
> >
> >  	/*
> > -	 * Record the sensor's timestamp in the request metadata.
> > +	 * Record the sensor's timestamp in the request metadata. The request
> > +	 * needs to be obtained from the user-facing buffer, as internal
> > +	 * buffers are free-wheeling and have no request associated with them.
> >  	 *
> >  	 * \todo The sensor timestamp should be better estimated by connecting
> >  	 * to the V4L2Device::frameStart signal if the platform provides it.
> >  	 */
> >  	Request *request = buffer->request();
> > -	request->metadata().set(controls::SensorTimestamp,
> > -				buffer->metadata().timestamp);
> > +
> > +	if (data->useConverter_ && !data->converterQueue_.empty()) {
> > +		const std::map<unsigned int, FrameBuffer *> &outputs =
> > +			data->converterQueue_.front();
> > +		if (!outputs.empty()) {
> 
> Can outputs be empty if !data->converterQueue_.empty() ?

They shouldn't, but as there's the same sanity check in
converter->queueBuffers(), which is called *after* this code block, I
thought it would be best to be cautious. Eventually we should probably
rework this to only queue buffers for capture when there are request
queued, the same way te IPU3 pipeline handler does it, but that was a
too big change for a fix.

> > +			FrameBuffer *outputBuffer = outputs.begin()->second;
> > +			if (outputBuffer)
> > +				request = outputBuffer->request();
> > +		}
> > +	}
> > +
> > +	if (request)
> > +		request->metadata().set(controls::SensorTimestamp,
> > +					buffer->metadata().timestamp);
> 
> We could end up without a request
> Before 922833f774f6 the Request * was simply retreived with
> 
>         Request *request = buffer->request();
> 
> If we can now end up without a Request *, how can we complete it here
> below ?

request can be null here if

	data->useConverter && data->converterQueue_.empty()

In that case we'll take the `if (data->useConverter_)` branch, and won't
reach the completeBuffer() call.

It's a little bit convoluted, the other option was to deplicate the
request->metadata().set() call, which I didn't like much.

> >  	/*
> >  	 * Queue the captured and the request buffer to the converter if format

-- 
Regards,

Laurent Pinchart