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

[Jacopo Mondi]

Hi Laurent,

On Thu, Jun 17, 2021 at 12:12:28PM +0300, Laurent Pinchart wrote:
> 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.

Oh right, I missed the condition check

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

Yes, not easy to follow in full, thanks for the clarifications

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

Thanks
  j

>
> > >  	/*
> > >  	 * Queue the captured and the request buffer to the converter if format
>
> --
> Regards,
>
> Laurent Pinchart