[libcamera-devel] [PATCH v5 4/4] 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>
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(),
+				   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_;
-- 
2.31.0