[{"id":20339,"web_url":"https://patchwork.libcamera.org/comment/20339/","msgid":"","date":"2021-10-21T01:46:07","subject":"Re: [libcamera-devel] [PATCH v5 4/4] android: camera_device:\n\tProtect descriptor status_ with lock","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 20, 2021 at 04:12:12PM +0530, Umang Jain wrote:\n> From: Laurent Pinchart \n> \n> The Camera3RequestDescriptor::status_ is checked as a stop condition for\n> the sendCaptureResults() loop, protected by the descriptorsMutex_. The\n> status is however not set with the mutex locked, which can cause a race\n> condition with a concurrent sendCaptureResults() call (from the\n> post-processor thread for instance).\n> \n> This should be harmless in practice, as the reader thread will either\n> see the old status (Pending) and stop iterating over descriptors, or the\n> new status and continue. Still, if the Camera3RequestDescriptor state\n> machine were to change in the future, this could introduce hard to debug\n> issues. Close the race window by always setting the status with the lock\n> taken.\n> \n> Signed-off-by: Laurent Pinchart \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 29 +++++++++++++++++++----------\n> src/android/camera_device.h | 5 ++++-\n> 2 files changed, 23 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b14416ce..4e8fb2ee 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -803,8 +803,6 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> \n> \tfor (auto &buffer : descriptor->buffers_)\n> \t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> -\n> -\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> }\n> \n> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> @@ -988,7 +986,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\tdescriptors_.push(std::move(descriptor));\n> \t\t}\n> \n> -\t\tsendCaptureResults();\n> +\t\tcompleteDescriptor(descriptor.get(),\n\ndescriptor will be a nullptr here as it has been moved just above :-/\nI think one option would be to apply the following fixup.\n\ndiff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex 4e8fb2ee49f2..e876d2ce8bfa 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -803,6 +803,9 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n\n \tfor (auto &buffer : descriptor->buffers_)\n \t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n+\n+\tMutexLocker lock(descriptorsMutex_);\n+\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n }\n\n bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n@@ -986,8 +989,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\t\tdescriptors_.push(std::move(descriptor));\n \t\t}\n\n-\t\tcompleteDescriptor(descriptor.get(),\n-\t\t\t\t Camera3RequestDescriptor::Status::Error);\n+\t\tsendCaptureResults();\n\n \t\treturn 0;\n \t}\n@@ -1057,8 +1059,7 @@ void CameraDevice::requestComplete(Request *request)\n \t\t\t\t<< request->status();\n\n \t\tabortRequest(descriptor);\n-\t\tcompleteDescriptor(descriptor,\n-\t\t\t\t Camera3RequestDescriptor::Status::Error);\n+\t\tsendCaptureResults();\n\n \t\treturn;\n \t}\n@@ -1153,9 +1154,10 @@ void CameraDevice::requestComplete(Request *request)\n void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,\n \t\t\t\t Camera3RequestDescriptor::Status status)\n {\n-\tMutexLocker lock(descriptorsMutex_);\n-\tdescriptor->status_ = status;\n-\tlock.unlock();\n+\t{\n+\t\tMutexLocker lock(descriptorsMutex_);\n+\t\tdescriptor->status_ = status;\n+\t}\n\n \tsendCaptureResults();\n }\n@@ -1262,14 +1264,14 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n \t\t\thasPostProcessingErrors = true;\n \t}\n\n+\tlocker.unlock();\n+\n \tCamera3RequestDescriptor::Status descriptorStatus;\n \tif (hasPostProcessingErrors)\n \t\tdescriptorStatus = Camera3RequestDescriptor::Status::Error;\n \telse\n \t\tdescriptorStatus = Camera3RequestDescriptor::Status::Success;\n\n-\tlocker.unlock();\n-\n \tcompleteDescriptor(request, descriptorStatus);\n }\n\ndiff --git a/src/android/camera_device.h b/src/android/camera_device.h\nindex 46fb93ee777b..a85602cf178f 100644\n--- a/src/android/camera_device.h\n+++ b/src/android/camera_device.h\n@@ -124,7 +124,7 @@ private:\n \tstd::vector streams_;\n\n \t/* Protects descriptors_ and Camera3RequestDescriptor::status_. */\n-\tlibcamera::Mutex descriptorsMutex_;\n+\tmutable libcamera::Mutex descriptorsMutex_;\n \tstd::queue> descriptors_;\n\n \tstd::string maker_;\n\n\n> +\t\t\t\t Camera3RequestDescriptor::Status::Error);\n> \n> \t\treturn 0;\n> \t}\n> @@ -1058,7 +1057,8 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\t\t<< request->status();\n> \n> \t\tabortRequest(descriptor);\n> -\t\tsendCaptureResults();\n> +\t\tcompleteDescriptor(descriptor,\n> +\t\t\t\t Camera3RequestDescriptor::Status::Error);\n> \n> \t\treturn;\n> \t}\n> @@ -1135,20 +1135,28 @@ void CameraDevice::requestComplete(Request *request)\n> \n> \tif (needsPostProcessing) {\n> \t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n> -\t\t\tdescriptor->status_ = processingStatus;\n> \t\t\t/*\n> \t\t\t * \\todo This is problematic when some streams are\n> \t\t\t * queued successfully, but some fail to get queued.\n> \t\t\t * We might end up with use-after-free situation for\n> \t\t\t * descriptor in streamProcessingComplete().\n> \t\t\t */\n> -\t\t\tsendCaptureResults();\n> +\t\t\tcompleteDescriptor(descriptor, processingStatus);\n> \t\t}\n> \n> \t\treturn;\n> \t}\n> \n> -\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> +\tcompleteDescriptor(descriptor, Camera3RequestDescriptor::Status::Success);\n> +}\n> +\n> +void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,\n> +\t\t\t\t Camera3RequestDescriptor::Status status)\n> +{\n> +\tMutexLocker lock(descriptorsMutex_);\n> +\tdescriptor->status_ = status;\n> +\tlock.unlock();\n> +\n> \tsendCaptureResults();\n> }\n> \n> @@ -1254,14 +1262,15 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> \t\t\thasPostProcessingErrors = true;\n> \t}\n> \n> +\tCamera3RequestDescriptor::Status descriptorStatus;\n> \tif (hasPostProcessingErrors)\n> -\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n> +\t\tdescriptorStatus = Camera3RequestDescriptor::Status::Error;\n> \telse\n> -\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n> +\t\tdescriptorStatus = Camera3RequestDescriptor::Status::Success;\n> \n> \tlocker.unlock();\n> \n> -\tsendCaptureResults();\n> +\tcompleteDescriptor(request, descriptorStatus);\n> }\n> \n> std::string CameraDevice::logPrefix() const\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 1ef933da..46fb93ee 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -96,6 +96,8 @@ private:\n> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> \t\t\t camera3_error_msg_code code) const;\n> \tint processControls(Camera3RequestDescriptor *descriptor);\n> +\tvoid completeDescriptor(Camera3RequestDescriptor *descriptor,\n> +\t\t\t\tCamera3RequestDescriptor::Status status);\n> \tvoid sendCaptureResults();\n> \tvoid setBufferStatus(CameraStream *cameraStream,\n> \t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n> @@ -121,7 +123,8 @@ private:\n> \n> \tstd::vector streams_;\n> \n> -\tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> +\t/* Protects descriptors_ and Camera3RequestDescriptor::status_. */\n> +\tlibcamera::Mutex descriptorsMutex_;\n> \tstd::queue> descriptors_;\n> \n> \tstd::string maker_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 83B2DBF415\n\tfor ;\n\tThu, 21 Oct 2021 01:46:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1FE968F5E;\n\tThu, 21 Oct 2021 03:46:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A96B68F5A\n\tfor ;\n\tThu, 21 Oct 2021 03:46:27 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8CF6893;\n\tThu, 21 Oct 2021 03:46:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"exNTM3gd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634780786;\n\tbh=cOsMzg/HxMH3bZnSIT8F6MJPZy9aH7GCWwcGDEvU8VA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=exNTM3gdxBe44my+vNzHQZMquw/DetCU3ckplv1Aybz9WmwMnX5xj35FUwGuiXi74\n\trlNwDF4PzlJ7ft4HUdKf+ZgCj+Lako+GbOV9Sa1w4nHW6CXJxzLAVlOlOT9y3I3Vup\n\tt4V5a6mssGwWddxZKN6VrDteuYHYjhZG/g6JvqZA=","Date":"Thu, 21 Oct 2021 04:46:07 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-5-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211020104212.121743-5-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 4/4] android: camera_device:\n\tProtect descriptor status_ with lock","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20354,"web_url":"https://patchwork.libcamera.org/comment/20354/","msgid":"","date":"2021-10-21T06:16:20","subject":"Re: [libcamera-devel] [PATCH v5 4/4] android: camera_device:\n\tProtect descriptor status_ with lock","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Launent,\n\nOn 10/21/21 7:16 AM, Laurent Pinchart wrote:\n> On Wed, Oct 20, 2021 at 04:12:12PM +0530, Umang Jain wrote:\n>> From: Laurent Pinchart \n>>\n>> The Camera3RequestDescriptor::status_ is checked as a stop condition for\n>> the sendCaptureResults() loop, protected by the descriptorsMutex_. The\n>> status is however not set with the mutex locked, which can cause a race\n>> condition with a concurrent sendCaptureResults() call (from the\n>> post-processor thread for instance).\n>>\n>> This should be harmless in practice, as the reader thread will either\n>> see the old status (Pending) and stop iterating over descriptors, or the\n>> new status and continue. Still, if the Camera3RequestDescriptor state\n>> machine were to change in the future, this could introduce hard to debug\n>> issues. Close the race window by always setting the status with the lock\n>> taken.\n>>\n>> Signed-off-by: Laurent Pinchart \n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 29 +++++++++++++++++++----------\n>> src/android/camera_device.h | 5 ++++-\n>> 2 files changed, 23 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index b14416ce..4e8fb2ee 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -803,8 +803,6 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n>> \n>> \tfor (auto &buffer : descriptor->buffers_)\n>> \t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n>> -\n>> -\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>> }\n>> \n>> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n>> @@ -988,7 +986,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\t\tdescriptors_.push(std::move(descriptor));\n>> \t\t}\n>> \n>> -\t\tsendCaptureResults();\n>> +\t\tcompleteDescriptor(descriptor.get(),\n> descriptor will be a nullptr here as it has been moved just above :-/\n\n\nCan't you just save .get() pointer and use it afterwards to fix this?\n\nA similar practice has been done at the end of same function for \nCaptureRequest\n> I think one option would be to apply the following fixup.\n\n\nI don't like this option very much. It is because we are using \nsendCaptureResults() and completeDescriptor() both, as we please, to \nsend back capture results.\n\n(If you haven't noticed already,  sendCaptureResults() is now called \nonly at one place in camera_device.cpp,  i.e. completeDescriptor()). I \nwould like to keep that status-quo as is, if possible\n\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 4e8fb2ee49f2..e876d2ce8bfa 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -803,6 +803,9 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n>\n> \tfor (auto &buffer : descriptor->buffers_)\n> \t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> +\n> +\tMutexLocker lock(descriptorsMutex_);\n> +\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> }\n>\n> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> @@ -986,8 +989,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\tdescriptors_.push(std::move(descriptor));\n> \t\t}\n>\n> -\t\tcompleteDescriptor(descriptor.get(),\n> -\t\t\t\t Camera3RequestDescriptor::Status::Error);\n> +\t\tsendCaptureResults();\n>\n> \t\treturn 0;\n> \t}\n> @@ -1057,8 +1059,7 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\t\t<< request->status();\n>\n> \t\tabortRequest(descriptor);\n> -\t\tcompleteDescriptor(descriptor,\n> -\t\t\t\t Camera3RequestDescriptor::Status::Error);\n> +\t\tsendCaptureResults();\n>\n> \t\treturn;\n> \t}\n> @@ -1153,9 +1154,10 @@ void CameraDevice::requestComplete(Request *request)\n> void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,\n> \t\t\t\t Camera3RequestDescriptor::Status status)\n> {\n> -\tMutexLocker lock(descriptorsMutex_);\n> -\tdescriptor->status_ = status;\n> -\tlock.unlock();\n> +\t{\n> +\t\tMutexLocker lock(descriptorsMutex_);\n> +\t\tdescriptor->status_ = status;\n> +\t}\n>\n> \tsendCaptureResults();\n> }\n> @@ -1262,14 +1264,14 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> \t\t\thasPostProcessingErrors = true;\n> \t}\n>\n> +\tlocker.unlock();\n> +\n> \tCamera3RequestDescriptor::Status descriptorStatus;\n> \tif (hasPostProcessingErrors)\n> \t\tdescriptorStatus = Camera3RequestDescriptor::Status::Error;\n> \telse\n> \t\tdescriptorStatus = Camera3RequestDescriptor::Status::Success;\n>\n> -\tlocker.unlock();\n> -\n> \tcompleteDescriptor(request, descriptorStatus);\n> }\n>\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 46fb93ee777b..a85602cf178f 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -124,7 +124,7 @@ private:\n> \tstd::vector streams_;\n>\n> \t/* Protects descriptors_ and Camera3RequestDescriptor::status_. */\n> -\tlibcamera::Mutex descriptorsMutex_;\n> +\tmutable libcamera::Mutex descriptorsMutex_;\n> \tstd::queue> descriptors_;\n>\n> \tstd::string maker_;\n>\n>\n>> +\t\t\t\t Camera3RequestDescriptor::Status::Error);\n>> \n>> \t\treturn 0;\n>> \t}\n>> @@ -1058,7 +1057,8 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t\t\t<< request->status();\n>> \n>> \t\tabortRequest(descriptor);\n>> -\t\tsendCaptureResults();\n>> +\t\tcompleteDescriptor(descriptor,\n>> +\t\t\t\t Camera3RequestDescriptor::Status::Error);\n>> \n>> \t\treturn;\n>> \t}\n>> @@ -1135,20 +1135,28 @@ void CameraDevice::requestComplete(Request *request)\n>> \n>> \tif (needsPostProcessing) {\n>> \t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n>> -\t\t\tdescriptor->status_ = processingStatus;\n>> \t\t\t/*\n>> \t\t\t * \\todo This is problematic when some streams are\n>> \t\t\t * queued successfully, but some fail to get queued.\n>> \t\t\t * We might end up with use-after-free situation for\n>> \t\t\t * descriptor in streamProcessingComplete().\n>> \t\t\t */\n>> -\t\t\tsendCaptureResults();\n>> +\t\t\tcompleteDescriptor(descriptor, processingStatus);\n>> \t\t}\n>> \n>> \t\treturn;\n>> \t}\n>> \n>> -\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n>> +\tcompleteDescriptor(descriptor, Camera3RequestDescriptor::Status::Success);\n>> +}\n>> +\n>> +void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,\n>> +\t\t\t\t Camera3RequestDescriptor::Status status)\n>> +{\n>> +\tMutexLocker lock(descriptorsMutex_);\n>> +\tdescriptor->status_ = status;\n>> +\tlock.unlock();\n>> +\n>> \tsendCaptureResults();\n>> }\n>> \n>> @@ -1254,14 +1262,15 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>> \t\t\thasPostProcessingErrors = true;\n>> \t}\n>> \n>> +\tCamera3RequestDescriptor::Status descriptorStatus;\n>> \tif (hasPostProcessingErrors)\n>> -\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n>> +\t\tdescriptorStatus = Camera3RequestDescriptor::Status::Error;\n>> \telse\n>> -\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n>> +\t\tdescriptorStatus = Camera3RequestDescriptor::Status::Success;\n>> \n>> \tlocker.unlock();\n>> \n>> -\tsendCaptureResults();\n>> +\tcompleteDescriptor(request, descriptorStatus);\n>> }\n>> \n>> std::string CameraDevice::logPrefix() const\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 1ef933da..46fb93ee 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -96,6 +96,8 @@ private:\n>> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n>> \t\t\t camera3_error_msg_code code) const;\n>> \tint processControls(Camera3RequestDescriptor *descriptor);\n>> +\tvoid completeDescriptor(Camera3RequestDescriptor *descriptor,\n>> +\t\t\t\tCamera3RequestDescriptor::Status status);\n>> \tvoid sendCaptureResults();\n>> \tvoid setBufferStatus(CameraStream *cameraStream,\n>> \t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n>> @@ -121,7 +123,8 @@ private:\n>> \n>> \tstd::vector streams_;\n>> \n>> -\tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>> +\t/* Protects descriptors_ and Camera3RequestDescriptor::status_. */\n>> +\tlibcamera::Mutex descriptorsMutex_;\n>> \tstd::queue> descriptors_;\n>> \n>> \tstd::string maker_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id ECD3BBF415\n\tfor ;\n\tThu, 21 Oct 2021 06:16:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 423C168F5B;\n\tThu, 21 Oct 2021 08:16:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7EAE60124\n\tfor ;\n\tThu, 21 Oct 2021 08:16:24 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.185])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ED763276;\n\tThu, 21 Oct 2021 08:16:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"c4kMLStG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634796984;\n\tbh=XyNoHusiNB6xImQajdGpaJCbPV3HuHo3NYGAQKcEjZ4=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=c4kMLStGN+MVCBXjIoMnPRiEQx2971vPxpL4danRBCGgOFsSJsRnbCrRLKf7lfF1a\n\tLLyYxNnd54xs7rURaUTl5C4ThgsxA0ki+8cUnJPxSvoUzo+f8y/HPD6zKP7hpJxpV+\n\tvHf939S8+t9sCEE+FlMCvWgY/jZwDGxuP2XiR8js=","To":"Laurent Pinchart ","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-5-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Thu, 21 Oct 2021 11:46:20 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 4/4] android: camera_device:\n\tProtect descriptor status_ with lock","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20359,"web_url":"https://patchwork.libcamera.org/comment/20359/","msgid":"","date":"2021-10-21T13:31:09","subject":"Re: [libcamera-devel] [PATCH v5 4/4] android: camera_device:\n\tProtect descriptor status_ with lock","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Thu, Oct 21, 2021 at 11:46:20AM +0530, Umang Jain wrote:\n> On 10/21/21 7:16 AM, Laurent Pinchart wrote:\n> > On Wed, Oct 20, 2021 at 04:12:12PM +0530, Umang Jain wrote:\n> >> From: Laurent Pinchart \n> >>\n> >> The Camera3RequestDescriptor::status_ is checked as a stop condition for\n> >> the sendCaptureResults() loop, protected by the descriptorsMutex_. The\n> >> status is however not set with the mutex locked, which can cause a race\n> >> condition with a concurrent sendCaptureResults() call (from the\n> >> post-processor thread for instance).\n> >>\n> >> This should be harmless in practice, as the reader thread will either\n> >> see the old status (Pending) and stop iterating over descriptors, or the\n> >> new status and continue. Still, if the Camera3RequestDescriptor state\n> >> machine were to change in the future, this could introduce hard to debug\n> >> issues. Close the race window by always setting the status with the lock\n> >> taken.\n> >>\n> >> Signed-off-by: Laurent Pinchart \n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 29 +++++++++++++++++++----------\n> >> src/android/camera_device.h | 5 ++++-\n> >> 2 files changed, 23 insertions(+), 11 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index b14416ce..4e8fb2ee 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -803,8 +803,6 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> >> \n> >> \tfor (auto &buffer : descriptor->buffers_)\n> >> \t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> >> -\n> >> -\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> >> }\n> >> \n> >> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> >> @@ -988,7 +986,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> \t\t\tdescriptors_.push(std::move(descriptor));\n> >> \t\t}\n> >> \n> >> -\t\tsendCaptureResults();\n> >> +\t\tcompleteDescriptor(descriptor.get(),\n> >\n> > descriptor will be a nullptr here as it has been moved just above :-/\n> \n> Can't you just save .get() pointer and use it afterwards to fix this?\n> \n> A similar practice has been done at the end of same function for \n> CaptureRequest\n\nI've tried to avoid it, but it could be the simplest option, I agree.\nThen I think I would prefer moving the abortRequest() call just before\ncompleteDescriptor(), after adding the descriptor to the queue, to have\nthe same sequence as in CameraDevice::requestComplete().\n\n> > I think one option would be to apply the following fixup.\n> \n> I don't like this option very much. It is because we are using \n> sendCaptureResults() and completeDescriptor() both, as we please, to \n> send back capture results.\n> \n> (If you haven't noticed already,  sendCaptureResults() is now called \n> only at one place in camera_device.cpp,  i.e. completeDescriptor()). I \n> would like to keep that status-quo as is, if possible\n> \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 4e8fb2ee49f2..e876d2ce8bfa 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -803,6 +803,9 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> >\n> > \tfor (auto &buffer : descriptor->buffers_)\n> > \t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> > +\n> > +\tMutexLocker lock(descriptorsMutex_);\n> > +\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > }\n> >\n> > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> > @@ -986,8 +989,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t\tdescriptors_.push(std::move(descriptor));\n> > \t\t}\n> >\n> > -\t\tcompleteDescriptor(descriptor.get(),\n> > -\t\t\t\t Camera3RequestDescriptor::Status::Error);\n> > +\t\tsendCaptureResults();\n> >\n> > \t\treturn 0;\n> > \t}\n> > @@ -1057,8 +1059,7 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\t\t\t<< request->status();\n> >\n> > \t\tabortRequest(descriptor);\n> > -\t\tcompleteDescriptor(descriptor,\n> > -\t\t\t\t Camera3RequestDescriptor::Status::Error);\n> > +\t\tsendCaptureResults();\n> >\n> > \t\treturn;\n> > \t}\n> > @@ -1153,9 +1154,10 @@ void CameraDevice::requestComplete(Request *request)\n> > void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,\n> > \t\t\t\t Camera3RequestDescriptor::Status status)\n> > {\n> > -\tMutexLocker lock(descriptorsMutex_);\n> > -\tdescriptor->status_ = status;\n> > -\tlock.unlock();\n> > +\t{\n> > +\t\tMutexLocker lock(descriptorsMutex_);\n> > +\t\tdescriptor->status_ = status;\n> > +\t}\n> >\n> > \tsendCaptureResults();\n> > }\n> > @@ -1262,14 +1264,14 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> > \t\t\thasPostProcessingErrors = true;\n> > \t}\n> >\n> > +\tlocker.unlock();\n> > +\n> > \tCamera3RequestDescriptor::Status descriptorStatus;\n> > \tif (hasPostProcessingErrors)\n> > \t\tdescriptorStatus = Camera3RequestDescriptor::Status::Error;\n> > \telse\n> > \t\tdescriptorStatus = Camera3RequestDescriptor::Status::Success;\n> >\n> > -\tlocker.unlock();\n> > -\n> > \tcompleteDescriptor(request, descriptorStatus);\n> > }\n> >\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 46fb93ee777b..a85602cf178f 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -124,7 +124,7 @@ private:\n> > \tstd::vector streams_;\n> >\n> > \t/* Protects descriptors_ and Camera3RequestDescriptor::status_. */\n> > -\tlibcamera::Mutex descriptorsMutex_;\n> > +\tmutable libcamera::Mutex descriptorsMutex_;\n> > \tstd::queue> descriptors_;\n> >\n> > \tstd::string maker_;\n> >\n> >\n> >> +\t\t\t\t Camera3RequestDescriptor::Status::Error);\n> >> \n> >> \t\treturn 0;\n> >> \t}\n> >> @@ -1058,7 +1057,8 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t\t\t\t<< request->status();\n> >> \n> >> \t\tabortRequest(descriptor);\n> >> -\t\tsendCaptureResults();\n> >> +\t\tcompleteDescriptor(descriptor,\n> >> +\t\t\t\t Camera3RequestDescriptor::Status::Error);\n> >> \n> >> \t\treturn;\n> >> \t}\n> >> @@ -1135,20 +1135,28 @@ void CameraDevice::requestComplete(Request *request)\n> >> \n> >> \tif (needsPostProcessing) {\n> >> \t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n> >> -\t\t\tdescriptor->status_ = processingStatus;\n> >> \t\t\t/*\n> >> \t\t\t * \\todo This is problematic when some streams are\n> >> \t\t\t * queued successfully, but some fail to get queued.\n> >> \t\t\t * We might end up with use-after-free situation for\n> >> \t\t\t * descriptor in streamProcessingComplete().\n> >> \t\t\t */\n> >> -\t\t\tsendCaptureResults();\n> >> +\t\t\tcompleteDescriptor(descriptor, processingStatus);\n> >> \t\t}\n> >> \n> >> \t\treturn;\n> >> \t}\n> >> \n> >> -\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> >> +\tcompleteDescriptor(descriptor, Camera3RequestDescriptor::Status::Success);\n> >> +}\n> >> +\n> >> +void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,\n> >> +\t\t\t\t Camera3RequestDescriptor::Status status)\n> >> +{\n> >> +\tMutexLocker lock(descriptorsMutex_);\n> >> +\tdescriptor->status_ = status;\n> >> +\tlock.unlock();\n> >> +\n> >> \tsendCaptureResults();\n> >> }\n> >> \n> >> @@ -1254,14 +1262,15 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> >> \t\t\thasPostProcessingErrors = true;\n> >> \t}\n> >> \n> >> +\tCamera3RequestDescriptor::Status descriptorStatus;\n> >> \tif (hasPostProcessingErrors)\n> >> -\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n> >> +\t\tdescriptorStatus = Camera3RequestDescriptor::Status::Error;\n> >> \telse\n> >> -\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n> >> +\t\tdescriptorStatus = Camera3RequestDescriptor::Status::Success;\n> >> \n> >> \tlocker.unlock();\n> >> \n> >> -\tsendCaptureResults();\n> >> +\tcompleteDescriptor(request, descriptorStatus);\n> >> }\n> >> \n> >> std::string CameraDevice::logPrefix() const\n> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >> index 1ef933da..46fb93ee 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -96,6 +96,8 @@ private:\n> >> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> >> \t\t\t camera3_error_msg_code code) const;\n> >> \tint processControls(Camera3RequestDescriptor *descriptor);\n> >> +\tvoid completeDescriptor(Camera3RequestDescriptor *descriptor,\n> >> +\t\t\t\tCamera3RequestDescriptor::Status status);\n> >> \tvoid sendCaptureResults();\n> >> \tvoid setBufferStatus(CameraStream *cameraStream,\n> >> \t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n> >> @@ -121,7 +123,8 @@ private:\n> >> \n> >> \tstd::vector streams_;\n> >> \n> >> -\tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> >> +\t/* Protects descriptors_ and Camera3RequestDescriptor::status_. */\n> >> +\tlibcamera::Mutex descriptorsMutex_;\n> >> \tstd::queue> descriptors_;\n> >> \n> >> \tstd::string maker_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3F3C3BF415\n\tfor ;\n\tThu, 21 Oct 2021 13:31:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 83BEC68F5B;\n\tThu, 21 Oct 2021 15:31:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 35246604FE\n\tfor ;\n\tThu, 21 Oct 2021 15:31:30 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A379D8B6;\n\tThu, 21 Oct 2021 15:31:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jAMp6hx5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634823089;\n\tbh=7/38cy1eIuwunokaXhZGsxxkY617+AWMxlUp1u6hyDU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jAMp6hx5mfVsx9Xygqcws5v6m/rg0asIFjtbRX1DOCRvcVHLeP4Z3fpMc4StQ6gAz\n\tbxigYhZ8jzJ7sQLgj8smlnpRVWWYNCVWLPsz+EDxsuyXgbQPJatNtdxNz5txEP/evp\n\ts1zQIRj3YqicSTh06m29IWbctDfJLOyazc836rfY=","Date":"Thu, 21 Oct 2021 16:31:09 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-5-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 4/4] android: camera_device:\n\tProtect descriptor status_ with lock","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]