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

[Hirokazu Honda]

Hi Umang, thank you for the patch.

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

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