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

[Jacopo Mondi]

Hi Laurent,

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

> ---
>  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() ?

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

Thanks
   j

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