[{"id":19217,"web_url":"https://patchwork.libcamera.org/comment/19217/","msgid":"<66f1bc7a-6591-e076-bfad-75e632796669@ideasonboard.com>","date":"2021-08-31T12:02:18","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash in\n\tcalling CameraDevice::close()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nThe commit message can be somewhat more contextual of the problem.\n\nOn 8/31/21 1:35 PM, Hirokazu Honda wrote:\n> This fixes the crash when post processing is needed and some capture\n> request is being processed by PipelineHandler upon calling\n> CameraDevice::close().\n> The crash happens in Request::completeBuffer(), which is invoked by\n> Camera::stop(). It uses FrameBuffer after free. The FrameBuffer is\n> allocated and owned by FrameBufferAllocator, and FrameBufferAllocator\n> is owned by CamerStream. So Camera::stop() should be executed before\n> destroying CameraStream.\n\n\nOne question: I can see the problem but what's exactly is crashing in \nRequest::completeBuffer() is something we should know? Is it the ASSERT? \nThat might help us to define the problem statement better.\n\nThe problem is happening because we seem to add a CameraStream \nassociated buffer(depending on the CameraStream::Type) to the Request, \nin CameraDevice::processCaptureRequest().\n\nHowever, when the camera stops, all the current buffers are marked with \nFrameMetadata::FrameCancelled and proceed to completion. But the buffer \nassociated with the CameraStream (that was previously added to the \nrequest) has now been cleared out with a part of streams_.clear(), even \nbefore the camera stop() has been invoked. Any access to those request \nbuffers after they have been cleared, shall result in a crash.\n\n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 8ca76719..74a95a2a 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -423,10 +423,10 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> \n> void CameraDevice::close()\n> {\n> -\tstreams_.clear();\n> -\n> \tstop();\n> \n> +\tstreams_.clear();\n\n\nHow about this being a part of a CameraDevice::stop() instead ?\n\n> +\n> \tcamera_->release();\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 28B31BDC71\n\tfor ;\n\tTue, 31 Aug 2021 12:02:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68B006916A;\n\tTue, 31 Aug 2021 14:02:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C7EE968890\n\tfor ;\n\tTue, 31 Aug 2021 14:02:23 +0200 (CEST)","from [192.168.1.105] (unknown [103.251.226.196])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F333A8F;\n\tTue, 31 Aug 2021 14:02:22 +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=\"m/hsR9uY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630411343;\n\tbh=OPd0PAqlyl7vpLwROnJs6BkvJgRyGoZpEMdx/QBQml8=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=m/hsR9uYombVrqU2taocyYD6Qcv+/mLdz+Gr9pPcJtpYTgNR6CtNLlquCVUyCiJ8k\n\thwMo3hwcVdKqN1RSUYJIxFY+AGvwpTLHN6dhtwInapz/rPET31A3vlGM61/EsrpoZA\n\tKc5EctA3ksxL+1trANCuIjdR5OT/fELUnE3FOtUY=","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","References":"<20210831080528.835236-1-hiroh@chromium.org>","From":"Umang Jain ","Message-ID":"<66f1bc7a-6591-e076-bfad-75e632796669@ideasonboard.com>","Date":"Tue, 31 Aug 2021 17:32:18 +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":"<20210831080528.835236-1-hiroh@chromium.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash in\n\tcalling CameraDevice::close()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19219,"web_url":"https://patchwork.libcamera.org/comment/19219/","msgid":"","date":"2021-08-31T18:26:55","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash in\n\tcalling CameraDevice::close()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for reviewing.\n\nOn Tue, Aug 31, 2021 at 9:02 PM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> The commit message can be somewhat more contextual of the problem.\n>\n> On 8/31/21 1:35 PM, Hirokazu Honda wrote:\n> > This fixes the crash when post processing is needed and some capture\n> > request is being processed by PipelineHandler upon calling\n> > CameraDevice::close().\n> > The crash happens in Request::completeBuffer(), which is invoked by\n> > Camera::stop(). It uses FrameBuffer after free. The FrameBuffer is\n> > allocated and owned by FrameBufferAllocator, and FrameBufferAllocator\n> > is owned by CamerStream. So Camera::stop() should be executed before\n> > destroying CameraStream.\n>\n>\n> One question: I can see the problem but what's exactly is crashing in\n> Request::completeBuffer() is something we should know? Is it the ASSERT?\n> That might help us to define the problem statement better.\n>\n\nIt is buffer->_d()->setRequest(nullptr) in\nhttps://git.linuxtv.org/libcamera.git/tree/src/libcamera/request.cpp#n339\nThe buffer has been deallocated.\n\n> The problem is happening because we seem to add a CameraStream\n> associated buffer(depending on the CameraStream::Type) to the Request,\n> in CameraDevice::processCaptureRequest().\n>\n> However, when the camera stops, all the current buffers are marked with\n> FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> associated with the CameraStream (that was previously added to the\n> request) has now been cleared out with a part of streams_.clear(), even\n> before the camera stop() has been invoked. Any access to those request\n> buffers after they have been cleared, shall result in a crash.\n>\n\nThanks. I replaced my commit message with this.\n\n-Hiro\n>\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 8ca76719..74a95a2a 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -423,10 +423,10 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> >\n> > void CameraDevice::close()\n> > {\n> > - streams_.clear();\n> > -\n> > stop();\n> >\n> > + streams_.clear();\n>\n>\n> How about this being a part of a CameraDevice::stop() instead ?\n>\n> > +\n> > camera_->release();\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 128F8BD87D\n\tfor ;\n\tTue, 31 Aug 2021 18:27:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B2086916A;\n\tTue, 31 Aug 2021 20:27:08 +0200 (CEST)","from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com\n\t[IPv6:2a00:1450:4864:20::62d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9700768890\n\tfor ;\n\tTue, 31 Aug 2021 20:27:06 +0200 (CEST)","by mail-ej1-x62d.google.com with SMTP id me10so707074ejb.11\n\tfor ;\n\tTue, 31 Aug 2021 11:27:06 -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=\"K0nL2awu\"; 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=GVWLudYiDpevIUAnallTCOJfght60mhYIXA6l65Up8k=;\n\tb=K0nL2awufFsdIi3FTedQOaTBChjwokz9qwSZJAX9LlxVrgaR4XlOd3DEKEfxrnRj/L\n\tpXTqCKkYvPbl4SWbehr8hCDII+EoPm3yP0pvAsU5EUsSotFXO0XlgH/P4p6fZfYWZWE+\n\tLcsPovNDVCsgWJdh/n+ljdavWaPNBjbpAf9Pc=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=GVWLudYiDpevIUAnallTCOJfght60mhYIXA6l65Up8k=;\n\tb=fJFY7+EF+lyGp0NvmsrQv7cquYGP61mc6jNJ8b9ujkzgSP5b+HgNVFgszZGam6Whap\n\teY90fG0/qpBC4J6dCRdD183a/tWWukv/dt/Knr6MJwYG/tlnYSmafdeRaqflumg73N2L\n\tub+E8t29d5QfToqD761Y1omKMj0p4xYkrXwURBfHPHU6uxXbaNGos4DR8zrMweXbZ35k\n\ttsMN43x7EX63Rzhcm6byc7WB9tkgOA/+mh0ixIIr0A9ZEByud8O/WNdk8So8wUdwTB7R\n\tLDdHJuk6uynCxjaIL8/FlHagu2bEAEMM/Uq7tDDdAghX+yuyZLYrxjXqpsH6DzQuRz7D\n\trmxQ==","X-Gm-Message-State":"AOAM532UCiopzYxXP0n7RZEnWI9A7iB1ewU5w+SMvjyiqWy7tPXpjdXP\n\tkxQIF4WJd2R4I73FijmgdeigPyZlkB4yiEsDydCj0QBddeTqMQ==","X-Google-Smtp-Source":"ABdhPJw32wridG9GXHvvLyEIT/nUvMOdVZdIAks7W0yX9ipLoALQLwDRvWLqhe8eCKKozithcYnAUMBrXWnkhGC+xPA=","X-Received":"by 2002:a17:907:d86:: with SMTP id\n\tgo6mr2141452ejc.475.1630434426071; \n\tTue, 31 Aug 2021 11:27:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20210831080528.835236-1-hiroh@chromium.org>\n\t<66f1bc7a-6591-e076-bfad-75e632796669@ideasonboard.com>","In-Reply-To":"<66f1bc7a-6591-e076-bfad-75e632796669@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Wed, 1 Sep 2021 03:26:55 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash in\n\tcalling CameraDevice::close()","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\" "}}]