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

[Hirokazu Honda]

Hi Umang,

On Fri, Sep 24, 2021 at 5:19 PM Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On 9/24/21 1:10 PM, Hirokazu Honda wrote:
> > Hi Umang, thank you for the patch.
> >
> > On Fri, Sep 24, 2021 at 3:44 PM Umang Jain <umang.jain at ideasonboard.com> 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.
> >>
> >> Fix it by adding the descriptor to map first, before
> >> CameraWorker::queueRequest().
> >
> > I don't understand the problem well.
>
>
> It's a race between queueRequest() and adding to the descriptor_ std::map.
>
> queueRequest() is called asynchronously(separate thread)  so it might
> happen that queueRequest is still processing/queuing the request while
> we std::move()ed the descriptor (Which wraps the request queueRequest is
> accessing)
>
> If we fix the order with correct data-access it fixes the issue.
>
> > I think the pointer of descriptors.request_.get() is not invalidated
> > by std::move().
>
>
> Why do you think so? :)
>

std::move() invalidates descriptor. Yes, descriptor.request_ becomes
invalidated after std::move(). descriptor must not be accessed after
std::move().
However, queueRequest holds a pointer value,
descriptor.request_.get(). descriptor is move()d to descriptors_ and
thus descriptor.request_ is alive until descriptor is removed in
descriptors_.
So the passed pointer should still be valid after executing std::move().

>      diff --git a/src/android/camera_device.cpp
> b/src/android/camera_device.cpp
>      index a693dcbe..0de7050d 100644
>      --- a/src/android/camera_device.cpp
>      +++ b/src/android/camera_device.cpp
>      @@ -1063,13 +1063,13 @@ int
> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>                      state_ = State::Running;
>              }
>
>      - worker_.queueRequest(descriptor.request_.get());
>      -
>              {
>                      MutexLocker descriptorsLock(descriptorsMutex_);
> descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
>              }
>
>      + worker_.queueRequest(descriptor.request_.get());
>      +
>
> is a segfaulted here (and expected no?).
>
> >
> >> 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());
> > Accessing descriptors_ must be while holding descriptorsMutex_.
>
>
> I am in two minds here,
>
> We can lock it yes, but we are just reading from the map so, should be
> fine?

Regardless of reading/writing, descriptorMutex_ must be acquired
whenever accessing descriptors_.
Some other thread can clear descriptors in parallel. Parallel
executing operations against std::map corrupts it.
By the way, std::map::operator[] is not a constant function. In fact,
it can create a new node if a key does not exist.

-Hiro
>
> >
> > -Hiro
> >> +
> >>          return 0;
> >>   }
> >>
> >> --
> >> 2.31.1
> >>