[{"id":25925,"web_url":"https://patchwork.libcamera.org/comment/25925/","msgid":"<166974355630.3103093.11675119050850192983@Monstersaurus>","date":"2022-11-29T17:39:16","subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)\n> In the simple capture tests, in the capture functions, the Requests were\n> auto-deallocated by the function going out of scope after the test\n> completed. However, before the end of the scope, the Framebuffers that\n> the Requests referred to were manually freed. Thus when the Requests\n> were deallocated, they tried to cancel their Framebuffers, which involve\n> setting the status to cancelled, which obviously causes a segfault\n> because the Framebuffers had already been freed.\n\nThis worries me that we have a path that could let application cause a\nsegfault in libcamera. That's not good.\n\n> Fix this by moving the list of Requests to a member variable and\n> deallocating them before deallocating the Framebuffers at stop() time.\n\nFixing this here seems reasonable, but I'm concerned that we now need a\nunit test in libcamera unit tests which ensure or validate the lifetimes\nof freeing buffers/requests.\n\nIf I understand it, that means that we have a lifetime management issue\nstill between the Request and the Buffer object that may be associated\nwith it.\n\nBug: https://bugs.libcamera.org/show_bug.cgi?id=171\nReviewed-by: Kieran Bingham \nTested-by: Kieran Bingham \n\nBut I am not sure we should consider the underlying issue resolved yet.\nI feel like the crash in lc-compliance is simply telling us we have a\nfault with our API.\n\nThe question is if we should still fix lc-compliance here or not - or if\nwe should keep it 'broken' so that we can validate whatever fix we do\ninternally in libcamera?\n\n--\nKieran\n\n\n> Signed-off-by: Paul Elder \n> ---\n> src/apps/lc-compliance/simple_capture.cpp | 7 +++----\n> src/apps/lc-compliance/simple_capture.h | 1 +\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp\n> index ab5cb35c..cf4d7cf3 100644\n> --- a/src/apps/lc-compliance/simple_capture.cpp\n> +++ b/src/apps/lc-compliance/simple_capture.cpp\n> @@ -65,6 +65,7 @@ void SimpleCapture::stop()\n> camera_->requestCompleted.disconnect(this);\n> \n> Stream *stream = config_->at(0).stream();\n> + requests_.clear();\n> allocator_->free(stream);\n> }\n> \n> @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> captureLimit_ = numRequests;\n> \n> /* Queue the recommended number of reqeuests. */\n> - std::vector> requests;\n> for (const std::unique_ptr &buffer : buffers) {\n> std::unique_ptr request = camera_->createRequest();\n> ASSERT_TRUE(request) << \"Can't create request\";\n> @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> \n> ASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> \n> - requests.push_back(std::move(request));\n> + requests_.push_back(std::move(request));\n> }\n> \n> /* Run capture session. */\n> @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> captureLimit_ = numRequests;\n> \n> /* Queue the recommended number of reqeuests. */\n> - std::vector> requests;\n> for (const std::unique_ptr &buffer : buffers) {\n> std::unique_ptr request = camera_->createRequest();\n> ASSERT_TRUE(request) << \"Can't create request\";\n> @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> \n> ASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> \n> - requests.push_back(std::move(request));\n> + requests_.push_back(std::move(request));\n> }\n> \n> /* Run capture session. */\n> diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h\n> index fd9d2a97..2911d601 100644\n> --- a/src/apps/lc-compliance/simple_capture.h\n> +++ b/src/apps/lc-compliance/simple_capture.h\n> @@ -32,6 +32,7 @@ protected:\n> std::shared_ptr camera_;\n> std::unique_ptr allocator_;\n> std::unique_ptr config_;\n> + std::vector> requests_;\n> };\n> \n> class SimpleCaptureBalanced : public SimpleCapture\n> -- \n> 2.35.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 CB9D6BDE6B\n\tfor ;\n\tTue, 29 Nov 2022 17:39:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0C5C6333F;\n\tTue, 29 Nov 2022 18:39:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 508EF63331\n\tfor ;\n\tTue, 29 Nov 2022 18:39:19 +0100 (CET)","from pendragon.ideasonboard.com\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 C7A0F4E6;\n\tTue, 29 Nov 2022 18:39:18 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669743561;\n\tbh=lWTpiPZFkvn7BJNppJhg6ddZ9mBi4sFlHIrvQIv+SLg=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=jd6ZVlkZHMZeWdvHtCy4uDziemCM46LvcU5GAWhUZf+DSvoxMb8rHG3TIttXSfsTK\n\tC+LTUAYION0Ypr6nXxGMY/ILMaU/QDXLptODs6PWv+C7q0/3ysARrIkYjFF17pgVBL\n\tPuFrPKqyZqOknZ7ELKdv6MREcdwWQzDn+PS4CPDm8ks50dUUdx3WFlG3PWsK1fj966\n\tvBvBzDwqHTK40sAs4BHp7/thxEQrYhCu8mUR63k0uugLwHiY3/a7lppKjyBrQtJc8/\n\top8Y4+UncZuaGZjs9Dam6Yp+yEYNwu47P3EiewNrnX2mzsGHwIznMS98XQK7YQJqkC\n\tYrwqfhHTONj1w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669743558;\n\tbh=lWTpiPZFkvn7BJNppJhg6ddZ9mBi4sFlHIrvQIv+SLg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=ALjvE1PO1O1kQaL0rkwhKGdMreoDpWgxokIwie+ojoe/IklYs/akU7KbUKnzQ4hpn\n\tiwJL4zH/Y6T0jxYMxlGakT3ll3kz/suV7iALa2X+jQDbTycxwaLWwBxgnfAYThXPq0\n\t1uJy1N1UA8aJM5kivc1TOg3Nf7T/MxgGIaZu8t/o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ALjvE1PO\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>","References":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 29 Nov 2022 17:39:16 +0000","Message-ID":"<166974355630.3103093.11675119050850192983@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25926,"web_url":"https://patchwork.libcamera.org/comment/25926/","msgid":"","date":"2022-11-30T10:35:47","subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 11/30/22 1:39 AM, Kieran Bingham via libcamera-devel wrote:\n> Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)\n>> In the simple capture tests, in the capture functions, the Requests were\n>> auto-deallocated by the function going out of scope after the test\n>> completed. However, before the end of the scope, the Framebuffers that\n>> the Requests referred to were manually freed. Thus when the Requests\n>> were deallocated, they tried to cancel their Framebuffers, which involve\n>> setting the status to cancelled, which obviously causes a segfault\n>> because the Framebuffers had already been freed.\n> This worries me that we have a path that could let application cause a\n> segfault in libcamera. That's not good.\n>\n>> Fix this by moving the list of Requests to a member variable and\n>> deallocating them before deallocating the Framebuffers at stop() time.\n> Fixing this here seems reasonable, but I'm concerned that we now need a\n> unit test in libcamera unit tests which ensure or validate the lifetimes\n> of freeing buffers/requests.\n>\n> If I understand it, that means that we have a lifetime management issue\n> still between the Request and the Buffer object that may be associated\n> with it.\n\nIf the buffers to be associated with the Requests are allocated and \nmis-managed by the application themselves - how would that be address by \nlibcamera-core?\n\nI am reading the Request->addBuffer() documentation and clearly mentioned:\n\n  *\n  * A reference to the buffer is stored in the request. The caller is \nresponsible\n  * for ensuring that the buffer will remain valid until the request \ncomplete\n  * callback is called.\n*\n\nIn this case the buffers from manually freed before the request got \ncompleted (via set 'cancelled' status and completeRequest() codepath in \nPIpelineHandler::Stop()). So that's not the right thing to do / happen.\n\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=171\n> Reviewed-by: Kieran Bingham \n> Tested-by: Kieran Bingham \n>\n> But I am not sure we should consider the underlying issue resolved yet.\n> I feel like the crash in lc-compliance is simply telling us we have a\n> fault with our API.\n>\n> The question is if we should still fix lc-compliance here or not - or if\n> we should keep it 'broken' so that we can validate whatever fix we do\n> internally in libcamera?\n>\n> --\n> Kieran\n>\n>\n>> Signed-off-by: Paul Elder \n>> ---\n>> src/apps/lc-compliance/simple_capture.cpp | 7 +++----\n>> src/apps/lc-compliance/simple_capture.h | 1 +\n>> 2 files changed, 4 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp\n>> index ab5cb35c..cf4d7cf3 100644\n>> --- a/src/apps/lc-compliance/simple_capture.cpp\n>> +++ b/src/apps/lc-compliance/simple_capture.cpp\n>> @@ -65,6 +65,7 @@ void SimpleCapture::stop()\n>> camera_->requestCompleted.disconnect(this);\n>> \n>> Stream *stream = config_->at(0).stream();\n>> + requests_.clear();\n>> allocator_->free(stream);\n>> }\n>> \n>> @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n>> captureLimit_ = numRequests;\n>> \n>> /* Queue the recommended number of reqeuests. */\n>> - std::vector> requests;\n>> for (const std::unique_ptr &buffer : buffers) {\n>> std::unique_ptr request = camera_->createRequest();\n>> ASSERT_TRUE(request) << \"Can't create request\";\n>> @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n>> \n>> ASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n>> \n>> - requests.push_back(std::move(request));\n>> + requests_.push_back(std::move(request));\n>> }\n>> \n>> /* Run capture session. */\n>> @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n>> captureLimit_ = numRequests;\n>> \n>> /* Queue the recommended number of reqeuests. */\n>> - std::vector> requests;\n>> for (const std::unique_ptr &buffer : buffers) {\n>> std::unique_ptr request = camera_->createRequest();\n>> ASSERT_TRUE(request) << \"Can't create request\";\n>> @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n>> \n>> ASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n>> \n>> - requests.push_back(std::move(request));\n>> + requests_.push_back(std::move(request));\n>> }\n>> \n>> /* Run capture session. */\n>> diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h\n>> index fd9d2a97..2911d601 100644\n>> --- a/src/apps/lc-compliance/simple_capture.h\n>> +++ b/src/apps/lc-compliance/simple_capture.h\n>> @@ -32,6 +32,7 @@ protected:\n>> std::shared_ptr camera_;\n>> std::unique_ptr allocator_;\n>> std::unique_ptr config_;\n>> + std::vector> requests_;\n>> };\n>> \n>> class SimpleCaptureBalanced : public SimpleCapture\n>> -- \n>> 2.35.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 1120ABE08B\n\tfor ;\n\tWed, 30 Nov 2022 10:36:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E9F86333F;\n\tWed, 30 Nov 2022 11:36:51 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01F0360483\n\tfor ;\n\tWed, 30 Nov 2022 11:36:49 +0100 (CET)","from [192.168.10.186] (unknown [210.186.188.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C87D955A;\n\tWed, 30 Nov 2022 11:36:48 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669804611;\n\tbh=kSOUXN2GNkAbaFcCt1KZHzpGaMswDLeUNndIU/xcHXU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=c87e9U69Y0R5mF8CeMiB+KwG69KuJ6vUjNlhSvdO8m1Ie0h7hPJkXQkCbHE8PdS+o\n\txOoxCwmzH3NFG+L2q17rn8ZebpPsxHEDCxcnk/G5Emc/EohYtRP/cnRpUerzmGDuO9\n\tpWuAZMEob3BpiUl1cghab0To9NX/Mhm51KD8tAuf6+HXn4OjZIqCjvGAexQxg/FrOK\n\t4nlJ55KhacVFlvma/gtO59DmzoPyJWMTp8ZXkVmuz9D9FDj4ajhZQqKfbM2V2dJFJs\n\tdqMwlLjYh8SpODG3F9rN5ymcuiZemoKXIbeHTktOeeIYFEyJVAG4PhPaDXbd5opBA9\n\t34m4kHQa2k6lg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669804609;\n\tbh=kSOUXN2GNkAbaFcCt1KZHzpGaMswDLeUNndIU/xcHXU=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=XsgSSA2bImxZVDq850LAVNBtru6F+4cVczomlQoJzJ0S5D0m2m3CoHFFo/SQrK0nd\n\tm4JIWq+tq/JJvbL1pIokbl2/49w/uY+3LJ+tL0giG15zE32kpFr9FznTwogu3eo6HO\n\tXKaruzqowRd3Xd5psx0GSo9S+oKV5HiYu5mYgH48="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XsgSSA2b\"; dkim-atps=neutral","Message-ID":"","Date":"Wed, 30 Nov 2022 18:35:47 +0800","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.5.0","To":"Kieran Bingham ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>\n\t<166974355630.3103093.11675119050850192983@Monstersaurus>","Content-Language":"en-US","In-Reply-To":"<166974355630.3103093.11675119050850192983@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25927,"web_url":"https://patchwork.libcamera.org/comment/25927/","msgid":"<166980559040.1079859.15872634788506413846@Monstersaurus>","date":"2022-11-30T10:53:10","subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2022-11-30 10:35:47)\n> Hi Kieran,\n> \n> On 11/30/22 1:39 AM, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)\n> >> In the simple capture tests, in the capture functions, the Requests were\n> >> auto-deallocated by the function going out of scope after the test\n> >> completed. However, before the end of the scope, the Framebuffers that\n> >> the Requests referred to were manually freed. Thus when the Requests\n> >> were deallocated, they tried to cancel their Framebuffers, which involve\n> >> setting the status to cancelled, which obviously causes a segfault\n> >> because the Framebuffers had already been freed.\n> > This worries me that we have a path that could let application cause a\n> > segfault in libcamera. That's not good.\n> >\n> >> Fix this by moving the list of Requests to a member variable and\n> >> deallocating them before deallocating the Framebuffers at stop() time.\n> > Fixing this here seems reasonable, but I'm concerned that we now need a\n> > unit test in libcamera unit tests which ensure or validate the lifetimes\n> > of freeing buffers/requests.\n> >\n> > If I understand it, that means that we have a lifetime management issue\n> > still between the Request and the Buffer object that may be associated\n> > with it.\n> \n> If the buffers to be associated with the Requests are allocated and \n> mis-managed by the application themselves - how would that be address by \n> libcamera-core?\n> \n> I am reading the Request->addBuffer() documentation and clearly mentioned:\n> \n>  *\n>  * A reference to the buffer is stored in the request. The caller is \n> responsible\n>  * for ensuring that the buffer will remain valid until the request \n> complete\n>  * callback is called.\n> *\n> \n> In this case the buffers from manually freed before the request got \n> completed (via set 'cancelled' status and completeRequest() codepath in \n> PIpelineHandler::Stop()). So that's not the right thing to do / happen.\n\naha, ok - if it's documented, then we're probably good with this for\nnow.\n\nI do wonder if we should have some reverse association such that when\nyou add a buffer to a request, the buffer knows which request it's\ncontained in, and can 'remove' itself when it's freed... but ... that\nmakes me wonder if we could legitimately have a single buffer in\nmultiple requests which would break that possibility ... so maybe we're\nfine with just the documentation as we have it.\n\n--\nKieran\n\n\n> \n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=171\n> > Reviewed-by: Kieran Bingham \n> > Tested-by: Kieran Bingham \n> >\n> > But I am not sure we should consider the underlying issue resolved yet.\n> > I feel like the crash in lc-compliance is simply telling us we have a\n> > fault with our API.\n> >\n> > The question is if we should still fix lc-compliance here or not - or if\n> > we should keep it 'broken' so that we can validate whatever fix we do\n> > internally in libcamera?\n> >\n> > --\n> > Kieran\n> >\n> >\n> >> Signed-off-by: Paul Elder \n> >> ---\n> >> src/apps/lc-compliance/simple_capture.cpp | 7 +++----\n> >> src/apps/lc-compliance/simple_capture.h | 1 +\n> >> 2 files changed, 4 insertions(+), 4 deletions(-)\n> >>\n> >> diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp\n> >> index ab5cb35c..cf4d7cf3 100644\n> >> --- a/src/apps/lc-compliance/simple_capture.cpp\n> >> +++ b/src/apps/lc-compliance/simple_capture.cpp\n> >> @@ -65,6 +65,7 @@ void SimpleCapture::stop()\n> >> camera_->requestCompleted.disconnect(this);\n> >> \n> >> Stream *stream = config_->at(0).stream();\n> >> + requests_.clear();\n> >> allocator_->free(stream);\n> >> }\n> >> \n> >> @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> >> captureLimit_ = numRequests;\n> >> \n> >> /* Queue the recommended number of reqeuests. */\n> >> - std::vector> requests;\n> >> for (const std::unique_ptr &buffer : buffers) {\n> >> std::unique_ptr request = camera_->createRequest();\n> >> ASSERT_TRUE(request) << \"Can't create request\";\n> >> @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> >> \n> >> ASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> >> \n> >> - requests.push_back(std::move(request));\n> >> + requests_.push_back(std::move(request));\n> >> }\n> >> \n> >> /* Run capture session. */\n> >> @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> >> captureLimit_ = numRequests;\n> >> \n> >> /* Queue the recommended number of reqeuests. */\n> >> - std::vector> requests;\n> >> for (const std::unique_ptr &buffer : buffers) {\n> >> std::unique_ptr request = camera_->createRequest();\n> >> ASSERT_TRUE(request) << \"Can't create request\";\n> >> @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> >> \n> >> ASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> >> \n> >> - requests.push_back(std::move(request));\n> >> + requests_.push_back(std::move(request));\n> >> }\n> >> \n> >> /* Run capture session. */\n> >> diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h\n> >> index fd9d2a97..2911d601 100644\n> >> --- a/src/apps/lc-compliance/simple_capture.h\n> >> +++ b/src/apps/lc-compliance/simple_capture.h\n> >> @@ -32,6 +32,7 @@ protected:\n> >> std::shared_ptr camera_;\n> >> std::unique_ptr allocator_;\n> >> std::unique_ptr config_;\n> >> + std::vector> requests_;\n> >> };\n> >> \n> >> class SimpleCaptureBalanced : public SimpleCapture\n> >> -- \n> >> 2.35.1\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 85252BDE6B\n\tfor ;\n\tWed, 30 Nov 2022 10:53:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4900763341;\n\tWed, 30 Nov 2022 11:53:14 +0100 (CET)","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 DE21163339\n\tfor ;\n\tWed, 30 Nov 2022 11:53:12 +0100 (CET)","from pendragon.ideasonboard.com\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 42E6C55A;\n\tWed, 30 Nov 2022 11:53:12 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669805594;\n\tbh=EnT6Xx7dOQSAG26fK41xdOqujVXrGiiNw8qitjkwd4Q=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=wi7zzl43u27FndY6VBb5aTd74ma99slPHomRUOMTm80cdXch8tYQ94VQPaBFWP6TR\n\tXK7Je2IlVWrh0c4e5bKIyQD9wHEapHVQFGL1Tmm+IlKJYzx/yW8WMSqa5fVQcNKWYY\n\touqAyKvFH3Y5b3wBBTriIp/rxaED4Ki3pn6uC08bAtHRBdklqYynOs9Bw0Ujf8QwNF\n\tQM8rRinceXaRZF8Q0eQc9IPV5S7ltkSsyr7qAiD0ktV0l3lJ+cmHjNQ9PxS0wJR4S0\n\tAUIdjfEPF/djS4PViLH/HfNFPU2xzO6b6vZjG2G+HFjVj2PPemPYCpMIGbAVSrA+Ka\n\tEpF7uzvoAPXuA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669805592;\n\tbh=EnT6Xx7dOQSAG26fK41xdOqujVXrGiiNw8qitjkwd4Q=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=RIwj/D4ISmSr3QvmoSAl1ubbTkirovZxclHcnnbULA3xY+fAIt0Il1mtnbqrqF1Fl\n\tD22pgiTtTR+bMK6Aw+Uy6aBVB7dcVjpu8k14EqZidCRXUjrdL1Op3hEG9Ult3+G2Nb\n\t2g8nBgnDKkg8KH01yyR1jAJ7OdSKd0hmpPnDc/ec="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RIwj/D4I\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>\n\t<166974355630.3103093.11675119050850192983@Monstersaurus>\n\t","To":"Paul Elder ,\n\tUmang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 30 Nov 2022 10:53:10 +0000","Message-ID":"<166980559040.1079859.15872634788506413846@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25934,"web_url":"https://patchwork.libcamera.org/comment/25934/","msgid":"<2f0d8883-4c43-29cb-5495-687c045c78f4@ideasonboard.com>","date":"2022-11-30T11:24:52","subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 11/30/22 6:53 PM, Kieran Bingham wrote:\n> Quoting Umang Jain (2022-11-30 10:35:47)\n>> Hi Kieran,\n>>\n>> On 11/30/22 1:39 AM, Kieran Bingham via libcamera-devel wrote:\n>>> Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)\n>>>> In the simple capture tests, in the capture functions, the Requests were\n>>>> auto-deallocated by the function going out of scope after the test\n>>>> completed. However, before the end of the scope, the Framebuffers that\n>>>> the Requests referred to were manually freed. Thus when the Requests\n>>>> were deallocated, they tried to cancel their Framebuffers, which involve\n>>>> setting the status to cancelled, which obviously causes a segfault\n>>>> because the Framebuffers had already been freed.\n>>> This worries me that we have a path that could let application cause a\n>>> segfault in libcamera. That's not good.\n>>>\n>>>> Fix this by moving the list of Requests to a member variable and\n>>>> deallocating them before deallocating the Framebuffers at stop() time.\n>>> Fixing this here seems reasonable, but I'm concerned that we now need a\n>>> unit test in libcamera unit tests which ensure or validate the lifetimes\n>>> of freeing buffers/requests.\n>>>\n>>> If I understand it, that means that we have a lifetime management issue\n>>> still between the Request and the Buffer object that may be associated\n>>> with it.\n>> If the buffers to be associated with the Requests are allocated and\n>> mis-managed by the application themselves - how would that be address by\n>> libcamera-core?\n>>\n>> I am reading the Request->addBuffer() documentation and clearly mentioned:\n>>\n>>  *\n>>  * A reference to the buffer is stored in the request. The caller is\n>> responsible\n>>  * for ensuring that the buffer will remain valid until the request\n>> complete\n>>  * callback is called.\n>> *\n>>\n>> In this case the buffers from manually freed before the request got\n>> completed (via set 'cancelled' status and completeRequest() codepath in\n>> PIpelineHandler::Stop()). So that's not the right thing to do / happen.\n> aha, ok - if it's documented, then we're probably good with this for\n> now.\n>\n> I do wonder if we should have some reverse association such that when\n> you add a buffer to a request, the buffer knows which request it's\n> contained in, and can 'remove' itself when it's freed... but ... that\n> makes me wonder if we could legitimately have a single buffer in\n> multiple requests which would break that possibility ... so maybe we're\n> fine with just the documentation as we have it.\n\nWe can enforce a std::shared_ptr <> model (but that's also not 100% \nsafety) but not sure how well it'll scale with language bindings in the \nfuture.\n\nAll in all, similar to dangling pointers problem.\n>\n> --\n> Kieran\n>\n>\n>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=171\n>>> Reviewed-by: Kieran Bingham \n>>> Tested-by: Kieran Bingham \n>>>\n>>> But I am not sure we should consider the underlying issue resolved yet.\n>>> I feel like the crash in lc-compliance is simply telling us we have a\n>>> fault with our API.\n>>>\n>>> The question is if we should still fix lc-compliance here or not - or if\n>>> we should keep it 'broken' so that we can validate whatever fix we do\n>>> internally in libcamera?\n>>>\n>>> --\n>>> Kieran\n>>>\n>>>\n>>>> Signed-off-by: Paul Elder \n>>>> ---\n>>>> src/apps/lc-compliance/simple_capture.cpp | 7 +++----\n>>>> src/apps/lc-compliance/simple_capture.h | 1 +\n>>>> 2 files changed, 4 insertions(+), 4 deletions(-)\n>>>>\n>>>> diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp\n>>>> index ab5cb35c..cf4d7cf3 100644\n>>>> --- a/src/apps/lc-compliance/simple_capture.cpp\n>>>> +++ b/src/apps/lc-compliance/simple_capture.cpp\n>>>> @@ -65,6 +65,7 @@ void SimpleCapture::stop()\n>>>> camera_->requestCompleted.disconnect(this);\n>>>> \n>>>> Stream *stream = config_->at(0).stream();\n>>>> + requests_.clear();\n>>>> allocator_->free(stream);\n>>>> }\n>>>> \n>>>> @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n>>>> captureLimit_ = numRequests;\n>>>> \n>>>> /* Queue the recommended number of reqeuests. */\n>>>> - std::vector> requests;\n>>>> for (const std::unique_ptr &buffer : buffers) {\n>>>> std::unique_ptr request = camera_->createRequest();\n>>>> ASSERT_TRUE(request) << \"Can't create request\";\n>>>> @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n>>>> \n>>>> ASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n>>>> \n>>>> - requests.push_back(std::move(request));\n>>>> + requests_.push_back(std::move(request));\n>>>> }\n>>>> \n>>>> /* Run capture session. */\n>>>> @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n>>>> captureLimit_ = numRequests;\n>>>> \n>>>> /* Queue the recommended number of reqeuests. */\n>>>> - std::vector> requests;\n>>>> for (const std::unique_ptr &buffer : buffers) {\n>>>> std::unique_ptr request = camera_->createRequest();\n>>>> ASSERT_TRUE(request) << \"Can't create request\";\n>>>> @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n>>>> \n>>>> ASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n>>>> \n>>>> - requests.push_back(std::move(request));\n>>>> + requests_.push_back(std::move(request));\n>>>> }\n>>>> \n>>>> /* Run capture session. */\n>>>> diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h\n>>>> index fd9d2a97..2911d601 100644\n>>>> --- a/src/apps/lc-compliance/simple_capture.h\n>>>> +++ b/src/apps/lc-compliance/simple_capture.h\n>>>> @@ -32,6 +32,7 @@ protected:\n>>>> std::shared_ptr camera_;\n>>>> std::unique_ptr allocator_;\n>>>> std::unique_ptr config_;\n>>>> + std::vector> requests_;\n>>>> };\n>>>> \n>>>> class SimpleCaptureBalanced : public SimpleCapture\n>>>> -- \n>>>> 2.35.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 6651FBDE6B\n\tfor ;\n\tWed, 30 Nov 2022 11:25:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB5FD63336;\n\tWed, 30 Nov 2022 12:25:56 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DBDD361F23\n\tfor ;\n\tWed, 30 Nov 2022 12:25:55 +0100 (CET)","from [192.168.10.186] (unknown [210.186.188.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B334A13EA;\n\tWed, 30 Nov 2022 12:25:54 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669807556;\n\tbh=FJHTWFTC2ateSelt6vcz2YKMnupL8UyBTaPbST4DtLQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=z2TqVrJ6cuNQPssmfEwJG6FlF46n2vySvU1QuY6+Q42Z44ycgvVBmlofjTWVZ3plu\n\trAD9cg8twgbRgpc2Mvb2la2w79kuZut7wD0LpTLr/qvgcej9ZQGdqIY3dZpu8oqsGT\n\tFrs0y2Gbcha4HtZdgxu9At8zGoxoGX1GTaAAZtlxv+K/0Ts6T3AE5cNJftndhHiUeL\n\tNLyZ+FefvgasRFGdXF1bcCeBSclzz5qoIkfzJp3iPO/ms9aWVt2cWAUHjxK2oOMqug\n\te4o82I2LRP0CqNifKg4OMyW/F5O7JWiXltXF++XE8HyM1IaPCUtf7pLJFJcHSN6hyr\n\t/eD5siiqzqdTw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669807555;\n\tbh=FJHTWFTC2ateSelt6vcz2YKMnupL8UyBTaPbST4DtLQ=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=oXmJjeq44zUX2b+grhNLDVhENymQLvYFwsJl3r/OdxQheJJFYzn0o1JHf7dWv+h39\n\tCOtH2Q2bI0CwRdE+sqBBDyuxoSk+s2o3cp/3FL1gtxUPD1KOg4VbfE4ZxJRIjq3y0Y\n\toqlYF6bCdIv3f+FupYMOva4kJJciZ7DnzGjMMjVE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oXmJjeq4\"; dkim-atps=neutral","Message-ID":"<2f0d8883-4c43-29cb-5495-687c045c78f4@ideasonboard.com>","Date":"Wed, 30 Nov 2022 19:24:52 +0800","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.5.0","Content-Language":"en-US","To":"Kieran Bingham ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>\n\t<166974355630.3103093.11675119050850192983@Monstersaurus>\n\t\n\t<166980559040.1079859.15872634788506413846@Monstersaurus>","In-Reply-To":"<166980559040.1079859.15872634788506413846@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25935,"web_url":"https://patchwork.libcamera.org/comment/25935/","msgid":"","date":"2022-11-30T11:27:40","subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Tue, Nov 29, 2022 at 05:39:16PM +0000, Kieran Bingham wrote:\n> Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)\n> > In the simple capture tests, in the capture functions, the Requests were\n> > auto-deallocated by the function going out of scope after the test\n> > completed. However, before the end of the scope, the Framebuffers that\n> > the Requests referred to were manually freed. Thus when the Requests\n> > were deallocated, they tried to cancel their Framebuffers, which involve\n> > setting the status to cancelled, which obviously causes a segfault\n> > because the Framebuffers had already been freed.\n> \n> This worries me that we have a path that could let application cause a\n> segfault in libcamera. That's not good.\n\nThat's true.\n\nShould we fix this instead in Request then? By checking that\nFramBuffer::_d() == nullptr in Request::Private::doCancelRequest() like\nrsglobal does in the github issue [1]?\n\n[1] https://github.com/kbingham/libcamera/issues/60#issue-1450103563\n\n> \n> > Fix this by moving the list of Requests to a member variable and\n> > deallocating them before deallocating the Framebuffers at stop() time.\n> \n> Fixing this here seems reasonable, but I'm concerned that we now need a\n> unit test in libcamera unit tests which ensure or validate the lifetimes\n> of freeing buffers/requests.\n\nYeah that's true, this should probably be split into another compliance\ntest in itself.\n\n> \n> If I understand it, that means that we have a lifetime management issue\n> still between the Request and the Buffer object that may be associated\n> with it.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=171\n> Reviewed-by: Kieran Bingham \n> Tested-by: Kieran Bingham \n> \n> But I am not sure we should consider the underlying issue resolved yet.\n> I feel like the crash in lc-compliance is simply telling us we have a\n> fault with our API.\n> \n> The question is if we should still fix lc-compliance here or not - or if\n> we should keep it 'broken' so that we can validate whatever fix we do\n> internally in libcamera?\n\nI think split this specific issue into a separate smaller test. I\ndon't think there's a need to test this specific issue in all\ncombinations that we test currently in the simple capture test.\n\n\nThanks,\n\nPaul\n\n> \n> \n> > Signed-off-by: Paul Elder \n> > ---\n> > src/apps/lc-compliance/simple_capture.cpp | 7 +++----\n> > src/apps/lc-compliance/simple_capture.h | 1 +\n> > 2 files changed, 4 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp\n> > index ab5cb35c..cf4d7cf3 100644\n> > --- a/src/apps/lc-compliance/simple_capture.cpp\n> > +++ b/src/apps/lc-compliance/simple_capture.cpp\n> > @@ -65,6 +65,7 @@ void SimpleCapture::stop()\n> > camera_->requestCompleted.disconnect(this);\n> > \n> > Stream *stream = config_->at(0).stream();\n> > + requests_.clear();\n> > allocator_->free(stream);\n> > }\n> > \n> > @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > captureLimit_ = numRequests;\n> > \n> > /* Queue the recommended number of reqeuests. */\n> > - std::vector> requests;\n> > for (const std::unique_ptr &buffer : buffers) {\n> > std::unique_ptr request = camera_->createRequest();\n> > ASSERT_TRUE(request) << \"Can't create request\";\n> > @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > \n> > ASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > \n> > - requests.push_back(std::move(request));\n> > + requests_.push_back(std::move(request));\n> > }\n> > \n> > /* Run capture session. */\n> > @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > captureLimit_ = numRequests;\n> > \n> > /* Queue the recommended number of reqeuests. */\n> > - std::vector> requests;\n> > for (const std::unique_ptr &buffer : buffers) {\n> > std::unique_ptr request = camera_->createRequest();\n> > ASSERT_TRUE(request) << \"Can't create request\";\n> > @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > \n> > ASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > \n> > - requests.push_back(std::move(request));\n> > + requests_.push_back(std::move(request));\n> > }\n> > \n> > /* Run capture session. */\n> > diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h\n> > index fd9d2a97..2911d601 100644\n> > --- a/src/apps/lc-compliance/simple_capture.h\n> > +++ b/src/apps/lc-compliance/simple_capture.h\n> > @@ -32,6 +32,7 @@ protected:\n> > std::shared_ptr camera_;\n> > std::unique_ptr allocator_;\n> > std::unique_ptr config_;\n> > + std::vector> requests_;\n> > };\n> > \n> > class SimpleCaptureBalanced : public SimpleCapture\n> > -- \n> > 2.35.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 713D7BDE6B\n\tfor ;\n\tWed, 30 Nov 2022 11:27:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF7A563336;\n\tWed, 30 Nov 2022 12:27:47 +0100 (CET)","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 E50C561F23\n\tfor ;\n\tWed, 30 Nov 2022 12:27:46 +0100 (CET)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8FB1113EA;\n\tWed, 30 Nov 2022 12:27:45 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669807667;\n\tbh=fmGbnNeuEDs34M3mqBv+GIR4H21VLrlFF+sbNSoEpi4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=eKIeJUPG4lFJ9EeYcQBP4La4xOXF2peUIYmb/oY/lbZhAHfYER1lRBDfbQAXjWjQM\n\twjpVXFFb+oUOXppe86rQtwVUcAoJUQ62LJ1sUHO91f3oycVyMazkBcwV725+Y/dS3m\n\tgz8xAULsANFtb67yiVv4OhdD4Kz/c9nRPGtINzN+2oPSrcmKH+2h5XJAdMjsjTIYUc\n\tBMK+UIg/VW6yfsJKHEdhtJroZpt/8buuGIfCqRvuNVRHZwjow26zj20kfbmLyE7bx5\n\tO804Ou4Q5BZHU1EA19jW50J1CcEVv7DdUoMq1xSGn5+v9Cwuq9svn37gsmuQdTiNUv\n\tVcwMCe2y+0bxA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669807666;\n\tbh=fmGbnNeuEDs34M3mqBv+GIR4H21VLrlFF+sbNSoEpi4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OyQcROKVLU51y/6ScunaUcmuPm+K2W8BdTIP64zT6/RDPiYUWc+XJUlnKdeat8v0m\n\t7Qwuie4fiYnsLyHsKIdE5cBZb/y/KVOdljSfNarZl92N25q8vb3dJFCe4AmBa1hCtB\n\toFtHDFhhDhxSTh17G/3+1vOjOf+928O3d0xnBCk0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"OyQcROKV\"; dkim-atps=neutral","Date":"Wed, 30 Nov 2022 20:27:40 +0900","To":"Kieran Bingham ","Message-ID":"","References":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>\n\t<166974355630.3103093.11675119050850192983@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<166974355630.3103093.11675119050850192983@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","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","From":"Paul Elder via libcamera-devel ","Reply-To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25936,"web_url":"https://patchwork.libcamera.org/comment/25936/","msgid":"","date":"2022-11-30T11:31:06","subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Nov 30, 2022 at 07:24:52PM +0800, Umang Jain wrote:\n> Hi Kieran,\n> \n> On 11/30/22 6:53 PM, Kieran Bingham wrote:\n> > Quoting Umang Jain (2022-11-30 10:35:47)\n> > > Hi Kieran,\n> > > \n> > > On 11/30/22 1:39 AM, Kieran Bingham via libcamera-devel wrote:\n> > > > Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)\n> > > > > In the simple capture tests, in the capture functions, the Requests were\n> > > > > auto-deallocated by the function going out of scope after the test\n> > > > > completed. However, before the end of the scope, the Framebuffers that\n> > > > > the Requests referred to were manually freed. Thus when the Requests\n> > > > > were deallocated, they tried to cancel their Framebuffers, which involve\n> > > > > setting the status to cancelled, which obviously causes a segfault\n> > > > > because the Framebuffers had already been freed.\n> > > > This worries me that we have a path that could let application cause a\n> > > > segfault in libcamera. That's not good.\n> > > > \n> > > > > Fix this by moving the list of Requests to a member variable and\n> > > > > deallocating them before deallocating the Framebuffers at stop() time.\n> > > > Fixing this here seems reasonable, but I'm concerned that we now need a\n> > > > unit test in libcamera unit tests which ensure or validate the lifetimes\n> > > > of freeing buffers/requests.\n> > > > \n> > > > If I understand it, that means that we have a lifetime management issue\n> > > > still between the Request and the Buffer object that may be associated\n> > > > with it.\n> > > If the buffers to be associated with the Requests are allocated and\n> > > mis-managed by the application themselves - how would that be address by\n> > > libcamera-core?\n> > > \n> > > I am reading the Request->addBuffer() documentation and clearly mentioned:\n> > > \n> > >  *\n> > >  * A reference to the buffer is stored in the request. The caller is\n> > > responsible\n> > >  * for ensuring that the buffer will remain valid until the request\n> > > complete\n> > >  * callback is called.\n> > > *\n> > > \n> > > In this case the buffers from manually freed before the request got\n> > > completed (via set 'cancelled' status and completeRequest() codepath in\n> > > PIpelineHandler::Stop()). So that's not the right thing to do / happen.\n> > aha, ok - if it's documented, then we're probably good with this for\n> > now.\n> > \n> > I do wonder if we should have some reverse association such that when\n> > you add a buffer to a request, the buffer knows which request it's\n> > contained in, and can 'remove' itself when it's freed... but ... that\n> > makes me wonder if we could legitimately have a single buffer in\n> > multiple requests which would break that possibility ... so maybe we're\n> > fine with just the documentation as we have it.\n> \n> We can enforce a std::shared_ptr <> model (but that's also not 100% safety)\n> but not sure how well it'll scale with language bindings in the future.\n> \n> All in all, similar to dangling pointers problem.\n\nOops, I didn't notice this thread in the meantime.\n\nSo this patch is good on its own and I don't have to do anything extra\nthen? :)\n\n\nPaul\n\n> > \n> > --\n> > Kieran\n> > \n> > \n> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=171\n> > > > Reviewed-by: Kieran Bingham \n> > > > Tested-by: Kieran Bingham \n> > > > \n> > > > But I am not sure we should consider the underlying issue resolved yet.\n> > > > I feel like the crash in lc-compliance is simply telling us we have a\n> > > > fault with our API.\n> > > > \n> > > > The question is if we should still fix lc-compliance here or not - or if\n> > > > we should keep it 'broken' so that we can validate whatever fix we do\n> > > > internally in libcamera?\n> > > > \n> > > > --\n> > > > Kieran\n> > > > \n> > > > \n> > > > > Signed-off-by: Paul Elder \n> > > > > ---\n> > > > > src/apps/lc-compliance/simple_capture.cpp | 7 +++----\n> > > > > src/apps/lc-compliance/simple_capture.h | 1 +\n> > > > > 2 files changed, 4 insertions(+), 4 deletions(-)\n> > > > > \n> > > > > diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp\n> > > > > index ab5cb35c..cf4d7cf3 100644\n> > > > > --- a/src/apps/lc-compliance/simple_capture.cpp\n> > > > > +++ b/src/apps/lc-compliance/simple_capture.cpp\n> > > > > @@ -65,6 +65,7 @@ void SimpleCapture::stop()\n> > > > > camera_->requestCompleted.disconnect(this);\n> > > > > Stream *stream = config_->at(0).stream();\n> > > > > + requests_.clear();\n> > > > > allocator_->free(stream);\n> > > > > }\n> > > > > @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > > > captureLimit_ = numRequests;\n> > > > > /* Queue the recommended number of reqeuests. */\n> > > > > - std::vector> requests;\n> > > > > for (const std::unique_ptr &buffer : buffers) {\n> > > > > std::unique_ptr request = camera_->createRequest();\n> > > > > ASSERT_TRUE(request) << \"Can't create request\";\n> > > > > @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > > > ASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > > > > - requests.push_back(std::move(request));\n> > > > > + requests_.push_back(std::move(request));\n> > > > > }\n> > > > > /* Run capture session. */\n> > > > > @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > > > > captureLimit_ = numRequests;\n> > > > > /* Queue the recommended number of reqeuests. */\n> > > > > - std::vector> requests;\n> > > > > for (const std::unique_ptr &buffer : buffers) {\n> > > > > std::unique_ptr request = camera_->createRequest();\n> > > > > ASSERT_TRUE(request) << \"Can't create request\";\n> > > > > @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > > > > ASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > > > > - requests.push_back(std::move(request));\n> > > > > + requests_.push_back(std::move(request));\n> > > > > }\n> > > > > /* Run capture session. */\n> > > > > diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h\n> > > > > index fd9d2a97..2911d601 100644\n> > > > > --- a/src/apps/lc-compliance/simple_capture.h\n> > > > > +++ b/src/apps/lc-compliance/simple_capture.h\n> > > > > @@ -32,6 +32,7 @@ protected:\n> > > > > std::shared_ptr camera_;\n> > > > > std::unique_ptr allocator_;\n> > > > > std::unique_ptr config_;\n> > > > > + std::vector> requests_;\n> > > > > };\n> > > > > class SimpleCaptureBalanced : public SimpleCapture\n> > > > > -- \n> > > > > 2.35.1\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 4E74ABE08B\n\tfor ;\n\tWed, 30 Nov 2022 11:31:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D691F61F24;\n\tWed, 30 Nov 2022 12:31:14 +0100 (CET)","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 0BC1C61F23\n\tfor ;\n\tWed, 30 Nov 2022 12:31:13 +0100 (CET)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C29F6D6;\n\tWed, 30 Nov 2022 12:31:11 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669807874;\n\tbh=cvj1sK6AGv6yikf7cgQyUxYHO+pgtblkpScoZJkwYZk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yzZx02joXV8Zt55kG6++7MEWJzcVvWcdNAqmw9TSPtL2i5qNZdX1Q5C87D1C49zKb\n\tPupw9NbTX/1RGV0t20DcvUvaL/4gDxubRWUIIcCbs3Vk6V8+soZdWm8aBqW+2JCzz0\n\tkaqY5/l+1yhOvZfyNZVYP3lGZDESrjLuq9UiJRogen7sR+c+nhhqJZiJiqe0n6aMHr\n\tHk+2u4/9YGWcrxg6WuSbphVByjqKVg2Viphs80H0ZNVP8/fJ3S63kOwMYC8ux105QA\n\t2fIz9gWQAvJJriR9wgLD3fF2WxP71hkmodqJj2xUkpXjYy3dFjd6BeagLw5zqKaW+K\n\tFgEr1aYqxokVg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669807872;\n\tbh=cvj1sK6AGv6yikf7cgQyUxYHO+pgtblkpScoZJkwYZk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tUrQ/U8stAKHPIShsjfOWWYYDmh0cg+jWywUa72mTl+z8nGjF8gEWzvQgyKxGAdl9\n\t/RWPoSm46YLXwaYkHTIGOWyVvy+QHMleFS0ni8UbmPccJHC/YyynK5d0zKafzkpLrm\n\tKkl7T+W+3Y9dg0Q7vdTY8oTSUMhcoKZDTOWIAzTM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tUrQ/U8s\"; dkim-atps=neutral","Date":"Wed, 30 Nov 2022 20:31:06 +0900","To":"Umang Jain ","Message-ID":"","References":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>\n\t<166974355630.3103093.11675119050850192983@Monstersaurus>\n\t\n\t<166980559040.1079859.15872634788506413846@Monstersaurus>\n\t<2f0d8883-4c43-29cb-5495-687c045c78f4@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<2f0d8883-4c43-29cb-5495-687c045c78f4@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","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","From":"Paul Elder via libcamera-devel ","Reply-To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25937,"web_url":"https://patchwork.libcamera.org/comment/25937/","msgid":"<166980811282.1079859.1901288610102904095@Monstersaurus>","date":"2022-11-30T11:35:12","subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2022-11-30 11:27:40)\n> On Tue, Nov 29, 2022 at 05:39:16PM +0000, Kieran Bingham wrote:\n> > Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)\n> > > In the simple capture tests, in the capture functions, the Requests were\n> > > auto-deallocated by the function going out of scope after the test\n> > > completed. However, before the end of the scope, the Framebuffers that\n> > > the Requests referred to were manually freed. Thus when the Requests\n> > > were deallocated, they tried to cancel their Framebuffers, which involve\n> > > setting the status to cancelled, which obviously causes a segfault\n> > > because the Framebuffers had already been freed.\n> > \n> > This worries me that we have a path that could let application cause a\n> > segfault in libcamera. That's not good.\n> \n> That's true.\n> \n> Should we fix this instead in Request then? By checking that\n> FramBuffer::_d() == nullptr in Request::Private::doCancelRequest() like\n> rsglobal does in the github issue [1]?\n\nAs this is clearly a situation that applciations could get themselves into -\nperhaps we need to have 'something' but with a clear message to tellng\nthem they've messed up ?\n\n\n \n> [1] https://github.com/kbingham/libcamera/issues/60#issue-1450103563\n> \n> > \n> > > Fix this by moving the list of Requests to a member variable and\n> > > deallocating them before deallocating the Framebuffers at stop() time.\n> > \n> > Fixing this here seems reasonable, but I'm concerned that we now need a\n> > unit test in libcamera unit tests which ensure or validate the lifetimes\n> > of freeing buffers/requests.\n> \n> Yeah that's true, this should probably be split into another compliance\n> test in itself.\n> \n> > \n> > If I understand it, that means that we have a lifetime management issue\n> > still between the Request and the Buffer object that may be associated\n> > with it.\n> > \n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=171\n> > Reviewed-by: Kieran Bingham \n> > Tested-by: Kieran Bingham \n> > \n> > But I am not sure we should consider the underlying issue resolved yet.\n> > I feel like the crash in lc-compliance is simply telling us we have a\n> > fault with our API.\n> > \n> > The question is if we should still fix lc-compliance here or not - or if\n> > we should keep it 'broken' so that we can validate whatever fix we do\n> > internally in libcamera?\n> \n> I think split this specific issue into a separate smaller test. I\n> don't think there's a need to test this specific issue in all\n> combinations that we test currently in the simple capture test.\n\nAbsolutely. It's just about figuring out what the applications could be\nlikely to get wrong, and if /we've/ got it wrong ... then I guess\napplications could.\n\n--\nKieran\n\n\n> \n> \n> Thanks,\n> \n> Paul\n> \n> > \n> > \n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > src/apps/lc-compliance/simple_capture.cpp | 7 +++----\n> > > src/apps/lc-compliance/simple_capture.h | 1 +\n> > > 2 files changed, 4 insertions(+), 4 deletions(-)\n> > > \n> > > diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp\n> > > index ab5cb35c..cf4d7cf3 100644\n> > > --- a/src/apps/lc-compliance/simple_capture.cpp\n> > > +++ b/src/apps/lc-compliance/simple_capture.cpp\n> > > @@ -65,6 +65,7 @@ void SimpleCapture::stop()\n> > > camera_->requestCompleted.disconnect(this);\n> > > \n> > > Stream *stream = config_->at(0).stream();\n> > > + requests_.clear();\n> > > allocator_->free(stream);\n> > > }\n> > > \n> > > @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > captureLimit_ = numRequests;\n> > > \n> > > /* Queue the recommended number of reqeuests. */\n> > > - std::vector> requests;\n> > > for (const std::unique_ptr &buffer : buffers) {\n> > > std::unique_ptr request = camera_->createRequest();\n> > > ASSERT_TRUE(request) << \"Can't create request\";\n> > > @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > \n> > > ASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > > \n> > > - requests.push_back(std::move(request));\n> > > + requests_.push_back(std::move(request));\n> > > }\n> > > \n> > > /* Run capture session. */\n> > > @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > > captureLimit_ = numRequests;\n> > > \n> > > /* Queue the recommended number of reqeuests. */\n> > > - std::vector> requests;\n> > > for (const std::unique_ptr &buffer : buffers) {\n> > > std::unique_ptr request = camera_->createRequest();\n> > > ASSERT_TRUE(request) << \"Can't create request\";\n> > > @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > > \n> > > ASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > > \n> > > - requests.push_back(std::move(request));\n> > > + requests_.push_back(std::move(request));\n> > > }\n> > > \n> > > /* Run capture session. */\n> > > diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h\n> > > index fd9d2a97..2911d601 100644\n> > > --- a/src/apps/lc-compliance/simple_capture.h\n> > > +++ b/src/apps/lc-compliance/simple_capture.h\n> > > @@ -32,6 +32,7 @@ protected:\n> > > std::shared_ptr camera_;\n> > > std::unique_ptr allocator_;\n> > > std::unique_ptr config_;\n> > > + std::vector> requests_;\n> > > };\n> > > \n> > > class SimpleCaptureBalanced : public SimpleCapture\n> > > -- \n> > > 2.35.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 02815BDE6B\n\tfor ;\n\tWed, 30 Nov 2022 11:35:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 828BC63336;\n\tWed, 30 Nov 2022 12:35:17 +0100 (CET)","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 2274361F23\n\tfor ;\n\tWed, 30 Nov 2022 12:35:16 +0100 (CET)","from pendragon.ideasonboard.com\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 BB08E6D6;\n\tWed, 30 Nov 2022 12:35:15 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669808117;\n\tbh=JwZ3dTIkOsoIiByiSAcD12sSZSDiLplu8+Qg/5uZMEA=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=uL/B4wuxnzfAY0mnGPUGg9f4KhY99MSakxbPqty9ITlposZhM1ERFCr+7+zmoMR5v\n\tX4972BWOnbMDticGhFtuZBsqhmLBzrKamQ9nCOp1i+eEdi+lF5cw9CL/RMsO6EV6r6\n\txWuDUjWLm/qFCUF3lepfNkbCYkAoD0HmSkq91u426QtP/qnQclacYYp6KgczmB+/CL\n\tskFpt2HPR0zc/DiKsPgPwuOjgLCef7xrOF0vFGaRhiBs6s0qKeHjL5GkC8mJrU0pTB\n\tHojhA3zMrNIzuw1ja2L2Y3L/jp1Skt7TQeQhyVZY1bV58s2Jutovvre3kVy+wdm0B7\n\tnc8go+QGqjewA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669808115;\n\tbh=JwZ3dTIkOsoIiByiSAcD12sSZSDiLplu8+Qg/5uZMEA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=IqYpkWTYoRuGbw42XqybTNKKoipLKN6swkO3npcGgnylIUHzr6b5IpFrq5Kb0E1he\n\tB18e7+eKT3f6Qyw6dRr6g795KQdF3jKALTChkxXB6TQGlGPNIU4ji1KpDvGAzrz/XC\n\t8L/ysDKKzS2cfH5a00Qek6a51OTCcIBCmAfrwcdU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"IqYpkWTY\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>\n\t<166974355630.3103093.11675119050850192983@Monstersaurus>\n\t","To":"Paul Elder ","Date":"Wed, 30 Nov 2022 11:35:12 +0000","Message-ID":"<166980811282.1079859.1901288610102904095@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25938,"web_url":"https://patchwork.libcamera.org/comment/25938/","msgid":"<166980817520.1079859.17083852305802406242@Monstersaurus>","date":"2022-11-30T11:36:15","subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2022-11-30 11:31:06)\n> On Wed, Nov 30, 2022 at 07:24:52PM +0800, Umang Jain wrote:\n> > Hi Kieran,\n> > \n> > On 11/30/22 6:53 PM, Kieran Bingham wrote:\n> > > Quoting Umang Jain (2022-11-30 10:35:47)\n> > > > Hi Kieran,\n> > > > \n> > > > On 11/30/22 1:39 AM, Kieran Bingham via libcamera-devel wrote:\n> > > > > Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)\n> > > > > > In the simple capture tests, in the capture functions, the Requests were\n> > > > > > auto-deallocated by the function going out of scope after the test\n> > > > > > completed. However, before the end of the scope, the Framebuffers that\n> > > > > > the Requests referred to were manually freed. Thus when the Requests\n> > > > > > were deallocated, they tried to cancel their Framebuffers, which involve\n> > > > > > setting the status to cancelled, which obviously causes a segfault\n> > > > > > because the Framebuffers had already been freed.\n> > > > > This worries me that we have a path that could let application cause a\n> > > > > segfault in libcamera. That's not good.\n> > > > > \n> > > > > > Fix this by moving the list of Requests to a member variable and\n> > > > > > deallocating them before deallocating the Framebuffers at stop() time.\n> > > > > Fixing this here seems reasonable, but I'm concerned that we now need a\n> > > > > unit test in libcamera unit tests which ensure or validate the lifetimes\n> > > > > of freeing buffers/requests.\n> > > > > \n> > > > > If I understand it, that means that we have a lifetime management issue\n> > > > > still between the Request and the Buffer object that may be associated\n> > > > > with it.\n> > > > If the buffers to be associated with the Requests are allocated and\n> > > > mis-managed by the application themselves - how would that be address by\n> > > > libcamera-core?\n> > > > \n> > > > I am reading the Request->addBuffer() documentation and clearly mentioned:\n> > > > \n> > > >  *\n> > > >  * A reference to the buffer is stored in the request. The caller is\n> > > > responsible\n> > > >  * for ensuring that the buffer will remain valid until the request\n> > > > complete\n> > > >  * callback is called.\n> > > > *\n> > > > \n> > > > In this case the buffers from manually freed before the request got\n> > > > completed (via set 'cancelled' status and completeRequest() codepath in\n> > > > PIpelineHandler::Stop()). So that's not the right thing to do / happen.\n> > > aha, ok - if it's documented, then we're probably good with this for\n> > > now.\n> > > \n> > > I do wonder if we should have some reverse association such that when\n> > > you add a buffer to a request, the buffer knows which request it's\n> > > contained in, and can 'remove' itself when it's freed... but ... that\n> > > makes me wonder if we could legitimately have a single buffer in\n> > > multiple requests which would break that possibility ... so maybe we're\n> > > fine with just the documentation as we have it.\n> > \n> > We can enforce a std::shared_ptr <> model (but that's also not 100% safety)\n> > but not sure how well it'll scale with language bindings in the future.\n> > \n> > All in all, similar to dangling pointers problem.\n> \n> Oops, I didn't notice this thread in the meantime.\n> \n> So this patch is good on its own and I don't have to do anything extra\n> then? :)\n\nThat depends on how likely it is that applications could /cause/ this\nproblem?\n\n--\nKieran\n\n\n> \n> \n> Paul\n> \n> > > \n> > > --\n> > > Kieran\n> > > \n> > > \n> > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=171\n> > > > > Reviewed-by: Kieran Bingham \n> > > > > Tested-by: Kieran Bingham \n> > > > > \n> > > > > But I am not sure we should consider the underlying issue resolved yet.\n> > > > > I feel like the crash in lc-compliance is simply telling us we have a\n> > > > > fault with our API.\n> > > > > \n> > > > > The question is if we should still fix lc-compliance here or not - or if\n> > > > > we should keep it 'broken' so that we can validate whatever fix we do\n> > > > > internally in libcamera?\n> > > > > \n> > > > > --\n> > > > > Kieran\n> > > > > \n> > > > > \n> > > > > > Signed-off-by: Paul Elder \n> > > > > > ---\n> > > > > > src/apps/lc-compliance/simple_capture.cpp | 7 +++----\n> > > > > > src/apps/lc-compliance/simple_capture.h | 1 +\n> > > > > > 2 files changed, 4 insertions(+), 4 deletions(-)\n> > > > > > \n> > > > > > diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp\n> > > > > > index ab5cb35c..cf4d7cf3 100644\n> > > > > > --- a/src/apps/lc-compliance/simple_capture.cpp\n> > > > > > +++ b/src/apps/lc-compliance/simple_capture.cpp\n> > > > > > @@ -65,6 +65,7 @@ void SimpleCapture::stop()\n> > > > > > camera_->requestCompleted.disconnect(this);\n> > > > > > Stream *stream = config_->at(0).stream();\n> > > > > > + requests_.clear();\n> > > > > > allocator_->free(stream);\n> > > > > > }\n> > > > > > @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > > > > captureLimit_ = numRequests;\n> > > > > > /* Queue the recommended number of reqeuests. */\n> > > > > > - std::vector> requests;\n> > > > > > for (const std::unique_ptr &buffer : buffers) {\n> > > > > > std::unique_ptr request = camera_->createRequest();\n> > > > > > ASSERT_TRUE(request) << \"Can't create request\";\n> > > > > > @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > > > > ASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > > > > > - requests.push_back(std::move(request));\n> > > > > > + requests_.push_back(std::move(request));\n> > > > > > }\n> > > > > > /* Run capture session. */\n> > > > > > @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > > > > > captureLimit_ = numRequests;\n> > > > > > /* Queue the recommended number of reqeuests. */\n> > > > > > - std::vector> requests;\n> > > > > > for (const std::unique_ptr &buffer : buffers) {\n> > > > > > std::unique_ptr request = camera_->createRequest();\n> > > > > > ASSERT_TRUE(request) << \"Can't create request\";\n> > > > > > @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > > > > > ASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > > > > > - requests.push_back(std::move(request));\n> > > > > > + requests_.push_back(std::move(request));\n> > > > > > }\n> > > > > > /* Run capture session. */\n> > > > > > diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h\n> > > > > > index fd9d2a97..2911d601 100644\n> > > > > > --- a/src/apps/lc-compliance/simple_capture.h\n> > > > > > +++ b/src/apps/lc-compliance/simple_capture.h\n> > > > > > @@ -32,6 +32,7 @@ protected:\n> > > > > > std::shared_ptr camera_;\n> > > > > > std::unique_ptr allocator_;\n> > > > > > std::unique_ptr config_;\n> > > > > > + std::vector> requests_;\n> > > > > > };\n> > > > > > class SimpleCaptureBalanced : public SimpleCapture\n> > > > > > -- \n> > > > > > 2.35.1\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 486A0BDE6B\n\tfor ;\n\tWed, 30 Nov 2022 11:36:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 141BA61F24;\n\tWed, 30 Nov 2022 12:36:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 061BD61F23\n\tfor ;\n\tWed, 30 Nov 2022 12:36:18 +0100 (CET)","from pendragon.ideasonboard.com\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 A99906D6;\n\tWed, 30 Nov 2022 12:36:17 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669808180;\n\tbh=99vBo4HBtfOdfaOc8j5tEXOhsluM55PNhiytQwmoWcI=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=n6yaMs2f2if6ltr0XpW38zbmFgFMOdxb4J/l2bhWalq2UJugxLEv2cmqkJAa1WAdf\n\tiBlzsIuAYVW774IodKfcqbaZmo3qEwko872A3mchf/ze6fA2IboX7s1T07sEIk0bh5\n\t9Yxamtttp6MqtIhUKLXh3c4aRqeErtW2kL1wOgmISwxs7PLftGDy96hCMeHNwlB8Pn\n\t/Q8N+TEVQJREcsd3usHd2cg4Ke5Pml7OyKFtdl9y0mDaLIQQs4IaO5L0bSudA00HSO\n\tyRVFJBV+DLfuUmF4XSdWSbAIIfRydrS0XpzhX2xnIqkoA5lt00SejHDDBOphhtPY1d\n\tr61z4We5aPwYg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669808177;\n\tbh=99vBo4HBtfOdfaOc8j5tEXOhsluM55PNhiytQwmoWcI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=elVDUVOQoGcBqyKrfFc+Fa28AasFVbCLC/FW1cZJEU8Yiq57MZfgemH0IlpuuR/eI\n\tBjbFQ9htn2GsQupHvjHOn3FjdCQzrSYmtfgpks7x5+GL5IMnx9oezXM7wosAD1TqcZ\n\tf7j9Mn7Ka5Qy7c4QyFwUGD2jMM/evH/LvR1v67vM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"elVDUVOQ\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>\n\t<166974355630.3103093.11675119050850192983@Monstersaurus>\n\t\n\t<166980559040.1079859.15872634788506413846@Monstersaurus>\n\t<2f0d8883-4c43-29cb-5495-687c045c78f4@ideasonboard.com>\n\t","To":"Paul Elder ,\n\tUmang Jain ","Date":"Wed, 30 Nov 2022 11:36:15 +0000","Message-ID":"<166980817520.1079859.17083852305802406242@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25945,"web_url":"https://patchwork.libcamera.org/comment/25945/","msgid":"<20221130173004.byt6sktfyzj4vqqh@notapiano>","date":"2022-11-30T17:30:04","subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"On Tue, Nov 29, 2022 at 08:00:05PM +0900, Paul Elder via libcamera-devel wrote:\n> In the simple capture tests, in the capture functions, the Requests were\n> auto-deallocated by the function going out of scope after the test\n> completed. However, before the end of the scope, the Framebuffers that\n> the Requests referred to were manually freed. Thus when the Requests\n> were deallocated, they tried to cancel their Framebuffers, which involve\n> setting the status to cancelled, which obviously causes a segfault\n> because the Framebuffers had already been freed.\n> \n> Fix this by moving the list of Requests to a member variable and\n> deallocating them before deallocating the Framebuffers at stop() time.\n> \n> Signed-off-by: Paul Elder \n\nI ended up sending almost exactly the same patch [1], so that's kind of a\nR-by/T-by tag in itself :), but here they are explicitly:\n\nReviewed-by: Nícolas F. R. A. Prado \nTested-by: Nícolas F. R. A. Prado \n\nThanks,\nNícolas\n\n[1] https://patchwork.libcamera.org/patch/17918/","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 0D737BE08B\n\tfor ;\n\tWed, 30 Nov 2022 17:30:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79FC263336;\n\tWed, 30 Nov 2022 18:30:10 +0100 (CET)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[46.235.227.172])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5179A61F23\n\tfor ;\n\tWed, 30 Nov 2022 18:30:09 +0100 (CET)","from notapiano (unknown\n\t[IPv6:2600:4041:5b1a:cd00:524d:e95d:1a9c:492a])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256) (No client certificate requested)\n\t(Authenticated sender: nfraprado)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id 9918466003EF;\n\tWed, 30 Nov 2022 17:30:08 +0000 (GMT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669829410;\n\tbh=OZCM2K9MjFNBr7EbBuNt5F9K+VQ3zmOnnvnHNeqRQgM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=rU57+LLB41ewS9OJoMmaPeAUimAe1k6RG6DKfJdsrLWDK94YKyCX+eFBpE46f6uRk\n\t4wHLR/9HXXPr9rO0OHU5nUXhvAJ5J0uCn3Re52XtK2xHaGWBhUTXT9K3j43wrl5NsL\n\tav2XvZMrfwH9SfOmXuDbb+/Rs257tbIKCJvkDSsqMBekYEgJEY0V/Te42n3uVHTQ/X\n\tEUmNT4Tl9P6pPrFp2vPbONdA4QZz86YQq0Ki5cUSpdtMZVKDBsJo2RH1tC6Hyu/yYM\n\t+TGNDiERcnI2TmLwLR0SV5BqYb/TYv9luIKzsTk9yg977/x2ZuuFdWFjjC8bpwO4jF\n\t6VP49kIVn+ReA==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1669829409;\n\tbh=OZCM2K9MjFNBr7EbBuNt5F9K+VQ3zmOnnvnHNeqRQgM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ISMZKzLTSY6VRDd1BVdB8Ql/f3plvyIxEX3R1mjIkgG5V8frqup6dZlqEBqCfc9bm\n\tBk36wzQ6vJcoLD8HwIMM7K9Xo/e+bDmdOiOI06NICbsK55yr4Bm/eSzCgbIKEH/pfq\n\tmtxrwt25/KG+xzuCRziY7t+v803jxUyRy2+IZsMaGeTGN6I1GkK8qUzWfRi924lONI\n\tDRKvpFOTcgW/pqezJWNylHWDzbjMj4AQzdUPzCxzo38N5BjICroPNaTLUongPDPqlo\n\tpJTEgA3naWjngvlOCyLOg4acJ/RvAHtCef5pOnj2V00GDI5OvDhaGdbapS7bVgXEqe\n\t+YRpiAnByoQ+A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"ISMZKzLT\"; dkim-atps=neutral","Date":"Wed, 30 Nov 2022 12:30:04 -0500","To":"Paul Elder ","Message-ID":"<20221130173004.byt6sktfyzj4vqqh@notapiano>","References":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","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","From":"=?utf-8?q?N=C3=ADcolas_F=2E_R=2E_A=2E_Prado_via_libcamera-devel?=\n\t","Reply-To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25947,"web_url":"https://patchwork.libcamera.org/comment/25947/","msgid":"","date":"2022-12-01T01:31:18","subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Nov 30, 2022 at 11:36:15AM +0000, Kieran Bingham via libcamera-devel wrote:\n> Quoting Paul Elder (2022-11-30 11:31:06)\n> > On Wed, Nov 30, 2022 at 07:24:52PM +0800, Umang Jain wrote:\n> > > On 11/30/22 6:53 PM, Kieran Bingham wrote:\n> > > > Quoting Umang Jain (2022-11-30 10:35:47)\n> > > > > On 11/30/22 1:39 AM, Kieran Bingham via libcamera-devel wrote:\n> > > > > > Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)\n> > > > > > > In the simple capture tests, in the capture functions, the Requests were\n> > > > > > > auto-deallocated by the function going out of scope after the test\n> > > > > > > completed. However, before the end of the scope, the Framebuffers that\n> > > > > > > the Requests referred to were manually freed. Thus when the Requests\n> > > > > > > were deallocated, they tried to cancel their Framebuffers, which involve\n> > > > > > > setting the status to cancelled, which obviously causes a segfault\n> > > > > > > because the Framebuffers had already been freed.\n> > > > > >\n> > > > > > This worries me that we have a path that could let application cause a\n> > > > > > segfault in libcamera. That's not good.\n\nWell, if applications really mess it up, it's bound to happen. I do\nhowever agree that in this case it seems possibly too easy for\napplications to get it wrong.\n\n> > > > > > > Fix this by moving the list of Requests to a member variable and\n> > > > > > > deallocating them before deallocating the Framebuffers at stop() time.\n> > > > > >\n> > > > > > Fixing this here seems reasonable, but I'm concerned that we now need a\n> > > > > > unit test in libcamera unit tests which ensure or validate the lifetimes\n> > > > > > of freeing buffers/requests.\n> > > > > > \n> > > > > > If I understand it, that means that we have a lifetime management issue\n> > > > > > still between the Request and the Buffer object that may be associated\n> > > > > > with it.\n\nWe have an issue if the application gets it wrong. How would a unit test\nhelp here ?\n\n> > > > > If the buffers to be associated with the Requests are allocated and\n> > > > > mis-managed by the application themselves - how would that be address by\n> > > > > libcamera-core?\n> > > > > \n> > > > > I am reading the Request->addBuffer() documentation and clearly mentioned:\n> > > > > \n> > > > >  *\n> > > > >  * A reference to the buffer is stored in the request. The caller is responsible\n> > > > >  * for ensuring that the buffer will remain valid until the request complete\n> > > > >  * callback is called.\n> > > > > *\n> > > > > \n> > > > > In this case the buffers from manually freed before the request got\n> > > > > completed (via set 'cancelled' status and completeRequest() codepath in\n> > > > > PIpelineHandler::Stop()). So that's not the right thing to do / happen.\n> > > >\n> > > > aha, ok - if it's documented, then we're probably good with this for\n> > > > now.\n> > > > \n> > > > I do wonder if we should have some reverse association such that when\n> > > > you add a buffer to a request, the buffer knows which request it's\n> > > > contained in, and can 'remove' itself when it's freed... but ... that\n> > > > makes me wonder if we could legitimately have a single buffer in\n> > > > multiple requests which would break that possibility ... so maybe we're\n> > > > fine with just the documentation as we have it.\n> > > \n> > > We can enforce a std::shared_ptr <> model (but that's also not 100% safety)\n> > > but not sure how well it'll scale with language bindings in the future.\n\nstd::shared_ptr<> are usually painful, and not just for language\nbindings, they tend to creep in everywhere when you start using them. I\nwouldn't go that route if possible.\n\n> > > All in all, similar to dangling pointers problem.\n> > \n> > Oops, I didn't notice this thread in the meantime.\n> > \n> > So this patch is good on its own and I don't have to do anything extra\n> > then? :)\n> \n> That depends on how likely it is that applications could /cause/ this\n> problem?\n\nThere's something I don't quite get. The requests are destroyed after\nstop(). I thus expect all requests to have completed, either normally or\nbecause they have been cancelled, before the stop() function returns.\nHow comes Request::Private::doCancelRequest(), called from the request\ndestructor, sees a non-empty pending_ list ?\n\n> > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=171\n> > > > > > Reviewed-by: Kieran Bingham \n> > > > > > Tested-by: Kieran Bingham \n> > > > > > \n> > > > > > But I am not sure we should consider the underlying issue resolved yet.\n> > > > > > I feel like the crash in lc-compliance is simply telling us we have a\n> > > > > > fault with our API.\n> > > > > > \n> > > > > > The question is if we should still fix lc-compliance here or not - or if\n> > > > > > we should keep it 'broken' so that we can validate whatever fix we do\n> > > > > > internally in libcamera?\n> > > > > > \n> > > > > > --\n> > > > > > Kieran\n> > > > > > \n> > > > > > \n> > > > > > > Signed-off-by: Paul Elder \n> > > > > > > ---\n> > > > > > > src/apps/lc-compliance/simple_capture.cpp | 7 +++----\n> > > > > > > src/apps/lc-compliance/simple_capture.h | 1 +\n> > > > > > > 2 files changed, 4 insertions(+), 4 deletions(-)\n> > > > > > > \n> > > > > > > diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp\n> > > > > > > index ab5cb35c..cf4d7cf3 100644\n> > > > > > > --- a/src/apps/lc-compliance/simple_capture.cpp\n> > > > > > > +++ b/src/apps/lc-compliance/simple_capture.cpp\n> > > > > > > @@ -65,6 +65,7 @@ void SimpleCapture::stop()\n> > > > > > > camera_->requestCompleted.disconnect(this);\n> > > > > > > Stream *stream = config_->at(0).stream();\n> > > > > > > + requests_.clear();\n> > > > > > > allocator_->free(stream);\n> > > > > > > }\n> > > > > > > @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > > > > > captureLimit_ = numRequests;\n> > > > > > > /* Queue the recommended number of reqeuests. */\n> > > > > > > - std::vector> requests;\n> > > > > > > for (const std::unique_ptr &buffer : buffers) {\n> > > > > > > std::unique_ptr request = camera_->createRequest();\n> > > > > > > ASSERT_TRUE(request) << \"Can't create request\";\n> > > > > > > @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > > > > > ASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > > > > > > - requests.push_back(std::move(request));\n> > > > > > > + requests_.push_back(std::move(request));\n> > > > > > > }\n> > > > > > > /* Run capture session. */\n> > > > > > > @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > > > > > > captureLimit_ = numRequests;\n> > > > > > > /* Queue the recommended number of reqeuests. */\n> > > > > > > - std::vector> requests;\n> > > > > > > for (const std::unique_ptr &buffer : buffers) {\n> > > > > > > std::unique_ptr request = camera_->createRequest();\n> > > > > > > ASSERT_TRUE(request) << \"Can't create request\";\n> > > > > > > @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > > > > > > ASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > > > > > > - requests.push_back(std::move(request));\n> > > > > > > + requests_.push_back(std::move(request));\n> > > > > > > }\n> > > > > > > /* Run capture session. */\n> > > > > > > diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h\n> > > > > > > index fd9d2a97..2911d601 100644\n> > > > > > > --- a/src/apps/lc-compliance/simple_capture.h\n> > > > > > > +++ b/src/apps/lc-compliance/simple_capture.h\n> > > > > > > @@ -32,6 +32,7 @@ protected:\n> > > > > > > std::shared_ptr camera_;\n> > > > > > > std::unique_ptr allocator_;\n> > > > > > > std::unique_ptr config_;\n> > > > > > > + std::vector> requests_;\n> > > > > > > };\n> > > > > > > class SimpleCaptureBalanced : public SimpleCapture","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 137EABE08B\n\tfor ;\n\tThu, 1 Dec 2022 01:31:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69A2561F24;\n\tThu, 1 Dec 2022 02:31:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7529C61F24\n\tfor ;\n\tThu, 1 Dec 2022 02:31:35 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B7BE02D9;\n\tThu, 1 Dec 2022 02:31:34 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669858296;\n\tbh=Dc8q665b7rwafIJyEZvhU4LSpwWBar8c2M49euv6X5E=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=OxKKzPXN8qdVY0YL8EPvxQ0snzRyYgkN1kqudrqeFhpWkJMYKg6fmiJhRLT9mvDKf\n\tn1xoHWySwI/tbRXZ/li/Vk2RxjrddMUpBLp3+VIVI9ZRBmxPVx4g8MKgqcoHKtCech\n\tOAy7D9KkTdi7jDKG8sCvwD/FevQto37spIrO9Da0gpPtZHIyexpZjDUCjBLF3tskTA\n\te1w4T1gkIFMKhdl2VcBRDc4StxH4B4/B5vTCygf3xrpULu4Pr2I4wVq/Be0jRKmJzb\n\t9j+Zqj0zIokqpDGDV74JkTIA30y4831C/5WhFGeLdMofi/GUyi3/rENKdTH0ajseYk\n\tZiKbU6kWI7JrA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669858294;\n\tbh=Dc8q665b7rwafIJyEZvhU4LSpwWBar8c2M49euv6X5E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E3ezp4nifHr4vkUBHqOlNXJLeS1r5xcdTS0phObQR+TEagxh1+C2dtgIu1hIOmh/L\n\txM7ju0qGmNJ41uz6UgG4aDG16N5SepncS7ZPxbkmUMQkjnp9hyNRnceLVyC3wmkwBg\n\t5JvDnookA+Kzo4+1dzE3WGEuq57sIrNvSd8yoVaY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"E3ezp4ni\"; dkim-atps=neutral","Date":"Thu, 1 Dec 2022 03:31:18 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20221129110005.4067748-1-paul.elder@ideasonboard.com>\n\t<166974355630.3103093.11675119050850192983@Monstersaurus>\n\t\n\t<166980559040.1079859.15872634788506413846@Monstersaurus>\n\t<2f0d8883-4c43-29cb-5495-687c045c78f4@ideasonboard.com>\n\t\n\t<166980817520.1079859.17083852305802406242@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<166980817520.1079859.17083852305802406242@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] lc-compliance: simple_capture: Free\n\tRequests properly","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]