[libcamera-devel] [PATCH v4 6/7] android: camera_device: Protect descriptor status_ with lock

[Umang Jain]

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>
---
 src/android/camera_device.cpp | 27 +++++++++++++++++++--------
 src/android/camera_device.h   |  4 +++-
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 61b902ad..3541a74b 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -877,8 +877,6 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
 		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
 	}
 	result.output_buffers = resultBuffers.data();
-
-	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
 }
 
 bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
@@ -1058,8 +1056,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 
 	if (state_ == State::Flushing) {
 		abortRequest(descriptor.get());
+
 		{
 			MutexLocker descriptorsLock(descriptorsMutex_);
+			descriptor->status_ = Camera3RequestDescriptor::Status::Error;
 			descriptors_.push(std::move(descriptor));
 		}
 
@@ -1154,8 +1154,7 @@ void CameraDevice::requestComplete(Request *request)
 			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
 		}
 
-		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
-		sendCaptureResults();
+		completeDescriptor(descriptor, Camera3RequestDescriptor::Status::Error);
 
 		return;
 	}
@@ -1215,13 +1214,24 @@ void CameraDevice::requestComplete(Request *request)
 	}
 
 	captureResult.result = descriptor->resultMetadata_->get();
-	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();
 }
 
 void CameraDevice::sendCaptureResults()
 {
 	MutexLocker lock(descriptorsMutex_);
+
 	while (!descriptors_.empty() && !descriptors_.front()->isPending()) {
 		auto descriptor = std::move(descriptors_.front());
 		descriptors_.pop();
@@ -1262,12 +1272,13 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
 	if (cameraStream->type() == CameraStream::Type::Internal)
 		cameraStream->putBuffer(request->internalBuffer_);
 
+	Camera3RequestDescriptor::Status descriptorStatus;
 	if (status == PostProcessor::Status::Success)
-		request->status_ = Camera3RequestDescriptor::Status::Success;
+		descriptorStatus = Camera3RequestDescriptor::Status::Success;
 	else
-		request->status_ = Camera3RequestDescriptor::Status::Error;
+		descriptorStatus = Camera3RequestDescriptor::Status::Error;
 
-	sendCaptureResults();
+	completeDescriptor(request, descriptorStatus);
 }
 
 std::string CameraDevice::logPrefix() const
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 725a0618..0ce6caeb 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -126,6 +126,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();
 	std::unique_ptr<CameraMetadata> getResultMetadata(
 		const Camera3RequestDescriptor &descriptor) const;
@@ -147,7 +149,7 @@ private:
 
 	std::vector<CameraStream> streams_;
 
-	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
+	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_ and Camera3RequestDescriptor::status_. */
 	std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
 
 	std::string maker_;
-- 
2.31.1