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

[Hirokazu Honda]

Hi Umang,

On Fri, Sep 24, 2021 at 9:57 PM Hirokazu Honda <hiroh at chromium.org> wrote:
>
> 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.
>

Would you mind investigating more?

Best Regards,
-Hiro

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