Camera::acquire() is not thread-safe

[Hans de Goede]

Hi,

On 8/15/24 5:33 PM, Hans de Goede wrote:
> Hi,
> 
> On 8/15/24 5:09 PM, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> On Thu, Aug 15, 2024 at 04:50:50PM +0200, Hans de Goede wrote:
>>> On 8/15/24 4:37 PM, Laurent Pinchart wrote:
>>>> On Thu, Aug 15, 2024 at 04:03:19PM +0200, Hans de Goede wrote:
>>>>> Hi All,
>>>>>
>>>>> Camera::acquire() claims to be thread-safe, but it relies on
>>>>> PipelineHandler::acquire() to provide that thread-safety and
>>>>> PipelineHandler::acquire() allows multiple acquires of the same
>>>>> pipeline-handler since a single pipeline-handler may register multiple
>>>>> cameras.
>>>>>
>>>>> So that leaves the following possible race:
>>>>>
>>>>> 1. Thread A calls Camera::acquire()
>>>>>    and passed the isAccessAllowed() check
>>>>>
>>>>> 2. Thread B calls Camera::acquire()
>>>>>    and passed the isAccessAllowed() check
>>>>>
>>>>> 3. Thread A successfully calls pipe_->acquire()
>>>>>
>>>>> 4. Thread B successfully calls pipe_->acquire()
>>>>
>>>> In this example, are threads A and B running in the same process ?
>>>
>>> Yes I'm talking about 2 threads of the same process
>>> racing here.
>>
>> You are right, two threads in the same application could successfully
>> acquire the same camera.
>>
>> This shouldn't be hard to fix, but I'm wondering if libcamera should
>> handle that, or if we should remove the threadsafe annotation for the
>> function. I expect the vast majority of applications to not acquire the
>> same camera from multiple threads, so adding a lock would be an extra
>> cost for little gain. Any opinion ?
> 
> I agree that just removing the thread-safe annotation might be
> the best fix. release() is already marked as not being thread-safe.

And remove this comment from Camera::acquire() :

        /*
         * No manual locking is required as PipelineHandler::lock() is
         * thread-safe.
         */

Regards,
 
Hans



>>>>> 5. Thread A calls setState(Private::CameraAcquired)
>>>>>
>>>>> 6. Thread B calls setState(Private::CameraAcquired)
>>>>>
>>>>> And now we have 2 threads succesfully having acquired the same camera
>>>>> which I believe is not supposed to be possible.
>>
>