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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

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.

I agree, it's too easy to get it wrong as shown by the error.

>     The blank line addition in the first hunk is intentional.
> 
>  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.

Maybe

	 * \todo Improve the FrameInfo API to avoid this type of issue

?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	 */
> +	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;
>  	}
>  

-- 
Regards,

Laurent Pinchart