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

13925 diff mbox series

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.

Fix it by adding the descriptor to map first, before
CameraWorker::queueRequest().

Signed-off-by: Umang Jain <umang.jain@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(-)

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@ideasonboard.com>

> Fix it by adding the descriptor to map first, before
> CameraWorker::queueRequest().
> 
> Signed-off-by: Umang Jain <umang.jain@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;
>  }
>

Hi Umang, thank you for the patch.

On Tue, Sep 28, 2021 at 11:54 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@ideasonboard.com>
>

As Laurent said, std::move() is red-herring.
Although your fix is correct, there is a small window of the race,
which is Laurent explained in v1.
queueRequest() must be done after descriptor is in descriptors_ map for sure.

So with the commit message correction suggested by Laurent,

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

-Hiro
> > Fix it by adding the descriptor to map first, before
> > CameraWorker::queueRequest().
> >
> > Signed-off-by: Umang Jain <umang.jain@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