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

[Hirokazu Honda]

Hi Umang,

On Fri, Sep 24, 2021 at 9:39 PM Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On 9/24/21 5:07 PM, Hirokazu Honda wrote:
> > 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().
>
>
> Generally, in other programming models, I have often seen data that has
> been move()d should be now/further accessed(if necessary)  from that
> container that they have been move()d into. It clearly differentiates
> between the ownership model (before and after) and I find it as a good
> mental model to have (helps especially when there is a long function
> implementation )
>
> Yes, we can store a pointer before the move and call queueRequest() with
> it after the move. Either of the methods, I am not against any one.
>

Since copying a pointer value is before std::move(), it seems to be
natural for me to use descriptor.request_.get(), which is the original
code.
Hmm, I don't understand why the original code doesn't work. I guess
the seg fault is due to other reason.

-Hiro
> >
> >>       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
>
>
> Not sure, if this a possible yet, but sure!
>
> Thanks for the inputs.
>
> > 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
> >>>>