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

[Umang Jain]

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.

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