[{"id":19829,"web_url":"https://patchwork.libcamera.org/comment/19829/","msgid":"","date":"2021-09-24T07:40:12","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for the patch.\n\nOn Fri, Sep 24, 2021 at 3:44 PM Umang Jain wrote:\n>\n> The Camera3RequestDescriptor containing the capture request\n> is adding to the descriptors_ map after a call to\n> CameraWorker::queueRequest(). This is a race condition since\n> CameraWorker::queueRequest() queues request to libcamera::Camera\n> asynchronously and the addition of the descriptor to the map\n> occurs with std::move(). Hence, it might happen that the async\n> queueRequest() hasn't finished but the descriptor gets std::move()ed.\n>\n> Fix it by adding the descriptor to map first, before\n> CameraWorker::queueRequest().\n\n\nI don't understand the problem well.\nI think the pointer of descriptors.request_.get() is not invalidated\nby std::move().\n\n>\n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 21844e51..c4c03d86 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> state_ = State::Running;\n> }\n>\n> - worker_.queueRequest(descriptor.request_.get());\n> -\n> + unsigned long requestCookie = descriptor.request_->cookie();\n> {\n> MutexLocker descriptorsLock(descriptorsMutex_);\n> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> + descriptors_[requestCookie] = std::move(descriptor);\n> }\n>\n> + worker_.queueRequest(descriptors_[requestCookie].request_.get());\n\nAccessing descriptors_ must be while holding descriptorsMutex_.\n\n-Hiro\n> +\n> return 0;\n> }\n>\n> --\n> 2.31.1\n>","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 07A4DBDC71\n\tfor ;\n\tFri, 24 Sep 2021 07:40:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 413206918C;\n\tFri, 24 Sep 2021 09:40:26 +0200 (CEST)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C947869186\n\tfor ;\n\tFri, 24 Sep 2021 09:40:24 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id dj4so32769320edb.5\n\tfor ;\n\tFri, 24 Sep 2021 00:40:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"L9ux+47+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=zTTY5CpYfgH6acSyhxB7V9ultQnlUPghb1U99HYQwH8=;\n\tb=L9ux+47+RhWcEfqJci/r7PDYCPE7qspZE/GVvZkG2VpmSFz8JDsZMUqdYKrdBtB4Ws\n\tcFM1mAk4nxQ6sAIBZqhWXn+VtS4iXB4hTahOVwfyUfs6uwJCgQV5DT6pTdfnpjrWHNKP\n\t5hAbR40Z/SIii0oZ5dTCk5nbNYODWSxJNJ3II=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=zTTY5CpYfgH6acSyhxB7V9ultQnlUPghb1U99HYQwH8=;\n\tb=rGaqCMT893xuaNmVlsFLfWwE2ehSb5C3pQ8pgPsTV5+LBzlTePtSD5aV84yoMNa+T8\n\thXkzmd+b0oor2KlCxi5VlJo0oDcjeuRq9tnYTGIepSRupzerTS1JcFPS2of6qULLopoJ\n\tRt1hVvbTDa2I5wUy4VZX3QQgMXQJlE+zwOOqppj/XeG2wpQzuF5YksVKzzAO+5k9ZfcJ\n\tRTRVT+g/+2ixoMbg2ECAIdb9IzYGes7EfQ6oMReSIL10ZEdaTHu/CQ/JvxuFB6pOOxxt\n\taTU4DKgSPTpNnBMhyKJDW+9cjm1Sea8fu6r4LLcIbxOzmr+dgSA6ABh7ZTCD0COh6hhi\n\tbWQA==","X-Gm-Message-State":"AOAM532xEot1RrQlblE15rKkc3fnP10NPdI3D315qzeQox5fhUle1nm5\n\tB60vouUV1I2khgfAMLaLuaXs09sOmxIO3T5VDGlcYa5wfgkTVA==","X-Google-Smtp-Source":"ABdhPJw7vwFazBT0y2egkcKMslS34Q3bR1NKWpXJKGg3YyKqA/rAWnslvhYpqZAX+WINUiiKx/sKmzD+MQkRxWMqTUg=","X-Received":"by 2002:aa7:d402:: with SMTP id z2mr3465157edq.291.1632469224386;\n\tFri, 24 Sep 2021 00:40:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20210924064409.317140-1-umang.jain@ideasonboard.com>","In-Reply-To":"<20210924064409.317140-1-umang.jain@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Fri, 24 Sep 2021 16:40:12 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19831,"web_url":"https://patchwork.libcamera.org/comment/19831/","msgid":"","date":"2021-09-24T08:19:19","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 9/24/21 1:10 PM, Hirokazu Honda wrote:\n> Hi Umang, thank you for the patch.\n>\n> On Fri, Sep 24, 2021 at 3:44 PM Umang Jain wrote:\n>> The Camera3RequestDescriptor containing the capture request\n>> is adding to the descriptors_ map after a call to\n>> CameraWorker::queueRequest(). This is a race condition since\n>> CameraWorker::queueRequest() queues request to libcamera::Camera\n>> asynchronously and the addition of the descriptor to the map\n>> occurs with std::move(). Hence, it might happen that the async\n>> queueRequest() hasn't finished but the descriptor gets std::move()ed.\n>>\n>> Fix it by adding the descriptor to map first, before\n>> CameraWorker::queueRequest().\n>\n> I don't understand the problem well.\n\n\nIt's a race between queueRequest() and adding to the descriptor_ std::map.\n\nqueueRequest() is called asynchronously(separate thread)  so it might \nhappen that queueRequest is still processing/queuing the request while \nwe std::move()ed the descriptor (Which wraps the request queueRequest is \naccessing)\n\nIf we fix the order with correct data-access it fixes the issue.\n\n> I think the pointer of descriptors.request_.get() is not invalidated\n> by std::move().\n\n\nWhy do you think so? :)\n\n     diff --git a/src/android/camera_device.cpp \nb/src/android/camera_device.cpp\n     index a693dcbe..0de7050d 100644\n     --- a/src/android/camera_device.cpp\n     +++ b/src/android/camera_device.cpp\n     @@ -1063,13 +1063,13 @@ int \nCameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n                     state_ = State::Running;\n             }\n\n     - worker_.queueRequest(descriptor.request_.get());\n     -\n             {\n                     MutexLocker descriptorsLock(descriptorsMutex_);\ndescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n             }\n\n     + worker_.queueRequest(descriptor.request_.get());\n     +\n\nis a segfaulted here (and expected no?).\n\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 7 ++++---\n>> 1 file changed, 4 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 21844e51..c4c03d86 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> state_ = State::Running;\n>> }\n>>\n>> - worker_.queueRequest(descriptor.request_.get());\n>> -\n>> + unsigned long requestCookie = descriptor.request_->cookie();\n>> {\n>> MutexLocker descriptorsLock(descriptorsMutex_);\n>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>> + descriptors_[requestCookie] = std::move(descriptor);\n>> }\n>>\n>> + worker_.queueRequest(descriptors_[requestCookie].request_.get());\n> Accessing descriptors_ must be while holding descriptorsMutex_.\n\n\nI am in two minds here,\n\nWe can lock it yes, but we are just reading from the map so, should be \nfine?\n\n>\n> -Hiro\n>> +\n>> return 0;\n>> }\n>>\n>> --\n>> 2.31.1\n>>","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 F0014BDC71\n\tfor ;\n\tFri, 24 Sep 2021 08:19:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 611FF6918C;\n\tFri, 24 Sep 2021 10:19:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7E16D69186\n\tfor ;\n\tFri, 24 Sep 2021 10:19:24 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.86])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 83AEF45E;\n\tFri, 24 Sep 2021 10:19:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kiVKCx5u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632471564;\n\tbh=djp2CrBoF6YOcgVzFs0IMiBHP0P5Ej5V3chXLzfVfCc=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=kiVKCx5uqsfOk5jsqfEVjz9S4/4xVQ1EriHngmU47I6Y1gu1hO7Wury4DYzv2CrAg\n\to79ft/j+zpEvn8qOFPz7+hS8sILL6OirUJEiVbjHnuE12Ej2xmC7EXeRBv+Ltrbql7\n\tkSy93qVblqW3aTajyB+vmlk+SnycXnKWhumbg/3A=","To":"Hirokazu Honda ","References":"<20210924064409.317140-1-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Fri, 24 Sep 2021 13:49:19 +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] android: camera_device: Fix race on\n\tqueuing capture request","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19839,"web_url":"https://patchwork.libcamera.org/comment/19839/","msgid":"","date":"2021-09-24T11:37:27","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Fri, Sep 24, 2021 at 5:19 PM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> On 9/24/21 1:10 PM, Hirokazu Honda wrote:\n> > Hi Umang, thank you for the patch.\n> >\n> > On Fri, Sep 24, 2021 at 3:44 PM Umang Jain wrote:\n> >> The Camera3RequestDescriptor containing the capture request\n> >> is adding to the descriptors_ map after a call to\n> >> CameraWorker::queueRequest(). This is a race condition since\n> >> CameraWorker::queueRequest() queues request to libcamera::Camera\n> >> asynchronously and the addition of the descriptor to the map\n> >> occurs with std::move(). Hence, it might happen that the async\n> >> queueRequest() hasn't finished but the descriptor gets std::move()ed.\n> >>\n> >> Fix it by adding the descriptor to map first, before\n> >> CameraWorker::queueRequest().\n> >\n> > I don't understand the problem well.\n>\n>\n> It's a race between queueRequest() and adding to the descriptor_ std::map.\n>\n> queueRequest() is called asynchronously(separate thread) so it might\n> happen that queueRequest is still processing/queuing the request while\n> we std::move()ed the descriptor (Which wraps the request queueRequest is\n> accessing)\n>\n> If we fix the order with correct data-access it fixes the issue.\n>\n> > I think the pointer of descriptors.request_.get() is not invalidated\n> > by std::move().\n>\n>\n> Why do you think so? :)\n>\n\nstd::move() invalidates descriptor. Yes, descriptor.request_ becomes\ninvalidated after std::move(). descriptor must not be accessed after\nstd::move().\nHowever, queueRequest holds a pointer value,\ndescriptor.request_.get(). descriptor is move()d to descriptors_ and\nthus descriptor.request_ is alive until descriptor is removed in\ndescriptors_.\nSo the passed pointer should still be valid after executing std::move().\n\n> diff --git a/src/android/camera_device.cpp\n> b/src/android/camera_device.cpp\n> index a693dcbe..0de7050d 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1063,13 +1063,13 @@ int\n> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> state_ = State::Running;\n> }\n>\n> - worker_.queueRequest(descriptor.request_.get());\n> -\n> {\n> MutexLocker descriptorsLock(descriptorsMutex_);\n> descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> }\n>\n> + worker_.queueRequest(descriptor.request_.get());\n> +\n>\n> is a segfaulted here (and expected no?).\n>\n> >\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 7 ++++---\n> >> 1 file changed, 4 insertions(+), 3 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 21844e51..c4c03d86 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> state_ = State::Running;\n> >> }\n> >>\n> >> - worker_.queueRequest(descriptor.request_.get());\n> >> -\n> >> + unsigned long requestCookie = descriptor.request_->cookie();\n> >> {\n> >> MutexLocker descriptorsLock(descriptorsMutex_);\n> >> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> >> + descriptors_[requestCookie] = std::move(descriptor);\n> >> }\n> >>\n> >> + worker_.queueRequest(descriptors_[requestCookie].request_.get());\n> > Accessing descriptors_ must be while holding descriptorsMutex_.\n>\n>\n> I am in two minds here,\n>\n> We can lock it yes, but we are just reading from the map so, should be\n> fine?\n\nRegardless of reading/writing, descriptorMutex_ must be acquired\nwhenever accessing descriptors_.\nSome other thread can clear descriptors in parallel. Parallel\nexecuting operations against std::map corrupts it.\nBy the way, std::map::operator[] is not a constant function. In fact,\nit can create a new node if a key does not exist.\n\n-Hiro\n>\n> >\n> > -Hiro\n> >> +\n> >> return 0;\n> >> }\n> >>\n> >> --\n> >> 2.31.1\n> >>","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 83AA3BDC71\n\tfor ;\n\tFri, 24 Sep 2021 11:37:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 993086918C;\n\tFri, 24 Sep 2021 13:37:39 +0200 (CEST)","from mail-ed1-x532.google.com (mail-ed1-x532.google.com\n\t[IPv6:2a00:1450:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6E3D5687DD\n\tfor ;\n\tFri, 24 Sep 2021 13:37:38 +0200 (CEST)","by mail-ed1-x532.google.com with SMTP id v24so34946087eda.3\n\tfor ;\n\tFri, 24 Sep 2021 04:37:38 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Iw+9PZNK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=TY+TE/W3cK985LZLEYXtX8ECXfKRU/Fr6S6ULy9KN6U=;\n\tb=Iw+9PZNKNVrviW5bFAYTE0iCOMBS7OdDu/KuELw7cNEu8pRwLnpElgY7VJd8Q0Qs31\n\tjNyHzWt6eTcDiF3Yzu+Xroh1U6TsbpXuOzrYCX3tA1AUJIdSsZIVg4qBlfeZ+KmloOFb\n\tDbaTEuEznL/18DG4jQEVDQ6qwx9k3bJyDJfFE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=TY+TE/W3cK985LZLEYXtX8ECXfKRU/Fr6S6ULy9KN6U=;\n\tb=JJpVQ/f/bYnna6R3VrQLpvaK+cCkgdKehW5G1F/URko6nY1oZRMW7QlUL/kAUpP4/w\n\t79bLLDtjIzjHXz6aCqpQN6tK4LFePlzXoAaIt6CGcE51+pnW90LORIWKxU6aIAy2oBjP\n\teyXGVT722fLu5w3jbsS5lgUKC7glB/IHLqpl2W0PYooqLuCWaBeFDub3f9v1FusL/bnp\n\tYH2hjLOkb3bdl9IuqZ1iMKznT2utHgZll6RCGXv8y3hNOsZVI1McW9YOg8MnRFg/BtAj\n\tiCypIi9sZXE2dbMQoEC0H/BtuMjMlG3Bkn86O1e1oWLGf71kyvDY0cJesb+Hbg725WG9\n\t4lVQ==","X-Gm-Message-State":"AOAM530JhjTwzZy/NH0phD5r2Pc73ynR2VEf292MrO8h2WVSA4LhG5qV\n\tz9803p4FWIwyOEIoOA8yopUbwS3yF/izXF4MMhzc+MUjzIY=","X-Google-Smtp-Source":"ABdhPJzNPSekGU3gmn+YV5PIcXAo7Zmx2b1oK3wJtGpmEd0ENeO+3YU9/sGE1CbN+W9MZWpjfxzzhPcUXpQte9bB9mg=","X-Received":"by 2002:a17:906:32c9:: with SMTP id\n\tk9mr10934421ejk.218.1632483457961; \n\tFri, 24 Sep 2021 04:37:37 -0700 (PDT)","MIME-Version":"1.0","References":"<20210924064409.317140-1-umang.jain@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 24 Sep 2021 20:37:27 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19841,"web_url":"https://patchwork.libcamera.org/comment/19841/","msgid":"<06440643-b45d-05ae-984b-e65364ae0fd7@ideasonboard.com>","date":"2021-09-24T12:39:04","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 9/24/21 5:07 PM, Hirokazu Honda wrote:\n> Hi Umang,\n>\n> On Fri, Sep 24, 2021 at 5:19 PM Umang Jain wrote:\n>> Hi Hiro,\n>>\n>> On 9/24/21 1:10 PM, Hirokazu Honda wrote:\n>>> Hi Umang, thank you for the patch.\n>>>\n>>> On Fri, Sep 24, 2021 at 3:44 PM Umang Jain wrote:\n>>>> The Camera3RequestDescriptor containing the capture request\n>>>> is adding to the descriptors_ map after a call to\n>>>> CameraWorker::queueRequest(). This is a race condition since\n>>>> CameraWorker::queueRequest() queues request to libcamera::Camera\n>>>> asynchronously and the addition of the descriptor to the map\n>>>> occurs with std::move(). Hence, it might happen that the async\n>>>> queueRequest() hasn't finished but the descriptor gets std::move()ed.\n>>>>\n>>>> Fix it by adding the descriptor to map first, before\n>>>> CameraWorker::queueRequest().\n>>> I don't understand the problem well.\n>>\n>> It's a race between queueRequest() and adding to the descriptor_ std::map.\n>>\n>> queueRequest() is called asynchronously(separate thread) so it might\n>> happen that queueRequest is still processing/queuing the request while\n>> we std::move()ed the descriptor (Which wraps the request queueRequest is\n>> accessing)\n>>\n>> If we fix the order with correct data-access it fixes the issue.\n>>\n>>> I think the pointer of descriptors.request_.get() is not invalidated\n>>> by std::move().\n>>\n>> Why do you think so? :)\n>>\n> std::move() invalidates descriptor. Yes, descriptor.request_ becomes\n> invalidated after std::move(). descriptor must not be accessed after\n> std::move().\n> However, queueRequest holds a pointer value,\n> descriptor.request_.get(). descriptor is move()d to descriptors_ and\n> thus descriptor.request_ is alive until descriptor is removed in\n> descriptors_.\n> So the passed pointer should still be valid after executing std::move().\n\n\nGenerally, in other programming models, I have often seen data that has \nbeen move()d should be now/further accessed(if necessary)  from that \ncontainer that they have been move()d into. It clearly differentiates \nbetween the ownership model (before and after) and I find it as a good \nmental model to have (helps especially when there is a long function \nimplementation )\n\nYes, we can store a pointer before the move and call queueRequest() with \nit after the move. Either of the methods, I am not against any one.\n\n>\n>> diff --git a/src/android/camera_device.cpp\n>> b/src/android/camera_device.cpp\n>> index a693dcbe..0de7050d 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1063,13 +1063,13 @@ int\n>> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> state_ = State::Running;\n>> }\n>>\n>> - worker_.queueRequest(descriptor.request_.get());\n>> -\n>> {\n>> MutexLocker descriptorsLock(descriptorsMutex_);\n>> descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>> }\n>>\n>> + worker_.queueRequest(descriptor.request_.get());\n>> +\n>>\n>> is a segfaulted here (and expected no?).\n>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> src/android/camera_device.cpp | 7 ++++---\n>>>> 1 file changed, 4 insertions(+), 3 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 21844e51..c4c03d86 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> state_ = State::Running;\n>>>> }\n>>>>\n>>>> - worker_.queueRequest(descriptor.request_.get());\n>>>> -\n>>>> + unsigned long requestCookie = descriptor.request_->cookie();\n>>>> {\n>>>> MutexLocker descriptorsLock(descriptorsMutex_);\n>>>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>>>> + descriptors_[requestCookie] = std::move(descriptor);\n>>>> }\n>>>>\n>>>> + worker_.queueRequest(descriptors_[requestCookie].request_.get());\n>>> Accessing descriptors_ must be while holding descriptorsMutex_.\n>>\n>> I am in two minds here,\n>>\n>> We can lock it yes, but we are just reading from the map so, should be\n>> fine?\n> Regardless of reading/writing, descriptorMutex_ must be acquired\n> whenever accessing descriptors_.\n> Some other thread can clear descriptors in parallel. Parallel\n\n\nNot sure, if this a possible yet, but sure!\n\nThanks for the inputs.\n\n> executing operations against std::map corrupts it.\n> By the way, std::map::operator[] is not a constant function. In fact,\n> it can create a new node if a key does not exist.\n>\n> -Hiro\n>>> -Hiro\n>>>> +\n>>>> return 0;\n>>>> }\n>>>>\n>>>> --\n>>>> 2.31.1\n>>>>","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 8FE90BDC71\n\tfor ;\n\tFri, 24 Sep 2021 12:39:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6E0E6918C;\n\tFri, 24 Sep 2021 14:39:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 281B8687DD\n\tfor ;\n\tFri, 24 Sep 2021 14:39:09 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2FAAC45E;\n\tFri, 24 Sep 2021 14:39:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Bh3nTb1H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632487148;\n\tbh=ZQV17OiF4ukPGl09HgQzMsXMvhoAivs1GheB2BBI3Tw=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Bh3nTb1HxceMSsdAA2nE0LpGhvA4qpENAwdU6dHHU0iMmymu8ZOjP+73tGHRZrysB\n\tgxRGVzu1m4U2/VHFuIDsGM0tixssBm/qqQa3vFlz7YukphHIWUinhKwqyF05SFuldY\n\tGlPdgUkRHjvRLzZVfYbYRqyHIFgoz046tjuDJGpk=","To":"Hirokazu Honda ","References":"<20210924064409.317140-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t","From":"Umang Jain ","Message-ID":"<06440643-b45d-05ae-984b-e65364ae0fd7@ideasonboard.com>","Date":"Fri, 24 Sep 2021 18:09:04 +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] android: camera_device: Fix race on\n\tqueuing capture request","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19842,"web_url":"https://patchwork.libcamera.org/comment/19842/","msgid":"","date":"2021-09-24T12:57:42","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Fri, Sep 24, 2021 at 9:39 PM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> On 9/24/21 5:07 PM, Hirokazu Honda wrote:\n> > Hi Umang,\n> >\n> > On Fri, Sep 24, 2021 at 5:19 PM Umang Jain wrote:\n> >> Hi Hiro,\n> >>\n> >> On 9/24/21 1:10 PM, Hirokazu Honda wrote:\n> >>> Hi Umang, thank you for the patch.\n> >>>\n> >>> On Fri, Sep 24, 2021 at 3:44 PM Umang Jain wrote:\n> >>>> The Camera3RequestDescriptor containing the capture request\n> >>>> is adding to the descriptors_ map after a call to\n> >>>> CameraWorker::queueRequest(). This is a race condition since\n> >>>> CameraWorker::queueRequest() queues request to libcamera::Camera\n> >>>> asynchronously and the addition of the descriptor to the map\n> >>>> occurs with std::move(). Hence, it might happen that the async\n> >>>> queueRequest() hasn't finished but the descriptor gets std::move()ed.\n> >>>>\n> >>>> Fix it by adding the descriptor to map first, before\n> >>>> CameraWorker::queueRequest().\n> >>> I don't understand the problem well.\n> >>\n> >> It's a race between queueRequest() and adding to the descriptor_ std::map.\n> >>\n> >> queueRequest() is called asynchronously(separate thread) so it might\n> >> happen that queueRequest is still processing/queuing the request while\n> >> we std::move()ed the descriptor (Which wraps the request queueRequest is\n> >> accessing)\n> >>\n> >> If we fix the order with correct data-access it fixes the issue.\n> >>\n> >>> I think the pointer of descriptors.request_.get() is not invalidated\n> >>> by std::move().\n> >>\n> >> Why do you think so? :)\n> >>\n> > std::move() invalidates descriptor. Yes, descriptor.request_ becomes\n> > invalidated after std::move(). descriptor must not be accessed after\n> > std::move().\n> > However, queueRequest holds a pointer value,\n> > descriptor.request_.get(). descriptor is move()d to descriptors_ and\n> > thus descriptor.request_ is alive until descriptor is removed in\n> > descriptors_.\n> > So the passed pointer should still be valid after executing std::move().\n>\n>\n> Generally, in other programming models, I have often seen data that has\n> been move()d should be now/further accessed(if necessary) from that\n> container that they have been move()d into. It clearly differentiates\n> between the ownership model (before and after) and I find it as a good\n> mental model to have (helps especially when there is a long function\n> implementation )\n>\n> Yes, we can store a pointer before the move and call queueRequest() with\n> it after the move. Either of the methods, I am not against any one.\n>\n\nSince copying a pointer value is before std::move(), it seems to be\nnatural for me to use descriptor.request_.get(), which is the original\ncode.\nHmm, I don't understand why the original code doesn't work. I guess\nthe seg fault is due to other reason.\n\n-Hiro\n> >\n> >> diff --git a/src/android/camera_device.cpp\n> >> b/src/android/camera_device.cpp\n> >> index a693dcbe..0de7050d 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -1063,13 +1063,13 @@ int\n> >> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> state_ = State::Running;\n> >> }\n> >>\n> >> - worker_.queueRequest(descriptor.request_.get());\n> >> -\n> >> {\n> >> MutexLocker descriptorsLock(descriptorsMutex_);\n> >> descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> >> }\n> >>\n> >> + worker_.queueRequest(descriptor.request_.get());\n> >> +\n> >>\n> >> is a segfaulted here (and expected no?).\n> >>\n> >>>> Signed-off-by: Umang Jain \n> >>>> ---\n> >>>> src/android/camera_device.cpp | 7 ++++---\n> >>>> 1 file changed, 4 insertions(+), 3 deletions(-)\n> >>>>\n> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>> index 21844e51..c4c03d86 100644\n> >>>> --- a/src/android/camera_device.cpp\n> >>>> +++ b/src/android/camera_device.cpp\n> >>>> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>> state_ = State::Running;\n> >>>> }\n> >>>>\n> >>>> - worker_.queueRequest(descriptor.request_.get());\n> >>>> -\n> >>>> + unsigned long requestCookie = descriptor.request_->cookie();\n> >>>> {\n> >>>> MutexLocker descriptorsLock(descriptorsMutex_);\n> >>>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> >>>> + descriptors_[requestCookie] = std::move(descriptor);\n> >>>> }\n> >>>>\n> >>>> + worker_.queueRequest(descriptors_[requestCookie].request_.get());\n> >>> Accessing descriptors_ must be while holding descriptorsMutex_.\n> >>\n> >> I am in two minds here,\n> >>\n> >> We can lock it yes, but we are just reading from the map so, should be\n> >> fine?\n> > Regardless of reading/writing, descriptorMutex_ must be acquired\n> > whenever accessing descriptors_.\n> > Some other thread can clear descriptors in parallel. Parallel\n>\n>\n> Not sure, if this a possible yet, but sure!\n>\n> Thanks for the inputs.\n>\n> > executing operations against std::map corrupts it.\n> > By the way, std::map::operator[] is not a constant function. In fact,\n> > it can create a new node if a key does not exist.\n> >\n> > -Hiro\n> >>> -Hiro\n> >>>> +\n> >>>> return 0;\n> >>>> }\n> >>>>\n> >>>> --\n> >>>> 2.31.1\n> >>>>","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 4B33DBF01C\n\tfor ;\n\tFri, 24 Sep 2021 12:57:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 821DE6918C;\n\tFri, 24 Sep 2021 14:57:55 +0200 (CEST)","from mail-yb1-xb31.google.com (mail-yb1-xb31.google.com\n\t[IPv6:2607:f8b0:4864:20::b31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 76CB6687DD\n\tfor ;\n\tFri, 24 Sep 2021 14:57:54 +0200 (CEST)","by mail-yb1-xb31.google.com with SMTP id r4so5539633ybp.4\n\tfor ;\n\tFri, 24 Sep 2021 05:57:54 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"lqS/oW2g\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=VGf6QXiujLMcZkTKGBPcQJDCwSsgJRhjAgE0GwbcJqg=;\n\tb=lqS/oW2g1I5+CBjlXje6W/5bs9Wh1to/NA1SMH5iEm1X3gmpHd/S8PLMDrgaNIykaS\n\tP/jlItyMuNZtJXaoFWCDkOdy4axfzj+LnY7WNldf75g42OJBk8mMEyNsgZ4fckIHwtJj\n\tdGDzCbiclhlPzZGbtWU1aTXexGJQmCv8m0g/M=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=VGf6QXiujLMcZkTKGBPcQJDCwSsgJRhjAgE0GwbcJqg=;\n\tb=LPsj9zcfm1xIPThT5HwArBpgLs5k2VgnfbIadMjheWgAy78l7k1xXyv9YUXKb+Gvy6\n\tdgpzBKWwsCcDiAdJVZO66WJT2i4rCatYScuZsacca7Zno2ccGSL+8YSjj6a/LC7FeMK0\n\tfJMShfqZ5CaveABzIGb5ZP9gt/n8C2gAptB9c57a6yJebb7Kg4S/pExnfdKUn9ptsoWj\n\t93DMJV+KA4MGfPHe2iUO74uYzfohkud0hYk+PT21xRQDacyDQ5hbCPbLrKjwRClgrZy+\n\tifLa+byAfbAX8gKDOHYczF8x9XoR2Xx7LYWyCK7jElxGAIumTNlBD+M/wlcUdtqCqYRT\n\t0Kkg==","X-Gm-Message-State":"AOAM530iIKsB+76jdw6IPrHEhf6WZBzKYUmbBkcb6EziDeq56Cd7f1Wz\n\t4VtBbdMUOaAgJt0sypi7XoxMjEHWB6PjBzIaFYJXbQ==","X-Google-Smtp-Source":"ABdhPJzPke7Zl4G/e7je007XneRRsv8GgHdNN6rP5cEnqg3Kf8Kd3AebsUuLnrZ4mRcRHGCzzK0fP+daEWagedf5OKE=","X-Received":"by 2002:a25:3f04:: with SMTP id\n\tm4mr11600778yba.123.1632488272977; \n\tFri, 24 Sep 2021 05:57:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20210924064409.317140-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t<06440643-b45d-05ae-984b-e65364ae0fd7@ideasonboard.com>","In-Reply-To":"<06440643-b45d-05ae-984b-e65364ae0fd7@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Fri, 24 Sep 2021 21:57:42 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19853,"web_url":"https://patchwork.libcamera.org/comment/19853/","msgid":"","date":"2021-09-27T05:25:35","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Fri, Sep 24, 2021 at 9:57 PM Hirokazu Honda wrote:\n>\n> Hi Umang,\n>\n> On Fri, Sep 24, 2021 at 9:39 PM Umang Jain wrote:\n> >\n> > Hi Hiro,\n> >\n> > On 9/24/21 5:07 PM, Hirokazu Honda wrote:\n> > > Hi Umang,\n> > >\n> > > On Fri, Sep 24, 2021 at 5:19 PM Umang Jain wrote:\n> > >> Hi Hiro,\n> > >>\n> > >> On 9/24/21 1:10 PM, Hirokazu Honda wrote:\n> > >>> Hi Umang, thank you for the patch.\n> > >>>\n> > >>> On Fri, Sep 24, 2021 at 3:44 PM Umang Jain wrote:\n> > >>>> The Camera3RequestDescriptor containing the capture request\n> > >>>> is adding to the descriptors_ map after a call to\n> > >>>> CameraWorker::queueRequest(). This is a race condition since\n> > >>>> CameraWorker::queueRequest() queues request to libcamera::Camera\n> > >>>> asynchronously and the addition of the descriptor to the map\n> > >>>> occurs with std::move(). Hence, it might happen that the async\n> > >>>> queueRequest() hasn't finished but the descriptor gets std::move()ed.\n> > >>>>\n> > >>>> Fix it by adding the descriptor to map first, before\n> > >>>> CameraWorker::queueRequest().\n> > >>> I don't understand the problem well.\n> > >>\n> > >> It's a race between queueRequest() and adding to the descriptor_ std::map.\n> > >>\n> > >> queueRequest() is called asynchronously(separate thread) so it might\n> > >> happen that queueRequest is still processing/queuing the request while\n> > >> we std::move()ed the descriptor (Which wraps the request queueRequest is\n> > >> accessing)\n> > >>\n> > >> If we fix the order with correct data-access it fixes the issue.\n> > >>\n> > >>> I think the pointer of descriptors.request_.get() is not invalidated\n> > >>> by std::move().\n> > >>\n> > >> Why do you think so? :)\n> > >>\n> > > std::move() invalidates descriptor. Yes, descriptor.request_ becomes\n> > > invalidated after std::move(). descriptor must not be accessed after\n> > > std::move().\n> > > However, queueRequest holds a pointer value,\n> > > descriptor.request_.get(). descriptor is move()d to descriptors_ and\n> > > thus descriptor.request_ is alive until descriptor is removed in\n> > > descriptors_.\n> > > So the passed pointer should still be valid after executing std::move().\n> >\n> >\n> > Generally, in other programming models, I have often seen data that has\n> > been move()d should be now/further accessed(if necessary) from that\n> > container that they have been move()d into. It clearly differentiates\n> > between the ownership model (before and after) and I find it as a good\n> > mental model to have (helps especially when there is a long function\n> > implementation )\n> >\n> > Yes, we can store a pointer before the move and call queueRequest() with\n> > it after the move. Either of the methods, I am not against any one.\n> >\n>\n> Since copying a pointer value is before std::move(), it seems to be\n> natural for me to use descriptor.request_.get(), which is the original\n> code.\n> Hmm, I don't understand why the original code doesn't work. I guess\n> the seg fault is due to other reason.\n>\n\nWould you mind investigating more?\n\nBest Regards,\n-Hiro\n\n> -Hiro\n> > >\n> > >> diff --git a/src/android/camera_device.cpp\n> > >> b/src/android/camera_device.cpp\n> > >> index a693dcbe..0de7050d 100644\n> > >> --- a/src/android/camera_device.cpp\n> > >> +++ b/src/android/camera_device.cpp\n> > >> @@ -1063,13 +1063,13 @@ int\n> > >> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >> state_ = State::Running;\n> > >> }\n> > >>\n> > >> - worker_.queueRequest(descriptor.request_.get());\n> > >> -\n> > >> {\n> > >> MutexLocker descriptorsLock(descriptorsMutex_);\n> > >> descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > >> }\n> > >>\n> > >> + worker_.queueRequest(descriptor.request_.get());\n> > >> +\n> > >>\n> > >> is a segfaulted here (and expected no?).\n> > >>\n> > >>>> Signed-off-by: Umang Jain \n> > >>>> ---\n> > >>>> src/android/camera_device.cpp | 7 ++++---\n> > >>>> 1 file changed, 4 insertions(+), 3 deletions(-)\n> > >>>>\n> > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > >>>> index 21844e51..c4c03d86 100644\n> > >>>> --- a/src/android/camera_device.cpp\n> > >>>> +++ b/src/android/camera_device.cpp\n> > >>>> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >>>> state_ = State::Running;\n> > >>>> }\n> > >>>>\n> > >>>> - worker_.queueRequest(descriptor.request_.get());\n> > >>>> -\n> > >>>> + unsigned long requestCookie = descriptor.request_->cookie();\n> > >>>> {\n> > >>>> MutexLocker descriptorsLock(descriptorsMutex_);\n> > >>>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > >>>> + descriptors_[requestCookie] = std::move(descriptor);\n> > >>>> }\n> > >>>>\n> > >>>> + worker_.queueRequest(descriptors_[requestCookie].request_.get());\n> > >>> Accessing descriptors_ must be while holding descriptorsMutex_.\n> > >>\n> > >> I am in two minds here,\n> > >>\n> > >> We can lock it yes, but we are just reading from the map so, should be\n> > >> fine?\n> > > Regardless of reading/writing, descriptorMutex_ must be acquired\n> > > whenever accessing descriptors_.\n> > > Some other thread can clear descriptors in parallel. Parallel\n> >\n> >\n> > Not sure, if this a possible yet, but sure!\n> >\n> > Thanks for the inputs.\n> >\n> > > executing operations against std::map corrupts it.\n> > > By the way, std::map::operator[] is not a constant function. In fact,\n> > > it can create a new node if a key does not exist.\n> > >\n> > > -Hiro\n> > >>> -Hiro\n> > >>>> +\n> > >>>> return 0;\n> > >>>> }\n> > >>>>\n> > >>>> --\n> > >>>> 2.31.1\n> > >>>>","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 1D3D0BDC71\n\tfor ;\n\tMon, 27 Sep 2021 05:25:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5ABD96918E;\n\tMon, 27 Sep 2021 07:25:48 +0200 (CEST)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2F5A684C8\n\tfor ;\n\tMon, 27 Sep 2021 07:25:46 +0200 (CEST)","by mail-ed1-x52e.google.com with SMTP id bd28so2700935edb.9\n\tfor ;\n\tSun, 26 Sep 2021 22:25:46 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"kEFa8vr2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=IThufDCQpTiceICBIT3trANW+DVu8vyU0R37i/kh9N8=;\n\tb=kEFa8vr21Qm/LlgjVsCCKfIzkcyCH/D1nwCvQIXn+jiuoqu6CUqUtYEyHlOFaFGzy2\n\tBvl4ksrS61PI1lhqCjTGcWzGrYAv40IVF3o5d2eOrSgrWQCjkiMk+J5XrSV5b32cVO+N\n\tqdwUF+Txc70kL8Q++vR5KrL6nPyJfS6S5joFc=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=IThufDCQpTiceICBIT3trANW+DVu8vyU0R37i/kh9N8=;\n\tb=pLSPWn8M8u9y0DTMlsyv9CKC6KrFAwCgiYz35wpJ321mnKxyS6TR2zLzWw+CHFOKOJ\n\tcpus071s41QJecehgAz5jrZy/1OYpsX1eNVRqcYp2lqfLwxdxCNQbVnkLjlHK6pPZ31/\n\tbRwpFRpRRlvsToSU+twVA7MiJeKDlEuu+aOOC+SnAk15zmXQ5WK+rLWgMsa3CJ88IQ4v\n\tKsxNd6w8uyzBUioegVfNHeDf6lfo/MxD2gFxnIo1PTRTaRLylz4kLhsXo65zVarweztq\n\ttAFjbIdsYE8DVQXruXXklZBcEISHLHT532qtrLan6Ceo8aAWxfI/VS8aWNtLjwb19ula\n\t9iLQ==","X-Gm-Message-State":"AOAM533szM07Mfsp7BpIPUkafX4L33WyZOu5NxLLnqDV1QSXZjpNltLj\n\tTYmENOCZ/5mkXo2z6xf+lsz5c+an2WKYy/G1RK50vwhL+Qs=","X-Google-Smtp-Source":"ABdhPJxswI3NSWEWVuLfrpmoWrGJEHLMgPKbLQ+YeqEuVLnDolMKGTNaGj3sWv9MzLI9MfTympKpkpK6qo9lLsrVGAE=","X-Received":"by 2002:aa7:c401:: with SMTP id j1mr3420982edq.354.1632720346380;\n\tSun, 26 Sep 2021 22:25:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20210924064409.317140-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t<06440643-b45d-05ae-984b-e65364ae0fd7@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 27 Sep 2021 14:25:35 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19855,"web_url":"https://patchwork.libcamera.org/comment/19855/","msgid":"","date":"2021-09-27T05:45:04","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"+Jacopo Mondi\n\nI saw Jacopo pointed this in https://patchwork.libcamera.org/patch/13866/.\nI consider there is no race condition here. Jacopo, am I wrong?\n\n-Hiro\n\nOn Mon, Sep 27, 2021 at 2:25 PM Hirokazu Honda wrote:\n>\n> Hi Umang,\n>\n> On Fri, Sep 24, 2021 at 9:57 PM Hirokazu Honda wrote:\n> >\n> > Hi Umang,\n> >\n> > On Fri, Sep 24, 2021 at 9:39 PM Umang Jain wrote:\n> > >\n> > > Hi Hiro,\n> > >\n> > > On 9/24/21 5:07 PM, Hirokazu Honda wrote:\n> > > > Hi Umang,\n> > > >\n> > > > On Fri, Sep 24, 2021 at 5:19 PM Umang Jain wrote:\n> > > >> Hi Hiro,\n> > > >>\n> > > >> On 9/24/21 1:10 PM, Hirokazu Honda wrote:\n> > > >>> Hi Umang, thank you for the patch.\n> > > >>>\n> > > >>> On Fri, Sep 24, 2021 at 3:44 PM Umang Jain wrote:\n> > > >>>> The Camera3RequestDescriptor containing the capture request\n> > > >>>> is adding to the descriptors_ map after a call to\n> > > >>>> CameraWorker::queueRequest(). This is a race condition since\n> > > >>>> CameraWorker::queueRequest() queues request to libcamera::Camera\n> > > >>>> asynchronously and the addition of the descriptor to the map\n> > > >>>> occurs with std::move(). Hence, it might happen that the async\n> > > >>>> queueRequest() hasn't finished but the descriptor gets std::move()ed.\n> > > >>>>\n> > > >>>> Fix it by adding the descriptor to map first, before\n> > > >>>> CameraWorker::queueRequest().\n> > > >>> I don't understand the problem well.\n> > > >>\n> > > >> It's a race between queueRequest() and adding to the descriptor_ std::map.\n> > > >>\n> > > >> queueRequest() is called asynchronously(separate thread) so it might\n> > > >> happen that queueRequest is still processing/queuing the request while\n> > > >> we std::move()ed the descriptor (Which wraps the request queueRequest is\n> > > >> accessing)\n> > > >>\n> > > >> If we fix the order with correct data-access it fixes the issue.\n> > > >>\n> > > >>> I think the pointer of descriptors.request_.get() is not invalidated\n> > > >>> by std::move().\n> > > >>\n> > > >> Why do you think so? :)\n> > > >>\n> > > > std::move() invalidates descriptor. Yes, descriptor.request_ becomes\n> > > > invalidated after std::move(). descriptor must not be accessed after\n> > > > std::move().\n> > > > However, queueRequest holds a pointer value,\n> > > > descriptor.request_.get(). descriptor is move()d to descriptors_ and\n> > > > thus descriptor.request_ is alive until descriptor is removed in\n> > > > descriptors_.\n> > > > So the passed pointer should still be valid after executing std::move().\n> > >\n> > >\n> > > Generally, in other programming models, I have often seen data that has\n> > > been move()d should be now/further accessed(if necessary) from that\n> > > container that they have been move()d into. It clearly differentiates\n> > > between the ownership model (before and after) and I find it as a good\n> > > mental model to have (helps especially when there is a long function\n> > > implementation )\n> > >\n> > > Yes, we can store a pointer before the move and call queueRequest() with\n> > > it after the move. Either of the methods, I am not against any one.\n> > >\n> >\n> > Since copying a pointer value is before std::move(), it seems to be\n> > natural for me to use descriptor.request_.get(), which is the original\n> > code.\n> > Hmm, I don't understand why the original code doesn't work. I guess\n> > the seg fault is due to other reason.\n> >\n>\n> Would you mind investigating more?\n>\n> Best Regards,\n> -Hiro\n>\n> > -Hiro\n> > > >\n> > > >> diff --git a/src/android/camera_device.cpp\n> > > >> b/src/android/camera_device.cpp\n> > > >> index a693dcbe..0de7050d 100644\n> > > >> --- a/src/android/camera_device.cpp\n> > > >> +++ b/src/android/camera_device.cpp\n> > > >> @@ -1063,13 +1063,13 @@ int\n> > > >> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > >> state_ = State::Running;\n> > > >> }\n> > > >>\n> > > >> - worker_.queueRequest(descriptor.request_.get());\n> > > >> -\n> > > >> {\n> > > >> MutexLocker descriptorsLock(descriptorsMutex_);\n> > > >> descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > > >> }\n> > > >>\n> > > >> + worker_.queueRequest(descriptor.request_.get());\n> > > >> +\n> > > >>\n> > > >> is a segfaulted here (and expected no?).\n> > > >>\n> > > >>>> Signed-off-by: Umang Jain \n> > > >>>> ---\n> > > >>>> src/android/camera_device.cpp | 7 ++++---\n> > > >>>> 1 file changed, 4 insertions(+), 3 deletions(-)\n> > > >>>>\n> > > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > >>>> index 21844e51..c4c03d86 100644\n> > > >>>> --- a/src/android/camera_device.cpp\n> > > >>>> +++ b/src/android/camera_device.cpp\n> > > >>>> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > >>>> state_ = State::Running;\n> > > >>>> }\n> > > >>>>\n> > > >>>> - worker_.queueRequest(descriptor.request_.get());\n> > > >>>> -\n> > > >>>> + unsigned long requestCookie = descriptor.request_->cookie();\n> > > >>>> {\n> > > >>>> MutexLocker descriptorsLock(descriptorsMutex_);\n> > > >>>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > > >>>> + descriptors_[requestCookie] = std::move(descriptor);\n> > > >>>> }\n> > > >>>>\n> > > >>>> + worker_.queueRequest(descriptors_[requestCookie].request_.get());\n> > > >>> Accessing descriptors_ must be while holding descriptorsMutex_.\n> > > >>\n> > > >> I am in two minds here,\n> > > >>\n> > > >> We can lock it yes, but we are just reading from the map so, should be\n> > > >> fine?\n> > > > Regardless of reading/writing, descriptorMutex_ must be acquired\n> > > > whenever accessing descriptors_.\n> > > > Some other thread can clear descriptors in parallel. Parallel\n> > >\n> > >\n> > > Not sure, if this a possible yet, but sure!\n> > >\n> > > Thanks for the inputs.\n> > >\n> > > > executing operations against std::map corrupts it.\n> > > > By the way, std::map::operator[] is not a constant function. In fact,\n> > > > it can create a new node if a key does not exist.\n> > > >\n> > > > -Hiro\n> > > >>> -Hiro\n> > > >>>> +\n> > > >>>> return 0;\n> > > >>>> }\n> > > >>>>\n> > > >>>> --\n> > > >>>> 2.31.1\n> > > >>>>","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 DFA29BDC71\n\tfor ;\n\tMon, 27 Sep 2021 05:45:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3578C69191;\n\tMon, 27 Sep 2021 07:45:18 +0200 (CEST)","from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com\n\t[IPv6:2a00:1450:4864:20::52b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4685684C8\n\tfor ;\n\tMon, 27 Sep 2021 07:45:15 +0200 (CEST)","by mail-ed1-x52b.google.com with SMTP id v10so59844439edj.10\n\tfor ;\n\tSun, 26 Sep 2021 22:45:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"oJVA3yD4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=xO+U2eSuLhQ/pFx2moo0tPrcyUmqeArFNIHORJtpVoc=;\n\tb=oJVA3yD4qYHw2yuTicX4LPae/ckQ1PQPanF+ZxExN+mFILChqVdhmDtTaVgsZQBmfi\n\tlcDbQjF/anhPa6EZ4W07PgmsHaQWyjDkmm3+IzUjh7IC+Ad1FY3oP7QiUxabPC/6l9Eo\n\t/9bfHpmlJND5f9iMfRlqltpEDl59VzI5kWzUQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=xO+U2eSuLhQ/pFx2moo0tPrcyUmqeArFNIHORJtpVoc=;\n\tb=hbugi32zwMbPtspXFDxk82YXCyDugfMoJKP9x0Sq4xiQ1iQE0EYk3W8sgnE4YHWwBl\n\tPckUqSwspIB0EfRFCCnybQjv/6YlmjOYzR78sxoz11UblGaXlgUpkCSdXTPxmXg/vMXx\n\t7gTMFPL32ZG43Mkz+6+54xD3sFX4FhKdJvyfCkSbSXdKs+do4Uj9tMvgefKhXhQOqhZn\n\tCkdLoW6D2SmsNTUWNkIODNiiJw7pxwm4UI2nwdY9IBGbT9Ou0DyNun1q9IZLcfkuosCb\n\tPKngV4A13eKvp86lDsoTpFHzbp4SMuQkA4jAwB6WfRbK/Z3jyi/+GWPhH2frGDcDHb2C\n\tHzlQ==","X-Gm-Message-State":"AOAM5306P96qHHtNnE5FLuL/gqHhGaCB9i538QrvPMKv2NoUPB3odHv7\n\t9/+WCR2+Nrf9I2O8SYs9v2HaGHYbVF9yXUr2R0HJ1g==","X-Google-Smtp-Source":"ABdhPJwHvGHPoprCu9odvGb4Syy/N+1PRvGrNwH3fwthlQ7ifBcchqjMllVgAmqRBaf1GcPMxdrcEIlpiOdmue0iVoc=","X-Received":"by 2002:a50:8161:: with SMTP id 88mr4493739edc.276.1632721515392;\n\tSun, 26 Sep 2021 22:45:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20210924064409.317140-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t<06440643-b45d-05ae-984b-e65364ae0fd7@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 27 Sep 2021 14:45:04 +0900","Message-ID":"","To":"Umang Jain ,\n\tJacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19882,"web_url":"https://patchwork.libcamera.org/comment/19882/","msgid":"<20210927125545.yjppgqxcomxynkth@uno.localdomain>","date":"2021-09-27T12:55:45","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, Sep 27, 2021 at 02:45:04PM +0900, Hirokazu Honda wrote:\n> +Jacopo Mondi\n>\n> I saw Jacopo pointed this in https://patchwork.libcamera.org/patch/13866/.\n> I consider there is no race condition here. Jacopo, am I wrong?\n\nNot really sure to which part you are referring to, if the race Umang\nis investigating or the access to the descriptors_ map being protected\nor not.\n\nI'll try go through the discussion in previous emails as here some\ncontext have been removed during the conversation.\n\n\n>\n> -Hiro\n>\n> On Mon, Sep 27, 2021 at 2:25 PM Hirokazu Honda wrote:\n> >\n> > Hi Umang,\n> >\n> > On Fri, Sep 24, 2021 at 9:57 PM Hirokazu Honda wrote:\n> > >\n> > > Hi Umang,\n> > >\n> > > On Fri, Sep 24, 2021 at 9:39 PM Umang Jain wrote:\n> > > >\n> > > > Hi Hiro,\n> > > >\n> > > > On 9/24/21 5:07 PM, Hirokazu Honda wrote:\n> > > > > Hi Umang,\n> > > > >\n> > > > > On Fri, Sep 24, 2021 at 5:19 PM Umang Jain wrote:\n> > > > >> Hi Hiro,\n> > > > >>\n> > > > >> On 9/24/21 1:10 PM, Hirokazu Honda wrote:\n> > > > >>> Hi Umang, thank you for the patch.\n> > > > >>>\n> > > > >>> On Fri, Sep 24, 2021 at 3:44 PM Umang Jain wrote:\n> > > > >>>> The Camera3RequestDescriptor containing the capture request\n> > > > >>>> is adding to the descriptors_ map after a call to\n> > > > >>>> CameraWorker::queueRequest(). This is a race condition since\n> > > > >>>> CameraWorker::queueRequest() queues request to libcamera::Camera\n> > > > >>>> asynchronously and the addition of the descriptor to the map\n> > > > >>>> occurs with std::move(). Hence, it might happen that the async\n> > > > >>>> queueRequest() hasn't finished but the descriptor gets std::move()ed.\n> > > > >>>>\n> > > > >>>> Fix it by adding the descriptor to map first, before\n> > > > >>>> CameraWorker::queueRequest().\n> > > > >>> I don't understand the problem well.\n> > > > >>\n> > > > >> It's a race between queueRequest() and adding to the descriptor_ std::map.\n> > > > >>\n> > > > >> queueRequest() is called asynchronously(separate thread) so it might\n> > > > >> happen that queueRequest is still processing/queuing the request while\n> > > > >> we std::move()ed the descriptor (Which wraps the request queueRequest is\n> > > > >> accessing)\n> > > > >>\n> > > > >> If we fix the order with correct data-access it fixes the issue.\n> > > > >>\n> > > > >>> I think the pointer of descriptors.request_.get() is not invalidated\n> > > > >>> by std::move().\n> > > > >>\n> > > > >> Why do you think so? :)\n> > > > >>\n> > > > > std::move() invalidates descriptor. Yes, descriptor.request_ becomes\n> > > > > invalidated after std::move(). descriptor must not be accessed after\n> > > > > std::move().\n> > > > > However, queueRequest holds a pointer value,\n> > > > > descriptor.request_.get(). descriptor is move()d to descriptors_ and\n> > > > > thus descriptor.request_ is alive until descriptor is removed in\n> > > > > descriptors_.\n> > > > > So the passed pointer should still be valid after executing std::move().\n> > > >\n> > > >\n> > > > Generally, in other programming models, I have often seen data that has\n> > > > been move()d should be now/further accessed(if necessary) from that\n> > > > container that they have been move()d into. It clearly differentiates\n> > > > between the ownership model (before and after) and I find it as a good\n> > > > mental model to have (helps especially when there is a long function\n> > > > implementation )\n> > > >\n> > > > Yes, we can store a pointer before the move and call queueRequest() with\n> > > > it after the move. Either of the methods, I am not against any one.\n> > > >\n> > >\n> > > Since copying a pointer value is before std::move(), it seems to be\n> > > natural for me to use descriptor.request_.get(), which is the original\n> > > code.\n> > > Hmm, I don't understand why the original code doesn't work. I guess\n> > > the seg fault is due to other reason.\n> > >\n> >\n> > Would you mind investigating more?\n> >\n> > Best Regards,\n> > -Hiro\n> >\n> > > -Hiro\n> > > > >\n> > > > >> diff --git a/src/android/camera_device.cpp\n> > > > >> b/src/android/camera_device.cpp\n> > > > >> index a693dcbe..0de7050d 100644\n> > > > >> --- a/src/android/camera_device.cpp\n> > > > >> +++ b/src/android/camera_device.cpp\n> > > > >> @@ -1063,13 +1063,13 @@ int\n> > > > >> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > >> state_ = State::Running;\n> > > > >> }\n> > > > >>\n> > > > >> - worker_.queueRequest(descriptor.request_.get());\n> > > > >> -\n> > > > >> {\n> > > > >> MutexLocker descriptorsLock(descriptorsMutex_);\n> > > > >> descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > > > >> }\n> > > > >>\n> > > > >> + worker_.queueRequest(descriptor.request_.get());\n> > > > >> +\n> > > > >>\n> > > > >> is a segfaulted here (and expected no?).\n> > > > >>\n> > > > >>>> Signed-off-by: Umang Jain \n> > > > >>>> ---\n> > > > >>>> src/android/camera_device.cpp | 7 ++++---\n> > > > >>>> 1 file changed, 4 insertions(+), 3 deletions(-)\n> > > > >>>>\n> > > > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > >>>> index 21844e51..c4c03d86 100644\n> > > > >>>> --- a/src/android/camera_device.cpp\n> > > > >>>> +++ b/src/android/camera_device.cpp\n> > > > >>>> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > >>>> state_ = State::Running;\n> > > > >>>> }\n> > > > >>>>\n> > > > >>>> - worker_.queueRequest(descriptor.request_.get());\n> > > > >>>> -\n> > > > >>>> + unsigned long requestCookie = descriptor.request_->cookie();\n> > > > >>>> {\n> > > > >>>> MutexLocker descriptorsLock(descriptorsMutex_);\n> > > > >>>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > > > >>>> + descriptors_[requestCookie] = std::move(descriptor);\n> > > > >>>> }\n> > > > >>>>\n> > > > >>>> + worker_.queueRequest(descriptors_[requestCookie].request_.get());\n> > > > >>> Accessing descriptors_ must be while holding descriptorsMutex_.\n> > > > >>\n> > > > >> I am in two minds here,\n> > > > >>\n> > > > >> We can lock it yes, but we are just reading from the map so, should be\n> > > > >> fine?\n> > > > > Regardless of reading/writing, descriptorMutex_ must be acquired\n> > > > > whenever accessing descriptors_.\n> > > > > Some other thread can clear descriptors in parallel. Parallel\n> > > >\n> > > >\n> > > > Not sure, if this a possible yet, but sure!\n> > > >\n> > > > Thanks for the inputs.\n> > > >\n> > > > > executing operations against std::map corrupts it.\n> > > > > By the way, std::map::operator[] is not a constant function. In fact,\n> > > > > it can create a new node if a key does not exist.\n> > > > >\n> > > > > -Hiro\n> > > > >>> -Hiro\n> > > > >>>> +\n> > > > >>>> return 0;\n> > > > >>>> }\n> > > > >>>>\n> > > > >>>> --\n> > > > >>>> 2.31.1\n> > > > >>>>","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 3182EC3243\n\tfor ;\n\tMon, 27 Sep 2021 12:55:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BC6D6918B;\n\tMon, 27 Sep 2021 14:55:00 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E9AF6012C\n\tfor ;\n\tMon, 27 Sep 2021 14:54:59 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 74A7F6000C;\n\tMon, 27 Sep 2021 12:54:58 +0000 (UTC)"],"Date":"Mon, 27 Sep 2021 14:55:45 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210927125545.yjppgqxcomxynkth@uno.localdomain>","References":"<20210924064409.317140-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t<06440643-b45d-05ae-984b-e65364ae0fd7@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19885,"web_url":"https://patchwork.libcamera.org/comment/19885/","msgid":"<20210927135036.nr26sjb33xbz65ga@uno.localdomain>","date":"2021-09-27T13:50:36","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang, Hiro,\n\nOn Fri, Sep 24, 2021 at 01:49:19PM +0530, Umang Jain wrote:\n> Hi Hiro,\n>\n> On 9/24/21 1:10 PM, Hirokazu Honda wrote:\n> > Hi Umang, thank you for the patch.\n> >\n> > On Fri, Sep 24, 2021 at 3:44 PM Umang Jain wrote:\n> > > The Camera3RequestDescriptor containing the capture request\n> > > is adding to the descriptors_ map after a call to\n> > > CameraWorker::queueRequest(). This is a race condition since\n> > > CameraWorker::queueRequest() queues request to libcamera::Camera\n> > > asynchronously and the addition of the descriptor to the map\n> > > occurs with std::move(). Hence, it might happen that the async\n> > > queueRequest() hasn't finished but the descriptor gets std::move()ed.\n> > >\n> > > Fix it by adding the descriptor to map first, before\n> > > CameraWorker::queueRequest().\n> >\n> > I don't understand the problem well.\n>\n>\n> It's a race between queueRequest() and adding to the descriptor_ std::map.\n>\n> queueRequest() is called asynchronously(separate thread)  so it might happen\n> that queueRequest is still processing/queuing the request while we\n> std::move()ed the descriptor (Which wraps the request queueRequest is\n> accessing)\n>\n> If we fix the order with correct data-access it fixes the issue.\n>\n> > I think the pointer of descriptors.request_.get() is not invalidated\n> > by std::move().\n\nYou know, I'm not really sure :)\nLet's try to find it out...\n\nThis is the descriptor lifecycle inside this function\n\nint processRequest()\n{\n // Allocated on the stack, request_ is a unique_ptr<> embedded in\n // descriptor\n Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);\n\n ....\n\n worker_.queueRequest(descriptor.request_.get());\n descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n}\n\nCamera3RequestDescriptor::request_ is initialized as\n\n\trequest_ = std::make_unique(camera);\n\nCameraDevice::descriptors_ is a map of type:\n std::map descriptors_;\n\nAnd Camera3RequestDescriptor has a defaulted move assignment operator=()\n \t\tCamera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n\nAs Camera3RequestDescriptor member are:\n\n\tstruct Camera3RequestDescriptor {\n\n\t\tuint32_t frameNumber_ = 0;\n\t\tstd::vector buffers_;\n\t\tstd::vector> frameBuffers_;\n\t\tCameraMetadata settings_;\n\t\tstd::unique_ptr request_;\n\t};\n\nAnd most of them have a non-implicitly defined move assignment operator (vecors\nand unique_ptr) I think we fall in the \"Implicitly defined\" section of [1]\n\n-------------------------------------------------------------------------------\nImplicitly-defined move assignment operator\n\nIf the implicitly-declared move assignment operator is neither deleted nor\ntrivial, it is defined (that is, a function body is generated and compiled) by\nthe compiler if odr-used or needed for constant evaluation (since C++14).\n\nFor union types, the implicitly-defined move assignment operator copies the\nobject representation (as by std::memmove).\n\nFor non-union class types (class and struct), the move assignment operator\nperforms full member-wise move assignment of the object's direct bases and\nimmediate non-static members, in their declaration order, using built-in\nassignment for the scalars, memberwise move-assignment for arrays, and move\nassignment operator for class types (called non-virtually).\n-------------------------------------------------------------------------------\n\nWhich means we're effectively calling std::unique_ptr::operator[](&&) on\nrequest_.\n\nAs std::unique_ptr::operator[](&&other) is equivalent to\n\n reset(other.release());\n\nI guess what happens is that the location in memory of request_ doesn't change\nbut it's only its ownership that gets moved to the newly created entry in the\ndescriptors_ map.\n\nHence I think the code is safe the way it is, even if it might look suspicious\nor fragile.\n\nI think it could be made 'safer' if we want to by:\n\n- worker_.queueRequest(descriptor.request_.get());\n-\n+ CaptureRequest *req;\n {\n MutexLocker descriptorsLock(descriptorsMutex_);\n+ unsigned long requestCookie = descriptor.request_->cookie();\n- descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n+ descriptors_[requestCookie] = std::move(descriptor);\n+ req = descriptors_[requestCookie].request_.get();\n }\n\n+ worker_.queueRequest(req);\n+\n return 0;\n }\n\nBut I didn't get if Umang hit any issue that lead him to send this patch.\n\n[1] https://en.cppreference.com/w/cpp/language/move_assignment\n\n>\n>\n> Why do you think so? :)\n>\n>     diff --git a/src/android/camera_device.cpp\n> b/src/android/camera_device.cpp\n>     index a693dcbe..0de7050d 100644\n>     --- a/src/android/camera_device.cpp\n>     +++ b/src/android/camera_device.cpp\n>     @@ -1063,13 +1063,13 @@ int\n> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>                     state_ = State::Running;\n>             }\n>\n>     - worker_.queueRequest(descriptor.request_.get());\n>     -\n>             {\n>                     MutexLocker descriptorsLock(descriptorsMutex_);\n> descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>             }\n>\n>     + worker_.queueRequest(descriptor.request_.get());\n>     +\n>\n> is a segfaulted here (and expected no?).\n\nFor sure it does, you're now using 'descriptor' after it has been moved and it's\nbeen left in unspecified state. As shown above std::unique_ptr::operator=(&&)\nhas been called on request_ which calls request_.release(), hence accessing it\nwith std::unique_ptr::get() is undefined behaviour.\n\n>\n> >\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > src/android/camera_device.cpp | 7 ++++---\n> > > 1 file changed, 4 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 21844e51..c4c03d86 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > state_ = State::Running;\n> > > }\n> > >\n> > > - worker_.queueRequest(descriptor.request_.get());\n> > > -\n> > > + unsigned long requestCookie = descriptor.request_->cookie();\n> > > {\n> > > MutexLocker descriptorsLock(descriptorsMutex_);\n> > > - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > > + descriptors_[requestCookie] = std::move(descriptor);\n> > > }\n> > >\n> > > + worker_.queueRequest(descriptors_[requestCookie].request_.get());\n> > Accessing descriptors_ must be while holding descriptorsMutex_.\n>\n>\n> I am in two minds here,\n>\n> We can lock it yes, but we are just reading from the map so, should be fine?\n\nI think accessing descriptors_ should always happen while holding the\ndescriptorsMutex_ lock, as we extract nodes from the map in requestComplete()\nwhich might invalidates iterators and re-points the internal pointers in the\nmap. I can't tell what happens if done concurrently with accessing the map, but\ndoesn't seem like a good idea.\n\nThanks\n j\n\n>\n> >\n> > -Hiro\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > --\n> > > 2.31.1\n> > >","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 7B7D8C3243\n\tfor ;\n\tMon, 27 Sep 2021 13:49:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E06B96918E;\n\tMon, 27 Sep 2021 15:49:50 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9093E6012C\n\tfor ;\n\tMon, 27 Sep 2021 15:49:49 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id E7D3A40004;\n\tMon, 27 Sep 2021 13:49:48 +0000 (UTC)"],"Date":"Mon, 27 Sep 2021 15:50:36 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210927135036.nr26sjb33xbz65ga@uno.localdomain>","References":"<20210924064409.317140-1-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] android: camera_device: Fix race on\n\tqueuing capture request","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19889,"web_url":"https://patchwork.libcamera.org/comment/19889/","msgid":"<04fbc104-97ea-c4f0-1e99-1bcb7c408161@ideasonboard.com>","date":"2021-09-27T15:13:48","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo, Hiro,\n\nOn 9/27/21 7:20 PM, Jacopo Mondi wrote:\n> Hi Umang, Hiro,\n>\n> On Fri, Sep 24, 2021 at 01:49:19PM +0530, Umang Jain wrote:\n>> Hi Hiro,\n>>\n>> On 9/24/21 1:10 PM, Hirokazu Honda wrote:\n>>> Hi Umang, thank you for the patch.\n>>>\n>>> On Fri, Sep 24, 2021 at 3:44 PM Umang Jain wrote:\n>>>> The Camera3RequestDescriptor containing the capture request\n>>>> is adding to the descriptors_ map after a call to\n>>>> CameraWorker::queueRequest(). This is a race condition since\n>>>> CameraWorker::queueRequest() queues request to libcamera::Camera\n>>>> asynchronously and the addition of the descriptor to the map\n>>>> occurs with std::move(). Hence, it might happen that the async\n>>>> queueRequest() hasn't finished but the descriptor gets std::move()ed.\n>>>>\n>>>> Fix it by adding the descriptor to map first, before\n>>>> CameraWorker::queueRequest().\n>>> I don't understand the problem well.\n>>\n>> It's a race between queueRequest() and adding to the descriptor_ std::map.\n>>\n>> queueRequest() is called asynchronously(separate thread)  so it might happen\n>> that queueRequest is still processing/queuing the request while we\n>> std::move()ed the descriptor (Which wraps the request queueRequest is\n>> accessing)\n>>\n>> If we fix the order with correct data-access it fixes the issue.\n>>\n>>> I think the pointer of descriptors.request_.get() is not invalidated\n>>> by std::move().\n> You know, I'm not really sure :)\n> Let's try to find it out...\n>\n> This is the descriptor lifecycle inside this function\n>\n> int processRequest()\n> {\n> // Allocated on the stack, request_ is a unique_ptr<> embedded in\n> // descriptor\n> Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);\n>\n> ....\n>\n> worker_.queueRequest(descriptor.request_.get());\n> descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> }\n>\n> Camera3RequestDescriptor::request_ is initialized as\n>\n> \trequest_ = std::make_unique(camera);\n>\n> CameraDevice::descriptors_ is a map of type:\n> std::map descriptors_;\n>\n> And Camera3RequestDescriptor has a defaulted move assignment operator=()\n> \t\tCamera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n>\n> As Camera3RequestDescriptor member are:\n>\n> \tstruct Camera3RequestDescriptor {\n>\n> \t\tuint32_t frameNumber_ = 0;\n> \t\tstd::vector buffers_;\n> \t\tstd::vector> frameBuffers_;\n> \t\tCameraMetadata settings_;\n> \t\tstd::unique_ptr request_;\n> \t};\n>\n> And most of them have a non-implicitly defined move assignment operator (vecors\n> and unique_ptr) I think we fall in the \"Implicitly defined\" section of [1]\n>\n> -------------------------------------------------------------------------------\n> Implicitly-defined move assignment operator\n>\n> If the implicitly-declared move assignment operator is neither deleted nor\n> trivial, it is defined (that is, a function body is generated and compiled) by\n> the compiler if odr-used or needed for constant evaluation (since C++14).\n>\n> For union types, the implicitly-defined move assignment operator copies the\n> object representation (as by std::memmove).\n>\n> For non-union class types (class and struct), the move assignment operator\n> performs full member-wise move assignment of the object's direct bases and\n> immediate non-static members, in their declaration order, using built-in\n> assignment for the scalars, memberwise move-assignment for arrays, and move\n> assignment operator for class types (called non-virtually).\n> -------------------------------------------------------------------------------\n>\n> Which means we're effectively calling std::unique_ptr::operator[](&&) on\n> request_.\n>\n> As std::unique_ptr::operator[](&&other) is equivalent to\n>\n> reset(other.release());\n>\n> I guess what happens is that the location in memory of request_ doesn't change\n> but it's only its ownership that gets moved to the newly created entry in the\n> descriptors_ map.\n>\n> Hence I think the code is safe the way it is, even if it might look suspicious\n> or fragile.\n\n\nOn a lighter note, looks don't matter ? :P\n\nI think I'll slightly dis-agree that it is safe..\n\nI understand that the ::move is not really copying to a different \nlocation of memory, rather transfer of ownership happens, so technically \nit might not cause any problems, since pointers are still pointing to \nmemory location they are supposed to point to (possibly v2 version of \nthis patch is based on this fact). But, ::move()ing  while an async \noperation is in play on a memory location does concern me if we look at it.\n\nIf we want to queueRequest() /before/ the descriptor is moved, I think \nthat should also happen with a lock, since, descriptor.request_.get() is \na descriptor access, no? It might happen that the queueRequest() might \nalso set something on the CaptureRequest pointer, so essentially, we \nwould still to lock the entire async operation before firing it off, \nuntil it returns.\n\n\n>\n> I think it could be made 'safer' if we want to by:\n>\n> - worker_.queueRequest(descriptor.request_.get());\n> -\n> + CaptureRequest *req;\n> {\n> MutexLocker descriptorsLock(descriptorsMutex_);\n> + unsigned long requestCookie = descriptor.request_->cookie();\n> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> + descriptors_[requestCookie] = std::move(descriptor);\n> + req = descriptors_[requestCookie].request_.get();\n> }\n>\n> + worker_.queueRequest(req);\n> +\n> return 0;\n> }\n\n\nPrecisely, that intended goal of the patch is to queueRequest() after \nthe descriptor is moved to the map.\n\n>\n> But I didn't get if Umang hit any issue that lead him to send this patch.\n\n\nPlease mind that v2 of this patch is already on the list last week, \nwhich might be of interest and further discussion. Also, I didn't get \nany issues with it, it's something Laurent initially spotted while code \nreview [1]\n\nExcerpt below :\n\n>/> We have a race condition here, worker_.queueRequest() should go after />/> adding the request to the queue. Could you fix it in a patch on top ? />//>/Do you mean the race condition is existing already, with the />/descriptors_ map (that has been removed from this patch)? /\nCorrect, it's already here.\n\n>/Yes, I can introduce a patch before this one, that fixes the race first />/in the map itself. Is my understanding correct? /\nSounds good to me. It should be a small patch.\n\n[1]: \nhttps://lists.libcamera.org/pipermail/libcamera-devel/2021-September/025004.html\n\n>\n> [1] https://en.cppreference.com/w/cpp/language/move_assignment\n>\n>>\n>> Why do you think so? :)\n>>\n>>     diff --git a/src/android/camera_device.cpp\n>> b/src/android/camera_device.cpp\n>>     index a693dcbe..0de7050d 100644\n>>     --- a/src/android/camera_device.cpp\n>>     +++ b/src/android/camera_device.cpp\n>>     @@ -1063,13 +1063,13 @@ int\n>> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>                     state_ = State::Running;\n>>             }\n>>\n>>     - worker_.queueRequest(descriptor.request_.get());\n>>     -\n>>             {\n>>                     MutexLocker descriptorsLock(descriptorsMutex_);\n>> descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>>             }\n>>\n>>     + worker_.queueRequest(descriptor.request_.get());\n>>     +\n>>\n>> is a segfaulted here (and expected no?).\n> For sure it does, you're now using 'descriptor' after it has been moved and it's\n> been left in unspecified state. As shown above std::unique_ptr::operator=(&&)\n> has been called on request_ which calls request_.release(), hence accessing it\n> with std::unique_ptr::get() is undefined behaviour.\n>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> src/android/camera_device.cpp | 7 ++++---\n>>>> 1 file changed, 4 insertions(+), 3 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 21844e51..c4c03d86 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> state_ = State::Running;\n>>>> }\n>>>>\n>>>> - worker_.queueRequest(descriptor.request_.get());\n>>>> -\n>>>> + unsigned long requestCookie = descriptor.request_->cookie();\n>>>> {\n>>>> MutexLocker descriptorsLock(descriptorsMutex_);\n>>>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>>>> + descriptors_[requestCookie] = std::move(descriptor);\n>>>> }\n>>>>\n>>>> + worker_.queueRequest(descriptors_[requestCookie].request_.get());\n>>> Accessing descriptors_ must be while holding descriptorsMutex_.\n>>\n>> I am in two minds here,\n>>\n>> We can lock it yes, but we are just reading from the map so, should be fine?\n> I think accessing descriptors_ should always happen while holding the\n> descriptorsMutex_ lock, as we extract nodes from the map in requestComplete()\n> which might invalidates iterators and re-points the internal pointers in the\n> map. I can't tell what happens if done concurrently with accessing the map, but\n> doesn't seem like a good idea.\n>\n> Thanks\n> j\n>\n>>> -Hiro\n>>>> +\n>>>> return 0;\n>>>> }\n>>>>\n>>>> --\n>>>> 2.31.1\n>>>>","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 DA842BDC71\n\tfor ;\n\tMon, 27 Sep 2021 15:13:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 23E256918E;\n\tMon, 27 Sep 2021 17:13:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6411E6012C\n\tfor ;\n\tMon, 27 Sep 2021 17:13:54 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.181])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 12E4CE81;\n\tMon, 27 Sep 2021 17:13:52 +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=\"JYIzLxYa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632755634;\n\tbh=JL2eozjHPCf+XXXTQOWx4GYHfry5xMxo/ZW8zWKkj48=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=JYIzLxYaojD3e4rpfelOeVsSxW2H0HgP1aDQuxbV9fW3hrus8vReyIuxRDJuP3DzA\n\tiiqZmlH7TsUaLT2ZL/VRxMHWgZeSaCgC7FWKRZBgjVSOGtmEwR36hZATX+5W7m8tUX\n\tqhfwZM48WwPpFDZBGfmB62Bmc1BxP93OCeJsArqc=","To":"Jacopo Mondi ","References":"<20210924064409.317140-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t<20210927135036.nr26sjb33xbz65ga@uno.localdomain>","From":"Umang Jain ","Message-ID":"<04fbc104-97ea-c4f0-1e99-1bcb7c408161@ideasonboard.com>","Date":"Mon, 27 Sep 2021 20:43:48 +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":"<20210927135036.nr26sjb33xbz65ga@uno.localdomain>","Content-Type":"multipart/alternative;\n\tboundary=\"------------A1606F34AEC6DAC460C72648\"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19905,"web_url":"https://patchwork.libcamera.org/comment/19905/","msgid":"","date":"2021-09-28T01:20:26","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Sep 24, 2021 at 12:14:09PM +0530, Umang Jain wrote:\n> The Camera3RequestDescriptor containing the capture request\n> is adding to the descriptors_ map after a call to\n> CameraWorker::queueRequest(). This is a race condition since\n> CameraWorker::queueRequest() queues request to libcamera::Camera\n> asynchronously and the addition of the descriptor to the map\n> occurs with std::move(). Hence, it might happen that the async\n> queueRequest() hasn't finished but the descriptor gets std::move()ed.\n\nThe issue isn't related to std::move(). The problem is that the request\nis queued to the worker, and at that point processing moves to another\nthread. It's thus possible (even if very unlikely, but still possible)\nthat the processing will complete, and CameraDevice::requestComplete()\ngets called, before this function proceeds to add the request to the\ndescriptors_ map. CameraDevice::requestComplete() will then not find it\nin the map, and bad things will happen.\n\nIt's a classic race condition, std::move() doesn't play any role here.\n\n> Fix it by adding the descriptor to map first, before\n> CameraWorker::queueRequest().\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 21844e51..c4c03d86 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1065,13 +1065,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tstate_ = State::Running;\n> \t}\n> \n> -\tworker_.queueRequest(descriptor.request_.get());\n> -\n> +\tunsigned long requestCookie = descriptor.request_->cookie();\n> \t{\n> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> -\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> +\t\tdescriptors_[requestCookie] = std::move(descriptor);\n> \t}\n> \n> +\tworker_.queueRequest(descriptors_[requestCookie].request_.get());\n\nAs Hiro mentioned, we shouldn't access descriptors_ without taking the\nlock. Here's how to fix it:\n\n\tCaptureRequest *request = descriptor.request_.get();\n\n\t{\n\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n\t\tdescriptors_[request->cookie()] = std::move(descriptor);\n\t}\n\n\tworker_.queueRequest(request);\n\n> +\n> \treturn 0;\n> }\n>","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 C5454C3243\n\tfor ;\n\tTue, 28 Sep 2021 01:20:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2EA3A6918B;\n\tTue, 28 Sep 2021 03:20:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1FD36012D\n\tfor ;\n\tTue, 28 Sep 2021 03:20:33 +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 457073F1;\n\tTue, 28 Sep 2021 03:20:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sww5M1FC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632792033;\n\tbh=xqCij94H2TMxGeN/2rgYDCIUq9elmuPq9H2VMKGCSLg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sww5M1FCPAZZfNUdGFSWZlXgImIGzt1eZw4jPe9XoJiYORZH66jjJqD6n3ZSpuSN6\n\tcj15cZdpTHOY6bVJNy1ZCA92GWTXZxIc+CjKfyKQFzWYkgIiVoJci4GwjKYAibD6B1\n\t1TUbnjpJAvT9gt+0huwGQtVw9jSyKOhjeWPjkkKg=","Date":"Tue, 28 Sep 2021 04:20:26 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210924064409.317140-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210924064409.317140-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix race on\n\tqueuing capture request","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\" "}}]