[libcamera-devel] [PATCH v3 2/4] libcamera: ipa: raspberrypi: ALSC: Improve locking in a few places

[Kieran Bingham]

Hi David,

I believe this patch has introduced the following coverity warning. If
you supply a patch to fix this issue, please add the following tag:

Reported-by: Coverity CID=298183

Alternatively, if you believe it's a false positive, let me know and
I'll close it.

** CID 298183:  Concurrent data access violations  (MISSING_LOCK)
/home/linuxembedded/iob/libcamera/libcamera-daily/src/ipa/raspberrypi/controller/rpi/alsc.cpp:
369 in RPi::Alsc::asyncFunc()()


________________________________________________________________________________________________________
*** CID 298183:  Concurrent data access violations  (MISSING_LOCK)
/home/linuxembedded/iob/libcamera/libcamera-daily/src/ipa/raspberrypi/controller/rpi/alsc.cpp:
369 in RPi::Alsc::asyncFunc()()
363     	while (true) {
364     		{
365     			std::unique_lock<std::mutex> lock(mutex_);
366     			async_signal_.wait(lock, [&] {
367     				return async_start_ || async_abort_;
368     			});
>>>     CID 298183:  Concurrent data access violations  (MISSING_LOCK)
>>>     Accessing "this->async_start_" without holding lock
"RPi::Alsc.mutex_". Elsewhere, "_ZN3RPi4AlscE.async_start_" is accessed
with "RPi::Alsc.mutex_" held 1 out of 2 times (1 of these accesses
strongly imply that it is necessary).
369     			async_start_ = false;
370     			if (async_abort_)
371     				break;
372     		}
373     		doAlsc();
374     		{



Briefly looking, I think that the call to async_signal_.wait(lock...)
means that the lock is held for the remainder of the scope, so in fact
this is likely a false positive. Is that interpretation accurate?


--
Regards

Kieran


On 01/08/2020 09:01, David Plowman wrote:
> Fix up a few locations where we call notify_one() with the lock
> held. In particular, restartAsync does not need to be called with the
> lock held for its entire duration.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/alsc.cpp | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> index 76e2f04..12359dc 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> @@ -32,8 +32,8 @@ Alsc::~Alsc()
>  	{
>  		std::lock_guard<std::mutex> lock(mutex_);
>  		async_abort_ = true;
> -		async_signal_.notify_one();
>  	}
> +	async_signal_.notify_one();
>  	async_thread_.join();
>  }
>  
> @@ -268,8 +268,11 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)
>  	frame_phase_ = 0;
>  	// copy the camera mode so it won't change during the calculations
>  	async_camera_mode_ = camera_mode_;
> -	async_start_ = true;
>  	async_started_ = true;
> +	{
> +		std::lock_guard<std::mutex> lock(mutex_);
> +		async_start_ = true;
> +	}
>  	async_signal_.notify_one();
>  }
>  
> @@ -315,7 +318,6 @@ void Alsc::Process(StatisticsPtr &stats, Metadata *image_metadata)
>  	RPI_LOG("Alsc: frame_phase " << frame_phase_);
>  	if (frame_phase_ >= (int)config_.frame_period ||
>  	    frame_count2_ < (int)config_.startup_frames) {
> -		std::unique_lock<std::mutex> lock(mutex_);
>  		if (async_started_ == false) {
>  			RPI_LOG("ALSC thread starting");
>  			restartAsync(stats, image_metadata);
> @@ -339,8 +341,8 @@ void Alsc::asyncFunc()
>  		{
>  			std::lock_guard<std::mutex> lock(mutex_);
>  			async_finished_ = true;
> -			sync_signal_.notify_one();
>  		}
> +		sync_signal_.notify_one();
>  	}
>  }
>  
> 

-- 
Regards
--
Kieran