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

[Umang Jain]

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? :)

     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?

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