[{"id":19913,"web_url":"https://patchwork.libcamera.org/comment/19913/","msgid":"","date":"2021-09-28T02:54:38","subject":"Re: [libcamera-devel] [PATCH v2] android: camera_device: Fix race\n\ton queuing 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 06:35:59PM +0530, Umang Jain wrote:\n> The Camera3RequestDescriptor containing the capture request\n> is adding to the descriptors_ map after a call to\n\ns/adding/added/\n\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\nstd::move() isn't really relevant here. I'd write\n\nThe Camera3RequestDescriptor containing the capture request is added to\nthe descriptors_ map after a call to CameraWorker::queueRequest(). This\nis a race condition since CameraWorker::queueRequest() queues requests\nto libcamera::Camera asynchronously. The requests may thus complete\nbefore they get added to descriptors_, in which case requestComplete()\nwill fail to lookup the request in the map.\n\nReviewed-by: Laurent Pinchart \n\n> Fix it by adding the descriptor to map first, before\n> CameraWorker::queueRequest().\n> \n> Signed-off-by: Umang Jain \n> ---\n> changes in v2:\n> - Save the pointer first and directly use it to queueRequest()\n> after std::move\n> ---\n> src/android/camera_device.cpp | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 21844e51..ab381168 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1065,13 +1065,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tstate_ = State::Running;\n> \t}\n> \n> -\tworker_.queueRequest(descriptor.request_.get());\n> +\tCaptureRequest *request = descriptor.request_.get();\n> \n> \t{\n> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> \t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> \t}\n> \n> +\tworker_.queueRequest(request);\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 4A369BDC71\n\tfor ;\n\tTue, 28 Sep 2021 02:54:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA0FE69189;\n\tTue, 28 Sep 2021 04:54:47 +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 EEC3F684C7\n\tfor ;\n\tTue, 28 Sep 2021 04:54:45 +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 637823F1;\n\tTue, 28 Sep 2021 04:54:45 +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=\"jAZQ15wJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632797685;\n\tbh=3Ws3X6jFVG0cC1MEv1045vWdF36IfY4zIf8rep5Imz8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jAZQ15wJjypszJ+c9OYE9i0bwQMeJvMlZcDAnQyjoVSQmhNXqz7pCizQppVYR9f1n\n\tHYrCHwrwUCrqO/QlzJpLXueQW4LV+56m8KTeEC3Zjy9GqcM+rX3my7gN9A9zAYWM4z\n\taPzGB3WzsuM9TMVrE9elSMrHHcJ+AK3oP10x58IE=","Date":"Tue, 28 Sep 2021 05:54:38 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210924130559.418704-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210924130559.418704-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] android: camera_device: Fix race\n\ton queuing 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\" "}},{"id":19914,"web_url":"https://patchwork.libcamera.org/comment/19914/","msgid":"","date":"2021-09-28T04:58:15","subject":"Re: [libcamera-devel] [PATCH v2] android: camera_device: Fix race\n\ton queuing 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 Tue, Sep 28, 2021 at 11:54 AM Laurent Pinchart\n wrote:\n>\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 24, 2021 at 06:35:59PM +0530, Umang Jain wrote:\n> > The Camera3RequestDescriptor containing the capture request\n> > is adding to the descriptors_ map after a call to\n>\n> s/adding/added/\n>\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> std::move() isn't really relevant here. I'd write\n>\n> The Camera3RequestDescriptor containing the capture request is added to\n> the descriptors_ map after a call to CameraWorker::queueRequest(). This\n> is a race condition since CameraWorker::queueRequest() queues requests\n> to libcamera::Camera asynchronously. The requests may thus complete\n> before they get added to descriptors_, in which case requestComplete()\n> will fail to lookup the request in the map.\n>\n> Reviewed-by: Laurent Pinchart \n>\n\nAs Laurent said, std::move() is red-herring.\nAlthough your fix is correct, there is a small window of the race,\nwhich is Laurent explained in v1.\nqueueRequest() must be done after descriptor is in descriptors_ map for sure.\n\nSo with the commit message correction suggested by Laurent,\n\nReviewed-by: Hirokazu Honda \n\n-Hiro\n> > Fix it by adding the descriptor to map first, before\n> > CameraWorker::queueRequest().\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > changes in v2:\n> > - Save the pointer first and directly use it to queueRequest()\n> > after std::move\n> > ---\n> > src/android/camera_device.cpp | 4 +++-\n> > 1 file changed, 3 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 21844e51..ab381168 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1065,13 +1065,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > state_ = State::Running;\n> > }\n> >\n> > - worker_.queueRequest(descriptor.request_.get());\n> > + CaptureRequest *request = descriptor.request_.get();\n> >\n> > {\n> > MutexLocker descriptorsLock(descriptorsMutex_);\n> > descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > }\n> >\n> > + worker_.queueRequest(request);\n> > +\n> > return 0;\n> > }\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 C0051C3243\n\tfor ;\n\tTue, 28 Sep 2021 04:58:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E06846918E;\n\tTue, 28 Sep 2021 06:58:27 +0200 (CEST)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD9F8684C7\n\tfor ;\n\tTue, 28 Sep 2021 06:58:26 +0200 (CEST)","by mail-ed1-x531.google.com with SMTP id g8so78135284edt.7\n\tfor ;\n\tMon, 27 Sep 2021 21:58:26 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"jLWiV3uj\"; 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=XuEb/63H9jgCnXqqxrCrHKzBhFRLqldaKR5eVzAPDX0=;\n\tb=jLWiV3ujKJR3jQy6Cr9Cn4oQ+LCQt37Moxo2ZzXTkFEX9huWIuVSekf+/yDxFih35X\n\tSFZuC2i2A7BVy4Olq4roVWmFkqEHA1kOtCL74Vg2CMoJJZOmAhDncFYCiZ63h9MHdOUu\n\tFBZSfCfSzrMOjE1mQtQDF2RHXNV6AGFbuqeok=","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=XuEb/63H9jgCnXqqxrCrHKzBhFRLqldaKR5eVzAPDX0=;\n\tb=PyxeaX40zCGLUFPa7+Q6E4Go6kEPtRd44msGgqRWIF+9ilKG2cj5VnZVLUtkol2uo8\n\tqZEELovihv+hSe1CCuXhNyU/YvrB51V8iWZIE4RWK1cicaIUVBKyOetRPK17oCOmz2Rq\n\tKuxLXyYRTzayNG7r5eIUqcTz8nO0964dtSiXs+9qzSZ2J9MNEceIxDcyMMiIMVcvQY4g\n\txAPzPSau3tkQImzw1mxzYP+ZqXlNH6TdLXYESqSzceQPc0L5iFa1AU+PYSzWzzS3o/jf\n\toUZ61II6WF30rVff7SwAOv5oAvHHy8Dcn6XTIrpUwcIPZz5QQNSjlgn++AxrEv97Tg5N\n\tZ0jA==","X-Gm-Message-State":"AOAM530e8diuquY5S9XDrhUvzjQ8pNfcaeLhjb6FIxbHiCTRHfhYxd4Z\n\tWhk2gSGrMrQzWfUEHxozBKMTdZrp0LqDCQWhnHUyqQ==","X-Google-Smtp-Source":"ABdhPJwl+gZQBKVWb0rxmmLadBuFzG8N8yoo47hx2oWKBK+neCbjq9Iby4xD3Dp8VuyjYvbcITKwUgNg3G4RvNKJt4M=","X-Received":"by 2002:a50:a0a2:: with SMTP id 31mr2351720edo.206.1632805106415;\n\tMon, 27 Sep 2021 21:58:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20210924130559.418704-1-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 28 Sep 2021 13:58:15 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2] android: camera_device: Fix race\n\ton queuing 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\" "}}]