From patchwork Wed Oct 20 10:42:12 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 14192 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 72700BF415 for ; Wed, 20 Oct 2021 10:42:29 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 2CDD568F5B; Wed, 20 Oct 2021 12:42:29 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="fKAxxH5u"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 400E968F61 for ; Wed, 20 Oct 2021 12:42:27 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.251.226.185]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 0ACEB2A5; Wed, 20 Oct 2021 12:42:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1634726547; bh=8wevhsEeDoT7GH/zuB8hQFildf6Fb6V/BSAK+h27oGE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=fKAxxH5ugzGtiSUHzP1iFLnMAZp7f9E4fPD1jINyGTA/xJuMFqexCvq+GTKuZIDD1 3Skri/rRBOQaRNq13TXH5eN00/cI8qVCx9Kx3PoXlXDclWRIQwS3pK7d5fLqtnahey bwxbzdyRyE0EV4uszTVUvnGLcchAc4pLa0moauQc= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Wed, 20 Oct 2021 16:12:12 +0530 Message-Id: <20211020104212.121743-5-umang.jain@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20211020104212.121743-1-umang.jain@ideasonboard.com> References: <20211020104212.121743-1-umang.jain@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v5 4/4] android: camera_device: Protect descriptor status_ with lock X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" From: Laurent Pinchart 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 Signed-off-by: Umang Jain --- 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 streams_; - libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ + /* Protects descriptors_ and Camera3RequestDescriptor::status_. */ + libcamera::Mutex descriptorsMutex_; std::queue> descriptors_; std::string maker_;