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

[Kieran Bingham]

On 03/03/2021 18:18, Laurent Pinchart wrote:
> 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
> 
> ?
> 

There's already a todo to rework the allocations. We need to tackle that
as soon as possible, which to me would include making it better so we
avoid this issue. But this patch fixes the bug as it stands now.

I'll add the todo all the same.



> 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
--
Kieran