[libcamera-devel] [PATCH v2] android: camera_device: Fix race on queuing capture request

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Fri, Sep 24, 2021 at 06:35:59PM +0530, Umang Jain wrote:
> The Camera3RequestDescriptor containing the capture request
> is adding to the descriptors_ map after a call to

s/adding/added/

> CameraWorker::queueRequest(). This is a race condition since
> CameraWorker::queueRequest() queues request to libcamera::Camera
> asynchronously and the addition of the descriptor to the map
> occurs with std::move(). Hence, it might happen that the async
> queueRequest() hasn't finished but the descriptor gets std::move()ed.

std::move() isn't really relevant here. I'd write

The Camera3RequestDescriptor containing the capture request is added to
the descriptors_ map after a call to CameraWorker::queueRequest(). This
is a race condition since CameraWorker::queueRequest() queues requests
to libcamera::Camera asynchronously. The requests may thus complete
before they get added to descriptors_, in which case requestComplete()
will fail to lookup the request in the map.

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

> Fix it by adding the descriptor to map first, before
> CameraWorker::queueRequest().
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> changes in v2:
> - Save the pointer first and directly use it to queueRequest()
>   after std::move
> ---
>  src/android/camera_device.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 21844e51..ab381168 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1065,13 +1065,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		state_ = State::Running;
>  	}
>  
> -	worker_.queueRequest(descriptor.request_.get());
> +	CaptureRequest *request = descriptor.request_.get();
>  
>  	{
>  		MutexLocker descriptorsLock(descriptorsMutex_);
>  		descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
>  	}
>  
> +	worker_.queueRequest(request);
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart