Camera::acquire() is not thread-safe

[Laurent Pinchart]

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 ?

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

-- 
Regards,

Laurent Pinchart