[libcamera-devel] [PATCH v5 4/4] android: camera_device: Protect descriptor status_ with lock

[Laurent Pinchart]

On Wed, Oct 20, 2021 at 04:12:12PM +0530, Umang Jain wrote:
> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> The Camera3RequestDescriptor::status_ is checked as a stop condition for
> the sendCaptureResults() loop, protected by the descriptorsMutex_. The
> status is however not set with the mutex locked, which can cause a race
> condition with a concurrent sendCaptureResults() call (from the
> post-processor thread for instance).
> 
> This should be harmless in practice, as the reader thread will either
> see the old status (Pending) and stop iterating over descriptors, or the
> new status and continue. Still, if the Camera3RequestDescriptor state
> machine were to change in the future, this could introduce hard to debug
> issues. Close the race window by always setting the status with the lock
> taken.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 29 +++++++++++++++++++----------
>  src/android/camera_device.h   |  5 ++++-
>  2 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b14416ce..4e8fb2ee 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -803,8 +803,6 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
>  
>  	for (auto &buffer : descriptor->buffers_)
>  		buffer.status = Camera3RequestDescriptor::Status::Error;
> -
> -	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>  }
>  
>  bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> @@ -988,7 +986,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			descriptors_.push(std::move(descriptor));
>  		}
>  
> -		sendCaptureResults();
> +		completeDescriptor(descriptor.get(),

descriptor will be a nullptr here as it has been moved just above :-/
I think one option would be to apply the following fixup.

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 4e8fb2ee49f2..e876d2ce8bfa 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -803,6 +803,9 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const

 	for (auto &buffer : descriptor->buffers_)
 		buffer.status = Camera3RequestDescriptor::Status::Error;
+
+	MutexLocker lock(descriptorsMutex_);
+	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
 }

 bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
@@ -986,8 +989,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 			descriptors_.push(std::move(descriptor));
 		}

-		completeDescriptor(descriptor.get(),
-				   Camera3RequestDescriptor::Status::Error);
+		sendCaptureResults();

 		return 0;
 	}
@@ -1057,8 +1059,7 @@ void CameraDevice::requestComplete(Request *request)
 				<< request->status();

 		abortRequest(descriptor);
-		completeDescriptor(descriptor,
-				   Camera3RequestDescriptor::Status::Error);
+		sendCaptureResults();

 		return;
 	}
@@ -1153,9 +1154,10 @@ void CameraDevice::requestComplete(Request *request)
 void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,
 				      Camera3RequestDescriptor::Status status)
 {
-	MutexLocker lock(descriptorsMutex_);
-	descriptor->status_ = status;
-	lock.unlock();
+	{
+		MutexLocker lock(descriptorsMutex_);
+		descriptor->status_ = status;
+	}

 	sendCaptureResults();
 }
@@ -1262,14 +1264,14 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
 			hasPostProcessingErrors = true;
 	}

+	locker.unlock();
+
 	Camera3RequestDescriptor::Status descriptorStatus;
 	if (hasPostProcessingErrors)
 		descriptorStatus = Camera3RequestDescriptor::Status::Error;
 	else
 		descriptorStatus = Camera3RequestDescriptor::Status::Success;

-	locker.unlock();
-
 	completeDescriptor(request, descriptorStatus);
 }

diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 46fb93ee777b..a85602cf178f 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -124,7 +124,7 @@ private:
 	std::vector<CameraStream> streams_;

 	/* Protects descriptors_ and Camera3RequestDescriptor::status_. */
-	libcamera::Mutex descriptorsMutex_;
+	mutable libcamera::Mutex descriptorsMutex_;
 	std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;

 	std::string maker_;


> +				   Camera3RequestDescriptor::Status::Error);
>  
>  		return 0;
>  	}
> @@ -1058,7 +1057,8 @@ void CameraDevice::requestComplete(Request *request)
>  				<< request->status();
>  
>  		abortRequest(descriptor);
> -		sendCaptureResults();
> +		completeDescriptor(descriptor,
> +				   Camera3RequestDescriptor::Status::Error);
>  
>  		return;
>  	}
> @@ -1135,20 +1135,28 @@ void CameraDevice::requestComplete(Request *request)
>  
>  	if (needsPostProcessing) {
>  		if (processingStatus == Camera3RequestDescriptor::Status::Error) {
> -			descriptor->status_ = processingStatus;
>  			/*
>  			 * \todo This is problematic when some streams are
>  			 * queued successfully, but some fail to get queued.
>  			 * We might end up with use-after-free situation for
>  			 * descriptor in streamProcessingComplete().
>  			 */
> -			sendCaptureResults();
> +			completeDescriptor(descriptor, processingStatus);
>  		}
>  
>  		return;
>  	}
>  
> -	descriptor->status_ = Camera3RequestDescriptor::Status::Success;
> +	completeDescriptor(descriptor, Camera3RequestDescriptor::Status::Success);
> +}
> +
> +void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,
> +				      Camera3RequestDescriptor::Status status)
> +{
> +	MutexLocker lock(descriptorsMutex_);
> +	descriptor->status_ = status;
> +	lock.unlock();
> +
>  	sendCaptureResults();
>  }
>  
> @@ -1254,14 +1262,15 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
>  			hasPostProcessingErrors = true;
>  	}
>  
> +	Camera3RequestDescriptor::Status descriptorStatus;
>  	if (hasPostProcessingErrors)
> -		request->status_ = Camera3RequestDescriptor::Status::Error;
> +		descriptorStatus = Camera3RequestDescriptor::Status::Error;
>  	else
> -		request->status_ = Camera3RequestDescriptor::Status::Success;
> +		descriptorStatus = Camera3RequestDescriptor::Status::Success;
>  
>  	locker.unlock();
>  
> -	sendCaptureResults();
> +	completeDescriptor(request, descriptorStatus);
>  }
>  
>  std::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 1ef933da..46fb93ee 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -96,6 +96,8 @@ private:
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>  			 camera3_error_msg_code code) const;
>  	int processControls(Camera3RequestDescriptor *descriptor);
> +	void completeDescriptor(Camera3RequestDescriptor *descriptor,
> +				Camera3RequestDescriptor::Status status);
>  	void sendCaptureResults();
>  	void setBufferStatus(CameraStream *cameraStream,
>  			     Camera3RequestDescriptor::StreamBuffer &buffer,
> @@ -121,7 +123,8 @@ private:
>  
>  	std::vector<CameraStream> streams_;
>  
> -	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> +	/* Protects descriptors_ and Camera3RequestDescriptor::status_. */
> +	libcamera::Mutex descriptorsMutex_;
>  	std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
>  
>  	std::string maker_;

-- 
Regards,

Laurent Pinchart