[{"id":15460,"web_url":"https://patchwork.libcamera.org/comment/15460/","msgid":"<YD/S5U1LiDwowg9D@pendragon.ideasonboard.com>","date":"2021-03-03T18:18:13","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: pipeline: ipu3: Ensure\n\tthat IPU3Frames::info is not used after delete","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Wed, Mar 03, 2021 at 05:04:25PM +0000, Kieran Bingham wrote:\n> When the IPU3Frames completes, it deletes the internal info storage.\n> \n> This storage contains the pointer to the Request, but in some cases the\n> pointer was being accessed after the info structure was removed.\n> \n> Ensure that the Request is obtained before attempting to complete to\n> obtain a valid pointer.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n> This may be a further sign that we should rework how this is allocated,\n> but for now - this patch fixes the crash which can occur when shutting\n> down streams.\n\nI agree, it's too easy to get it wrong as shown by the error.\n\n>     The blank line addition in the first hunk is intentional.\n> \n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++++++++++++--\n>  1 file changed, 18 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 2b4d31500533..9539393e5d84 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1164,6 +1164,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n>  \t\t * in action.controls to register additional metadata.\n>  \t\t */\n>  \t\tRequest *request = info->request;\n> +\n>  \t\tinfo->metadataProcessed = true;\n>  \t\tif (frameInfos_.tryComplete(info))\n>  \t\t\tpipe_->completeRequest(request);\n> @@ -1253,8 +1254,15 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>  \t\treturn;\n>  \n>  \tinfo->paramDequeued = true;\n> +\n> +\t/*\n> +\t * tryComplete() will delete info if it completes the IPU3Frame.\n> +\t * In that event, we must have obtained the Request before hand.\n\nMaybe\n\n\t * \\todo Improve the FrameInfo API to avoid this type of issue\n\n?\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +\t */\n> +\tRequest *request = info->request;\n> +\n>  \tif (frameInfos_.tryComplete(info))\n> -\t\tpipe_->completeRequest(info->request);\n> +\t\tpipe_->completeRequest(request);\n>  }\n>  \n>  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> @@ -1265,8 +1273,16 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>  \n>  \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n>  \t\tinfo->metadataProcessed = true;\n> +\n> +\t\t/*\n> +\t\t* tryComplete() will delete info if it completes the IPU3Frame.\n> +\t\t* In that event, we must have obtained the Request before hand.\n> +\t\t*/\n> +\t\tRequest *request = info->request;\n> +\n>  \t\tif (frameInfos_.tryComplete(info))\n> -\t\t\tpipe_->completeRequest(info->request);\n> +\t\t\tpipe_->completeRequest(request);\n> +\n>  \t\treturn;\n>  \t}\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 2FDF5BD80C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  3 Mar 2021 18:18:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A91EC68A91;\n\tWed,  3 Mar 2021 19:18:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97E2B68A7E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  3 Mar 2021 19:18:42 +0100 (CET)","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 123AB8CA;\n\tWed,  3 Mar 2021 19:18:42 +0100 (CET)"],"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=\"Mxx3m5iT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614795522;\n\tbh=sRjA/Iq60cwoTMaCeOScseVo9KIxCXZph+Q6d+aj4Q8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Mxx3m5iTQ8PkHpt+eRhx+La0mv61hZnq7vU8MkswnEsO8tzOQ/WS87i+JnuMTlxgE\n\tdby67sb4f/JYOPP6VYk2wyY0Nl3/AgTZNkrsoA4fKCaWXvYuNvgJh70ucjWgSkNPrP\n\tI+Sy1OdOuUMKzSPXMvS99C/BahMPBRiPKsV+M7so=","Date":"Wed, 3 Mar 2021 20:18:13 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YD/S5U1LiDwowg9D@pendragon.ideasonboard.com>","References":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>\n\t<20210303170426.189648-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210303170426.189648-3-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: pipeline: ipu3: Ensure\n\tthat IPU3Frames::info is not used after delete","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15477,"web_url":"https://patchwork.libcamera.org/comment/15477/","msgid":"<afb0aef7-85a3-f51f-e4f3-77c8a01e7d61@ideasonboard.com>","date":"2021-03-04T09:15:20","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: pipeline: ipu3: Ensure\n\tthat IPU3Frames::info is not used after delete","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 03/03/2021 18:18, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Wed, Mar 03, 2021 at 05:04:25PM +0000, Kieran Bingham wrote:\n>> When the IPU3Frames completes, it deletes the internal info storage.\n>>\n>> This storage contains the pointer to the Request, but in some cases the\n>> pointer was being accessed after the info structure was removed.\n>>\n>> Ensure that the Request is obtained before attempting to complete to\n>> obtain a valid pointer.\n>>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> ---\n>> This may be a further sign that we should rework how this is allocated,\n>> but for now - this patch fixes the crash which can occur when shutting\n>> down streams.\n> \n> I agree, it's too easy to get it wrong as shown by the error.\n> \n>>     The blank line addition in the first hunk is intentional.\n>>\n>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++++++++++++--\n>>  1 file changed, 18 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 2b4d31500533..9539393e5d84 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -1164,6 +1164,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n>>  \t\t * in action.controls to register additional metadata.\n>>  \t\t */\n>>  \t\tRequest *request = info->request;\n>> +\n>>  \t\tinfo->metadataProcessed = true;\n>>  \t\tif (frameInfos_.tryComplete(info))\n>>  \t\t\tpipe_->completeRequest(request);\n>> @@ -1253,8 +1254,15 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>>  \t\treturn;\n>>  \n>>  \tinfo->paramDequeued = true;\n>> +\n>> +\t/*\n>> +\t * tryComplete() will delete info if it completes the IPU3Frame.\n>> +\t * In that event, we must have obtained the Request before hand.\n> \n> Maybe\n> \n> \t * \\todo Improve the FrameInfo API to avoid this type of issue\n> \n> ?\n> \n\nThere's already a todo to rework the allocations. We need to tackle that\nas soon as possible, which to me would include making it better so we\navoid this issue. But this patch fixes the bug as it stands now.\n\nI'll add the todo all the same.\n\n\n\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n>> +\t */\n>> +\tRequest *request = info->request;\n>> +\n>>  \tif (frameInfos_.tryComplete(info))\n>> -\t\tpipe_->completeRequest(info->request);\n>> +\t\tpipe_->completeRequest(request);\n>>  }\n>>  \n>>  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>> @@ -1265,8 +1273,16 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>  \n>>  \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n>>  \t\tinfo->metadataProcessed = true;\n>> +\n>> +\t\t/*\n>> +\t\t* tryComplete() will delete info if it completes the IPU3Frame.\n>> +\t\t* In that event, we must have obtained the Request before hand.\n>> +\t\t*/\n>> +\t\tRequest *request = info->request;\n>> +\n>>  \t\tif (frameInfos_.tryComplete(info))\n>> -\t\t\tpipe_->completeRequest(info->request);\n>> +\t\t\tpipe_->completeRequest(request);\n>> +\n>>  \t\treturn;\n>>  \t}\n>>  \n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6D8CEBD80C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  4 Mar 2021 09:15:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D72DA68A92;\n\tThu,  4 Mar 2021 10:15:25 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D4C49602EC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  4 Mar 2021 10:15:23 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 569E327A;\n\tThu,  4 Mar 2021 10:15:23 +0100 (CET)"],"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=\"qVKj4XRM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614849323;\n\tbh=q4p7zejDdOyH4WpUiPTOZekNX/nRu6lSn7Ad1qfpL+Y=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=qVKj4XRM0R+nKJwtJQOtw1QlcirbzqNMVpBj4NkgYAJk433fW0AtSsJYolLQK+GMZ\n\t+so3Q73sySkMMtWmN5Se8N5ubaMrhum1pb1ie2IcR3TkeZkrqPg6FLHwTMNJezpoA+\n\tR8xEC0KpwFm0llrGPrtyk/adl6dB080H6zIBvj4g=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>\n\t<20210303170426.189648-3-kieran.bingham@ideasonboard.com>\n\t<YD/S5U1LiDwowg9D@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<afb0aef7-85a3-f51f-e4f3-77c8a01e7d61@ideasonboard.com>","Date":"Thu, 4 Mar 2021 09:15:20 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<YD/S5U1LiDwowg9D@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: pipeline: ipu3: Ensure\n\tthat IPU3Frames::info is not used after delete","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15491,"web_url":"https://patchwork.libcamera.org/comment/15491/","msgid":"<20210305195251.sbvmytecel5wucxd@uno.localdomain>","date":"2021-03-05T19:52:51","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: pipeline: ipu3: Ensure\n\tthat IPU3Frames::info is not used after delete","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Wed, Mar 03, 2021 at 05:04:25PM +0000, Kieran Bingham wrote:\n> When the IPU3Frames completes, it deletes the internal info storage.\n>\n> This storage contains the pointer to the Request, but in some cases the\n> pointer was being accessed after the info structure was removed.\n>\n> Ensure that the Request is obtained before attempting to complete to\n> obtain a valid pointer.\n>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>\n> ---\n> This may be a further sign that we should rework how this is allocated,\n> but for now - this patch fixes the crash which can occur when shutting\n> down streams.\n>\n>\n>     The blank line addition in the first hunk is intentional.\n\nYou replied to everything I would have questioned already :)\n\nThe FrameInfo API should be modified before it spreads to other\npipeline handlers.\n\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n  j\n\n>\n>\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++++++++++++--\n>  1 file changed, 18 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 2b4d31500533..9539393e5d84 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1164,6 +1164,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n>  \t\t * in action.controls to register additional metadata.\n>  \t\t */\n>  \t\tRequest *request = info->request;\n> +\n>  \t\tinfo->metadataProcessed = true;\n>  \t\tif (frameInfos_.tryComplete(info))\n>  \t\t\tpipe_->completeRequest(request);\n> @@ -1253,8 +1254,15 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>  \t\treturn;\n>\n>  \tinfo->paramDequeued = true;\n> +\n> +\t/*\n> +\t * tryComplete() will delete info if it completes the IPU3Frame.\n> +\t * In that event, we must have obtained the Request before hand.\n> +\t */\n> +\tRequest *request = info->request;\n> +\n>  \tif (frameInfos_.tryComplete(info))\n> -\t\tpipe_->completeRequest(info->request);\n> +\t\tpipe_->completeRequest(request);\n>  }\n>\n>  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> @@ -1265,8 +1273,16 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>\n>  \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n>  \t\tinfo->metadataProcessed = true;\n> +\n> +\t\t/*\n> +\t\t* tryComplete() will delete info if it completes the IPU3Frame.\n> +\t\t* In that event, we must have obtained the Request before hand.\n> +\t\t*/\n> +\t\tRequest *request = info->request;\n> +\n>  \t\tif (frameInfos_.tryComplete(info))\n> -\t\t\tpipe_->completeRequest(info->request);\n> +\t\t\tpipe_->completeRequest(request);\n> +\n>  \t\treturn;\n>  \t}\n>\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3718BBD80C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  5 Mar 2021 19:52:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFA8B68A9F;\n\tFri,  5 Mar 2021 20:52:25 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E673568A69\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  5 Mar 2021 20:52:24 +0100 (CET)","from uno.localdomain (host-79-22-58-175.retail.telecomitalia.it\n\t[79.22.58.175]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 5B6DAE0002;\n\tFri,  5 Mar 2021 19:52:22 +0000 (UTC)"],"X-Originating-IP":"79.22.58.175","Date":"Fri, 5 Mar 2021 20:52:51 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<20210305195251.sbvmytecel5wucxd@uno.localdomain>","References":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>\n\t<20210303170426.189648-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210303170426.189648-3-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: pipeline: ipu3: Ensure\n\tthat IPU3Frames::info is not used after delete","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]