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

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Fri, Sep 24, 2021 at 12:14:09PM +0530, Umang Jain wrote:
> The Camera3RequestDescriptor containing the capture request
> is adding to the descriptors_ map after a call to
> 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.

The issue isn't related to std::move(). The problem is that the request
is queued to the worker, and at that point processing moves to another
thread. It's thus possible (even if very unlikely, but still possible)
that the processing will complete, and CameraDevice::requestComplete()
gets called, before this function proceeds to add the request to the
descriptors_ map. CameraDevice::requestComplete() will then not find it
in the map, and bad things will happen.

It's a classic race condition, std::move() doesn't play any role here.

> Fix it by adding the descriptor to map first, before
> CameraWorker::queueRequest().
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 21844e51..c4c03d86 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		state_ = State::Running;
>  	}
>  
> -	worker_.queueRequest(descriptor.request_.get());
> -
> +	unsigned long requestCookie = descriptor.request_->cookie();
>  	{
>  		MutexLocker descriptorsLock(descriptorsMutex_);
> -		descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
> +		descriptors_[requestCookie] = std::move(descriptor);
>  	}
>  
> +	worker_.queueRequest(descriptors_[requestCookie].request_.get());

As Hiro mentioned, we shouldn't access descriptors_ without taking the
lock. Here's how to fix it:

	CaptureRequest *request = descriptor.request_.get();

	{
		MutexLocker descriptorsLock(descriptorsMutex_);
		descriptors_[request->cookie()] = std::move(descriptor);
	}

	worker_.queueRequest(request);

> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart