[libcamera-devel] [PATCH 2/3] libcamera: pipeline: ipu3: Ensure that IPU3Frames::info is not used after delete

[Jacopo Mondi]

Hi Kieran,

On Wed, Mar 03, 2021 at 05:04:25PM +0000, Kieran Bingham wrote:
> When the IPU3Frames completes, it deletes the internal info storage.
>
> This storage contains the pointer to the Request, but in some cases the
> pointer was being accessed after the info structure was removed.
>
> Ensure that the Request is obtained before attempting to complete to
> obtain a valid pointer.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> ---
> This may be a further sign that we should rework how this is allocated,
> but for now - this patch fixes the crash which can occur when shutting
> down streams.
>
>
>     The blank line addition in the first hunk is intentional.

You replied to everything I would have questioned already :)

The FrameInfo API should be modified before it spreads to other
pipeline handlers.

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

Thanks
  j

>
>
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 2b4d31500533..9539393e5d84 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1164,6 +1164,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
>  		 * in action.controls to register additional metadata.
>  		 */
>  		Request *request = info->request;
> +
>  		info->metadataProcessed = true;
>  		if (frameInfos_.tryComplete(info))
>  			pipe_->completeRequest(request);
> @@ -1253,8 +1254,15 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
>  		return;
>
>  	info->paramDequeued = true;
> +
> +	/*
> +	 * tryComplete() will delete info if it completes the IPU3Frame.
> +	 * In that event, we must have obtained the Request before hand.
> +	 */
> +	Request *request = info->request;
> +
>  	if (frameInfos_.tryComplete(info))
> -		pipe_->completeRequest(info->request);
> +		pipe_->completeRequest(request);
>  }
>
>  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> @@ -1265,8 +1273,16 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
>  		info->metadataProcessed = true;
> +
> +		/*
> +		* tryComplete() will delete info if it completes the IPU3Frame.
> +		* In that event, we must have obtained the Request before hand.
> +		*/
> +		Request *request = info->request;
> +
>  		if (frameInfos_.tryComplete(info))
> -			pipe_->completeRequest(info->request);
> +			pipe_->completeRequest(request);
> +
>  		return;
>  	}
>
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel