[libcamera-devel,v2,09/11] android: camera_device: Fix variables access without protection

14845 diff mbox series

This fixes the code accessing descriptors and
Camera3RequestDescriptor::pendingStreamsToProcess_ without holding
descriptorsMutex_ and Camera3RequestDescriptor::streamProcessMutex_
in CameraDevice.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Hi Hiro,

Thank you for the patch.

On Mon, Nov 29, 2021 at 08:44:51PM +0900, Hirokazu Honda wrote:
> This fixes the code accessing descriptors and
> Camera3RequestDescriptor::pendingStreamsToProcess_ without holding
> descriptorsMutex_ and Camera3RequestDescriptor::streamProcessMutex_
> in CameraDevice.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.cpp | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f2e0bdbd..59185e7f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -422,7 +422,11 @@ void CameraDevice::stop()
>  	worker_.stop();
>  	camera_->stop();
>  
> -	descriptors_ = {};
> +	{
> +		MutexLocker descriptorsLock(descriptorsMutex_);
> +		descriptors_ = {};
> +	}
> +
>  	streams_.clear();
>  
>  	state_ = State::Stopped;
> @@ -919,6 +923,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		 */
>  		FrameBuffer *frameBuffer = nullptr;
>  		int acquireFence = -1;
> +
> +		MutexLocker lock(descriptor->streamsProcessMutex_);

I'd add a blank line here.

It's annoying to take a lock here, when we know that there can be no
concurrent access. Is there any way to tell the thread analyzer that we
know what we're doing ? If not, I really hope std::mutex is backed by
futexes, and very cheap to lock and unlock when there's no contention.

>  		switch (cameraStream->type()) {
>  		case CameraStream::Type::Mapped:
>  			/*

Hi Laurent,

On Tue, Nov 30, 2021 at 1:29 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Mon, Nov 29, 2021 at 08:44:51PM +0900, Hirokazu Honda wrote:
> > This fixes the code accessing descriptors and
> > Camera3RequestDescriptor::pendingStreamsToProcess_ without holding
> > descriptorsMutex_ and Camera3RequestDescriptor::streamProcessMutex_
> > in CameraDevice.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/camera_device.cpp | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index f2e0bdbd..59185e7f 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -422,7 +422,11 @@ void CameraDevice::stop()
> >       worker_.stop();
> >       camera_->stop();
> >
> > -     descriptors_ = {};
> > +     {
> > +             MutexLocker descriptorsLock(descriptorsMutex_);
> > +             descriptors_ = {};
> > +     }
> > +
> >       streams_.clear();
> >
> >       state_ = State::Stopped;
> > @@ -919,6 +923,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                */
> >               FrameBuffer *frameBuffer = nullptr;
> >               int acquireFence = -1;
> > +
> > +             MutexLocker lock(descriptor->streamsProcessMutex_);
>
> I'd add a blank line here.
>
> It's annoying to take a lock here, when we know that there can be no
> concurrent access. Is there any way to tell the thread analyzer that we
> know what we're doing ? If not, I really hope std::mutex is backed by
> futexes, and very cheap to lock and unlock when there's no contention.
>

I don't think there is no annotation for that.
The workaround is to factorize this part and annotate with
NO_THREAD_SAFETY_ANALYSIS [1], or do this in Descriptor
constructor[2].
[1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-thread-safety-analysis
[2] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-checking-inside-constructors-and-destructors

-Hiro
> >               switch (cameraStream->type()) {
> >               case CameraStream::Type::Mapped:
> >                       /*
>
> --
> Regards,
>
> Laurent Pinchart