[{"id":15587,"web_url":"https://patchwork.libcamera.org/comment/15587/","msgid":"<3a4776a0-239c-da2c-509c-46eac4aaae4c@uajain.com>","date":"2021-03-10T15:05:31","subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Marian\n\nThank you for the patch.\n\nOn 3/9/21 8:05 PM, Marian Cichy wrote:\n> If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the\n> unique_ptr to the request-object gets reset and hence, its destructor\n> is called. However, the wrap-object points to the same object and is\n> still alive at this moment. When the task_run-function is finished, the\n> destructor of the wrap-object is called, which in return calls the\n> destructor of the request-object again.\n>\n> Also note the wrong comment, which claims that WrapRequest does not\n> take ownership of the request, however, actually it already has\n> ownership.\n>\n> Replacing request.reset() with request.release() doesn't call the\n> destructor on the request-object and only one free happens at the end.\n>\n> Signed-off-by: Marian Cichy \n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index a8ed7652..b0194c2f 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -279,10 +279,12 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \t\t\t\t\t\t &buffer, nullptr);\n> \t\tif (ret != GST_FLOW_OK) {\n> \t\t\t/*\n> -\t\t\t * RequestWrap does not take ownership, and we won't be\n> +\t\t\t * RequestWrap has ownership, and we won't be\n> \t\t\t * queueing this one due to lack of buffers.\n> +\t\t\t * So the request will be freed when RequestWrap\n> +\t\t\t * goes out of scope.\n> \t\t\t */\n> -\t\t\trequest.reset();\n> +\t\t\trequest.release();\nThis is a bit problematic as `request` is still in-used beyond this \nline(there is `if (request)` block below) and here it will set it as \nNULL. You will need to take care of this situation using the RequestWrap \nprobably. However, one thing I still am not sure of ...\n> \t\t\tbreak;\n> \t\t}\n> \n\nI am not sure if why `RequestWrap` is written intentionally in a way, to \ntransfer the ownership of the `Request`. This fact is quite evident \nlooking at the ~RequestWrap().  The larger picture here is (which is why \nyou hit this double-free issue) that, whenever RequestWrap is used - \nalways transfer ownership of Request. So, that's a 2-step manual in \norder to use the wrap; if the latter is forgotten - we will always hit \ndouble free issues.\n\nCan you please look how the RequestWrap is used elsewhere in the \ngstreamer plugin and try to see why transfer of ownership is intented in \nRequestWrap? If it's not essential, we should probably re-fine \nRequestWrap to really be a \"wrapper\" and not try to steal the reference \n- and take the additional responsibility of free-ing it.\n\nThanks!\n\n-Umang","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 D5CF8BD1F1\n\tfor ;\n\tWed, 10 Mar 2021 15:05:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D42D68A9C;\n\tWed, 10 Mar 2021 16:05:36 +0100 (CET)","from mail.uajain.com (static.126.159.217.95.clients.your-server.de\n\t[95.217.159.126])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 220DA60106\n\tfor ;\n\tWed, 10 Mar 2021 16:05:34 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"F8bzsJnX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1615388734; bh=Aa/33zHFNm9gzUDVvlqZTuQT7lhfYoUrSDC/FB289NY=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=F8bzsJnXMV8d6kBGK2uySdPaYJMYc52Er8A5nNPUYpbNlSCrClkrYCwaXQv31g/D5\n\tSJQ3T0v/agv58UyHGjpF5/DarS8GkyhqC9dcVBjsTcYO7DB6GXTqibZVwNr2vloTBp\n\tTqty7aeeEgzOy9GDCtd2w2RXs9+j3l+VZy8gkkRm17SVtVGMvt01U0SmVLhTziyjT6\n\tPraGVYeVSGZEPGQ+zzpl3ug6ydX3ssECW9eSWST3kdPx3Co/8a/6H9TovMQvh/t7hH\n\tM/YtP9keh5Ytfjgd6878Hocy/5MAh40YTBe8A6Ubm8OFE0x/79dnBxSlAUEluZq+Jl\n\tiWE1tQvZWMRaQ==","To":"Marian Cichy ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210309143518.31405-1-m.cichy@pengutronix.de>","From":"Umang Jain ","Message-ID":"<3a4776a0-239c-da2c-509c-46eac4aaae4c@uajain.com>","Date":"Wed, 10 Mar 2021 20:35:31 +0530","Mime-Version":"1.0","In-Reply-To":"<20210309143518.31405-1-m.cichy@pengutronix.de>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"graphics@pengutronix.de","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15588,"web_url":"https://patchwork.libcamera.org/comment/15588/","msgid":"","date":"2021-03-10T17:30:12","subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Marian\n\nThank you for the patch.\n\nOn Wed, Mar 10, 2021 at 08:35:31PM +0530, Umang Jain wrote:\n> On 3/9/21 8:05 PM, Marian Cichy wrote:\n> > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the\n> > unique_ptr to the request-object gets reset and hence, its destructor\n> > is called. However, the wrap-object points to the same object and is\n> > still alive at this moment. When the task_run-function is finished, the\n> > destructor of the wrap-object is called, which in return calls the\n> > destructor of the request-object again.\n> >\n> > Also note the wrong comment, which claims that WrapRequest does not\n> > take ownership of the request, however, actually it already has\n> > ownership.\n> >\n> > Replacing request.reset() with request.release() doesn't call the\n> > destructor on the request-object and only one free happens at the end.\n> >\n> > Signed-off-by: Marian Cichy \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 6 ++++--\n> > 1 file changed, 4 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index a8ed7652..b0194c2f 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -279,10 +279,12 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > \t\t\t\t\t\t &buffer, nullptr);\n> > \t\tif (ret != GST_FLOW_OK) {\n> > \t\t\t/*\n> > -\t\t\t * RequestWrap does not take ownership, and we won't be\n> > +\t\t\t * RequestWrap has ownership, and we won't be\n> > \t\t\t * queueing this one due to lack of buffers.\n> > +\t\t\t * So the request will be freed when RequestWrap\n> > +\t\t\t * goes out of scope.\n> > \t\t\t */\n> > -\t\t\trequest.reset();\n> > +\t\t\trequest.release();\n>\n> This is a bit problematic as `request` is still in-used beyond this \n> line(there is `if (request)` block below) and here it will set it as \n> NULL. You will need to take care of this situation using the RequestWrap \n> probably. However, one thing I still am not sure of ...\n>\n> > \t\t\tbreak;\n> > \t\t}\n> > \n> \n> I am not sure if why `RequestWrap` is written intentionally in a way, to \n> transfer the ownership of the `Request`. This fact is quite evident \n> looking at the ~RequestWrap().  The larger picture here is (which is why \n> you hit this double-free issue) that, whenever RequestWrap is used - \n> always transfer ownership of Request. So, that's a 2-step manual in \n> order to use the wrap; if the latter is forgotten - we will always hit \n> double free issues.\n> \n> Can you please look how the RequestWrap is used elsewhere in the \n> gstreamer plugin and try to see why transfer of ownership is intented in \n> RequestWrap? If it's not essential, we should probably re-fine \n> RequestWrap to really be a \"wrapper\" and not try to steal the reference \n> - and take the additional responsibility of free-ing it.\n\nThe request belongs to the gstreamer element, so we need to keep track\nof it and free it. Request instances are managed with std::unique_ptr<>,\nso the best option I think would be to simple move the request to the\nwrap, using a unique pointer there.\n\nHow about this untested change ?\n\ndiff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\nindex 636c14dff64e..e86c3d7f74c7 100644\n--- a/src/gstreamer/gstlibcamerasrc.cpp\n+++ b/src/gstreamer/gstlibcamerasrc.cpp\n@@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n #define GST_CAT_DEFAULT source_debug\n \n struct RequestWrap {\n-\tRequestWrap(Request *request);\n+\tRequestWrap(std::unique_ptr request);\n \t~RequestWrap();\n \n \tvoid attachBuffer(GstBuffer *buffer);\n \tGstBuffer *detachBuffer(Stream *stream);\n \n \t/* For ptr comparison only. */\n-\tRequest *request_;\n+\tstd::unique_ptr request_;\n \tstd::map buffers_;\n };\n \n-RequestWrap::RequestWrap(Request *request)\n-\t: request_(request)\n+RequestWrap::RequestWrap(std::unique_ptr request)\n+\t: request_(std::move(request))\n {\n }\n \n@@ -74,8 +74,6 @@ RequestWrap::~RequestWrap()\n \t\tif (item.second)\n \t\t\tgst_buffer_unref(item.second);\n \t}\n-\n-\tdelete request_;\n }\n \n void RequestWrap::attachBuffer(GstBuffer *buffer)\n@@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n \tstd::unique_ptr wrap = std::move(requests_.front());\n \trequests_.pop();\n \n-\tg_return_if_fail(wrap->request_ == request);\n+\tg_return_if_fail(wrap->request_.get() == request);\n \n \tif ((request->status() == Request::RequestCancelled)) {\n \t\tGST_DEBUG_OBJECT(src_, \"Request was cancelled\");\n@@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n \tGstLibcameraSrcState *state = self->state;\n \n-\tstd::unique_ptr request = state->cam_->createRequest();\n-\tauto wrap = std::make_unique(request.get());\n+\tauto wrap = std::make_unique(state->cam_->createRequest());\n \tfor (GstPad *srcpad : state->srcpads_) {\n \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n \t\tGstBuffer *buffer;\n@@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data)\n \t\t\t\t\t\t &buffer, nullptr);\n \t\tif (ret != GST_FLOW_OK) {\n \t\t\t/*\n-\t\t\t * RequestWrap does not take ownership, and we won't be\n-\t\t\t * queueing this one due to lack of buffers.\n+\t\t\t * RequestWrap has ownership of the rquest, and we\n+\t\t\t * won't be queueing this one due to lack of buffers.\n \t\t\t */\n-\t\t\trequest.reset();\n+\t\t\twrap.release();\n \t\t\tbreak;\n \t\t}\n \n \t\twrap->attachBuffer(buffer);\n \t}\n \n-\tif (request) {\n+\tif (wrap) {\n \t\tGLibLocker lock(GST_OBJECT(self));\n \t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n-\t\tstate->cam_->queueRequest(request.get());\n+\t\tstate->cam_->queueRequest(wrap->request_.get());\n \t\tstate->requests_.push(std::move(wrap));\n \n-\t\t/* The request will be deleted in the completion handler. */\n-\t\trequest.release();\n+\t\t/* The RequestWrap will be deleted in the completion handler. */\n \t}\n \n \tGstFlowReturn ret = GST_FLOW_OK;","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 0891DBD1F1\n\tfor ;\n\tWed, 10 Mar 2021 17:30:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 512AA68A9C;\n\tWed, 10 Mar 2021 18:30:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A19660106\n\tfor ;\n\tWed, 10 Mar 2021 18:30:45 +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 769F09F0;\n\tWed, 10 Mar 2021 18:30:44 +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=\"op89hoqa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615397444;\n\tbh=/PvMoM2/v3c2KNuPhVi6AMoIhWNP9tJBI1TCqvijPM8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=op89hoqaBhRmh6/ctNFf+F0T65kV81ebgepcWFezKKjPdVx3Qax0t/hxMtxXtVrZg\n\tTdSL5LEsEKUAQSeA1og86qpdlScwzuQrrvZF/+Qf9JtjSsCKs5rhQTlHYFp8VTtjwP\n\t+ALycPkghbkQuAvXa2lx/ywH7StXZGw7PjrxNpo4=","Date":"Wed, 10 Mar 2021 19:30:12 +0200","From":"Laurent Pinchart ","To":"Marian Cichy ","Message-ID":"","References":"<20210309143518.31405-1-m.cichy@pengutronix.de>\n\t<3a4776a0-239c-da2c-509c-46eac4aaae4c@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<3a4776a0-239c-da2c-509c-46eac4aaae4c@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, graphics@pengutronix.de","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15589,"web_url":"https://patchwork.libcamera.org/comment/15589/","msgid":"<1850310f-4b44-e809-12ea-88ca3d4391a6@pengutronix.de>","date":"2021-03-10T17:53:30","subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","submitter":{"id":81,"url":"https://patchwork.libcamera.org/api/people/81/","name":"Marian Cichy","email":"mci@pengutronix.de"},"content":"On 3/10/21 6:30 PM, Laurent Pinchart wrote:\n> Hi Marian\n>\n> Thank you for the patch.\n>\n> On Wed, Mar 10, 2021 at 08:35:31PM +0530, Umang Jain wrote:\n>> On 3/9/21 8:05 PM, Marian Cichy wrote:\n>>> If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the\n>>> unique_ptr to the request-object gets reset and hence, its destructor\n>>> is called. However, the wrap-object points to the same object and is\n>>> still alive at this moment. When the task_run-function is finished, the\n>>> destructor of the wrap-object is called, which in return calls the\n>>> destructor of the request-object again.\n>>>\n>>> Also note the wrong comment, which claims that WrapRequest does not\n>>> take ownership of the request, however, actually it already has\n>>> ownership.\n>>>\n>>> Replacing request.reset() with request.release() doesn't call the\n>>> destructor on the request-object and only one free happens at the end.\n>>>\n>>> Signed-off-by: Marian Cichy \n>>> ---\n>>> src/gstreamer/gstlibcamerasrc.cpp | 6 ++++--\n>>> 1 file changed, 4 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>>> index a8ed7652..b0194c2f 100644\n>>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>>> @@ -279,10 +279,12 @@ gst_libcamera_src_task_run(gpointer user_data)\n>>> \t\t\t\t\t\t &buffer, nullptr);\n>>> \t\tif (ret != GST_FLOW_OK) {\n>>> \t\t\t/*\n>>> -\t\t\t * RequestWrap does not take ownership, and we won't be\n>>> +\t\t\t * RequestWrap has ownership, and we won't be\n>>> \t\t\t * queueing this one due to lack of buffers.\n>>> +\t\t\t * So the request will be freed when RequestWrap\n>>> +\t\t\t * goes out of scope.\n>>> \t\t\t */\n>>> -\t\t\trequest.reset();\n>>> +\t\t\trequest.release();\n>> This is a bit problematic as `request` is still in-used beyond this\n>> line(there is `if (request)` block below) and here it will set it as\n>> NULL. You will need to take care of this situation using the RequestWrap\n>> probably. However, one thing I still am not sure of ...\n>>\n>>> \t\t\tbreak;\n>>> \t\t}\n>>> \n>> I am not sure if why `RequestWrap` is written intentionally in a way, to\n>> transfer the ownership of the `Request`. This fact is quite evident\n>> looking at the ~RequestWrap().  The larger picture here is (which is why\n>> you hit this double-free issue) that, whenever RequestWrap is used -\n>> always transfer ownership of Request. So, that's a 2-step manual in\n>> order to use the wrap; if the latter is forgotten - we will always hit\n>> double free issues.\n>>\n>> Can you please look how the RequestWrap is used elsewhere in the\n>> gstreamer plugin and try to see why transfer of ownership is intented in\n>> RequestWrap? If it's not essential, we should probably re-fine\n>> RequestWrap to really be a \"wrapper\" and not try to steal the reference\n>> - and take the additional responsibility of free-ing it.\n> The request belongs to the gstreamer element, so we need to keep track\n> of it and free it. Request instances are managed with std::unique_ptr<>,\n> so the best option I think would be to simple move the request to the\n> wrap, using a unique pointer there.\n>\n> How about this untested change ?\n>\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 636c14dff64e..e86c3d7f74c7 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n> #define GST_CAT_DEFAULT source_debug\n> \n> struct RequestWrap {\n> -\tRequestWrap(Request *request);\n> +\tRequestWrap(std::unique_ptr request);\n> \t~RequestWrap();\n> \n> \tvoid attachBuffer(GstBuffer *buffer);\n> \tGstBuffer *detachBuffer(Stream *stream);\n> \n> \t/* For ptr comparison only. */\n> -\tRequest *request_;\n> +\tstd::unique_ptr request_;\n> \tstd::map buffers_;\n> };\n> \n> -RequestWrap::RequestWrap(Request *request)\n> -\t: request_(request)\n> +RequestWrap::RequestWrap(std::unique_ptr request)\n> +\t: request_(std::move(request))\n> {\n> }\n> \n> @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap()\n> \t\tif (item.second)\n> \t\t\tgst_buffer_unref(item.second);\n> \t}\n> -\n> -\tdelete request_;\n> }\n> \n> void RequestWrap::attachBuffer(GstBuffer *buffer)\n> @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> \tstd::unique_ptr wrap = std::move(requests_.front());\n> \trequests_.pop();\n> \n> -\tg_return_if_fail(wrap->request_ == request);\n> +\tg_return_if_fail(wrap->request_.get() == request);\n> \n> \tif ((request->status() == Request::RequestCancelled)) {\n> \t\tGST_DEBUG_OBJECT(src_, \"Request was cancelled\");\n> @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> \tGstLibcameraSrcState *state = self->state;\n> \n> -\tstd::unique_ptr request = state->cam_->createRequest();\n> -\tauto wrap = std::make_unique(request.get());\n> +\tauto wrap = std::make_unique(state->cam_->createRequest());\n> \tfor (GstPad *srcpad : state->srcpads_) {\n> \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n> \t\tGstBuffer *buffer;\n> @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \t\t\t\t\t\t &buffer, nullptr);\n> \t\tif (ret != GST_FLOW_OK) {\n> \t\t\t/*\n> -\t\t\t * RequestWrap does not take ownership, and we won't be\n> -\t\t\t * queueing this one due to lack of buffers.\n> +\t\t\t * RequestWrap has ownership of the rquest, and we\n> +\t\t\t * won't be queueing this one due to lack of buffers.\n> \t\t\t */\n> -\t\t\trequest.reset();\n> +\t\t\twrap.release();\n> \t\t\tbreak;\n> \t\t}\n> \n> \t\twrap->attachBuffer(buffer);\n> \t}\n> \n> -\tif (request) {\n> +\tif (wrap) {\n> \t\tGLibLocker lock(GST_OBJECT(self));\n> \t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> -\t\tstate->cam_->queueRequest(request.get());\n> +\t\tstate->cam_->queueRequest(wrap->request_.get());\n\n\nDid you mean\n\nstate->cam_->queueRequest(wrap->request_)\n\nthe wrapped request is not a unique_ptr here.\n\n\nWith this change, I tested both the success and the error path and it \nlooks fine. However, I think there should be a printed message (maybe \nGST_WARNING) when we get in the error path, as there is no indicator now \nthat buffer acquistion failed.\n\n\nRegards,\n\nMarian\n\n\n> \t\tstate->requests_.push(std::move(wrap));\n> \n> -\t\t/* The request will be deleted in the completion handler. */\n> -\t\trequest.release();\n> +\t\t/* The RequestWrap will be deleted in the completion handler. */\n> \t}\n> \n> \tGstFlowReturn ret = GST_FLOW_OK;\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 22672BD80C\n\tfor ;\n\tWed, 10 Mar 2021 17:53:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0B8F68AA2;\n\tWed, 10 Mar 2021 18:53:32 +0100 (CET)","from metis.ext.pengutronix.de (metis.ext.pengutronix.de\n\t[IPv6:2001:67c:670:201:290:27ff:fe1d:cc33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7D3D60106\n\tfor ;\n\tWed, 10 Mar 2021 18:53:31 +0100 (CET)","from gallifrey.ext.pengutronix.de\n\t([2001:67c:670:201:5054:ff:fe8d:eefb] helo=[127.0.0.1])\n\tby metis.ext.pengutronix.de with esmtp (Exim 4.92)\n\t(envelope-from )\n\tid 1lK31X-0000vS-7V; Wed, 10 Mar 2021 18:53:31 +0100"],"To":"Laurent Pinchart ","References":"<20210309143518.31405-1-m.cichy@pengutronix.de>\n\t<3a4776a0-239c-da2c-509c-46eac4aaae4c@uajain.com>\n\t","From":"Marian Cichy ","Message-ID":"<1850310f-4b44-e809-12ea-88ca3d4391a6@pengutronix.de>","Date":"Wed, 10 Mar 2021 18:53:30 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.5.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","X-SA-Exim-Connect-IP":"2001:67c:670:201:5054:ff:fe8d:eefb","X-SA-Exim-Mail-From":"mci@pengutronix.de","X-SA-Exim-Scanned":"No (on metis.ext.pengutronix.de);\n\tSAEximRunCond expanded to false","X-PTX-Original-Recipient":"libcamera-devel@lists.libcamera.org","Subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, graphics@pengutronix.de","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15590,"web_url":"https://patchwork.libcamera.org/comment/15590/","msgid":"<6aacd08f-3c20-a619-b453-129d2400825f@pengutronix.de>","date":"2021-03-10T18:12:45","subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","submitter":{"id":81,"url":"https://patchwork.libcamera.org/api/people/81/","name":"Marian Cichy","email":"mci@pengutronix.de"},"content":"On 3/10/21 6:53 PM, Marian Cichy wrote:\n>\n> On 3/10/21 6:30 PM, Laurent Pinchart wrote:\n>> Hi Marian\n>>\n>> Thank you for the patch.\n>>\n>> On Wed, Mar 10, 2021 at 08:35:31PM +0530, Umang Jain wrote:\n>>> On 3/9/21 8:05 PM, Marian Cichy wrote:\n>>>> If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the\n>>>> unique_ptr to the request-object gets reset and hence, its destructor\n>>>> is called. However, the wrap-object points to the same object and is\n>>>> still alive at this moment. When the task_run-function is finished, \n>>>> the\n>>>> destructor of the wrap-object is called, which in return calls the\n>>>> destructor of the request-object again.\n>>>>\n>>>> Also note the wrong comment, which claims that WrapRequest does not\n>>>> take ownership of the request, however, actually it already has\n>>>> ownership.\n>>>>\n>>>> Replacing request.reset() with request.release() doesn't call the\n>>>> destructor on the request-object and only one free happens at the end.\n>>>>\n>>>> Signed-off-by: Marian Cichy \n>>>> ---\n>>>>    src/gstreamer/gstlibcamerasrc.cpp | 6 ++++--\n>>>>    1 file changed, 4 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp \n>>>> b/src/gstreamer/gstlibcamerasrc.cpp\n>>>> index a8ed7652..b0194c2f 100644\n>>>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>>>> @@ -279,10 +279,12 @@ gst_libcamera_src_task_run(gpointer user_data)\n>>>>                                 &buffer, nullptr);\n>>>>            if (ret != GST_FLOW_OK) {\n>>>>                /*\n>>>> -             * RequestWrap does not take ownership, and we won't be\n>>>> +             * RequestWrap has ownership, and we won't be\n>>>>                 * queueing this one due to lack of buffers.\n>>>> +             * So the request will be freed when RequestWrap\n>>>> +             * goes out of scope.\n>>>>                 */\n>>>> -            request.reset();\n>>>> +            request.release();\n>>> This is a bit problematic as `request` is still in-used beyond this\n>>> line(there is `if (request)` block below) and here it will set it as\n>>> NULL. You will need to take care of this situation using the \n>>> RequestWrap\n>>> probably. However, one thing I still am not sure of ...\n>>>\n>>>>                break;\n>>>>            }\n>>> I am not sure if why `RequestWrap` is written intentionally in a \n>>> way, to\n>>> transfer the ownership of the `Request`. This fact is quite evident\n>>> looking at the ~RequestWrap().  The larger picture here is (which is \n>>> why\n>>> you hit this double-free issue) that, whenever RequestWrap is used -\n>>> always transfer ownership of Request. So, that's a 2-step manual in\n>>> order to use the wrap; if the latter is forgotten - we will always hit\n>>> double free issues.\n>>>\n>>> Can you please look how the RequestWrap is used elsewhere in the\n>>> gstreamer plugin and try to see why transfer of ownership is \n>>> intented in\n>>> RequestWrap? If it's not essential, we should probably re-fine\n>>> RequestWrap to really be a \"wrapper\" and not try to steal the reference\n>>> - and take the additional responsibility of free-ing it.\n>> The request belongs to the gstreamer element, so we need to keep track\n>> of it and free it. Request instances are managed with std::unique_ptr<>,\n>> so the best option I think would be to simple move the request to the\n>> wrap, using a unique pointer there.\n>>\n>> How about this untested change ?\n>>\n>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp \n>> b/src/gstreamer/gstlibcamerasrc.cpp\n>> index 636c14dff64e..e86c3d7f74c7 100644\n>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>> @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n>>   #define GST_CAT_DEFAULT source_debug\n>>     struct RequestWrap {\n>> -    RequestWrap(Request *request);\n>> +    RequestWrap(std::unique_ptr request);\n>>       ~RequestWrap();\n>>         void attachBuffer(GstBuffer *buffer);\n>>       GstBuffer *detachBuffer(Stream *stream);\n>>         /* For ptr comparison only. */\n>> -    Request *request_;\n>> +    std::unique_ptr request_;\n>>       std::map buffers_;\n>>   };\n>>   -RequestWrap::RequestWrap(Request *request)\n>> -    : request_(request)\n>> +RequestWrap::RequestWrap(std::unique_ptr request)\n>> +    : request_(std::move(request))\n>>   {\n>>   }\n>>   @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap()\n>>           if (item.second)\n>>               gst_buffer_unref(item.second);\n>>       }\n>> -\n>> -    delete request_;\n>>   }\n>>     void RequestWrap::attachBuffer(GstBuffer *buffer)\n>> @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request \n>> *request)\n>>       std::unique_ptr wrap = std::move(requests_.front());\n>>       requests_.pop();\n>>   -    g_return_if_fail(wrap->request_ == request);\n>> +    g_return_if_fail(wrap->request_.get() == request);\n>>         if ((request->status() == Request::RequestCancelled)) {\n>>           GST_DEBUG_OBJECT(src_, \"Request was cancelled\");\n>> @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n>>       GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n>>       GstLibcameraSrcState *state = self->state;\n>>   -    std::unique_ptr request = state->cam_->createRequest();\n>> -    auto wrap = std::make_unique(request.get());\n>> +    auto wrap = \n>> std::make_unique(state->cam_->createRequest());\n>>       for (GstPad *srcpad : state->srcpads_) {\n>>           GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n>>           GstBuffer *buffer;\n>> @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data)\n>>                                &buffer, nullptr);\n>>           if (ret != GST_FLOW_OK) {\n>>               /*\n>> -             * RequestWrap does not take ownership, and we won't be\n>> -             * queueing this one due to lack of buffers.\n>> +             * RequestWrap has ownership of the rquest, and we\n>> +             * won't be queueing this one due to lack of buffers.\n>>                */\n>> -            request.reset();\n>> +            wrap.release();\n>>               break;\n>>           }\n>>             wrap->attachBuffer(buffer);\n>>       }\n>>   -    if (request) {\n>> +    if (wrap) {\n>>           GLibLocker lock(GST_OBJECT(self));\n>>           GST_TRACE_OBJECT(self, \"Requesting buffers\");\n>> -        state->cam_->queueRequest(request.get());\n>> + state->cam_->queueRequest(wrap->request_.get());\n>\n>\n> Did you mean\n>\n> state->cam_->queueRequest(wrap->request_)\n>\n> the wrapped request is not a unique_ptr here.\n>\n>\n> With this change, I tested both the success and the error path and it \n> looks fine. However, I think there should be a printed message (maybe \n> GST_WARNING) when we get in the error path, as there is no indicator \n> now that buffer acquistion failed.\n>\n>\n> Regards,\n>\n> Marian\n>\n>\n\nForget about this. My Mail reader decided to clip half of the message.\n\n\nWorks fine, but there should still be an Error message if buffer \nacquisition failed.\n\n\nRegards,\n\nMarian\n\n\n>> state->requests_.push(std::move(wrap));\n>>   -        /* The request will be deleted in the completion handler. */\n>> -        request.release();\n>> +        /* The RequestWrap will be deleted in the completion \n>> handler. */\n>>       }\n>>         GstFlowReturn ret = GST_FLOW_OK;\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 089D2BD1F1\n\tfor ;\n\tWed, 10 Mar 2021 18:12:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C0B768A9C;\n\tWed, 10 Mar 2021 19:12:47 +0100 (CET)","from metis.ext.pengutronix.de (metis.ext.pengutronix.de\n\t[IPv6:2001:67c:670:201:290:27ff:fe1d:cc33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47B2160106\n\tfor ;\n\tWed, 10 Mar 2021 19:12:46 +0100 (CET)","from gallifrey.ext.pengutronix.de\n\t([2001:67c:670:201:5054:ff:fe8d:eefb] helo=[127.0.0.1])\n\tby metis.ext.pengutronix.de with esmtp (Exim 4.92)\n\t(envelope-from )\n\tid 1lK3K9-00042w-Mu; Wed, 10 Mar 2021 19:12:45 +0100"],"From":"Marian Cichy ","To":"Laurent Pinchart ","References":"<20210309143518.31405-1-m.cichy@pengutronix.de>\n\t<3a4776a0-239c-da2c-509c-46eac4aaae4c@uajain.com>\n\t\n\t<1850310f-4b44-e809-12ea-88ca3d4391a6@pengutronix.de>","Message-ID":"<6aacd08f-3c20-a619-b453-129d2400825f@pengutronix.de>","Date":"Wed, 10 Mar 2021 19:12:45 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.5.0","MIME-Version":"1.0","In-Reply-To":"<1850310f-4b44-e809-12ea-88ca3d4391a6@pengutronix.de>","Content-Language":"en-US","X-SA-Exim-Connect-IP":"2001:67c:670:201:5054:ff:fe8d:eefb","X-SA-Exim-Mail-From":"mci@pengutronix.de","X-SA-Exim-Scanned":"No (on metis.ext.pengutronix.de);\n\tSAEximRunCond expanded to false","X-PTX-Original-Recipient":"libcamera-devel@lists.libcamera.org","Subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, graphics@pengutronix.de","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15591,"web_url":"https://patchwork.libcamera.org/comment/15591/","msgid":"","date":"2021-03-10T18:44:43","subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Marian\n\nOn Wed, Mar 10, 2021 at 06:53:30PM +0100, Marian Cichy wrote:\n> On 3/10/21 6:30 PM, Laurent Pinchart wrote:\n> > On Wed, Mar 10, 2021 at 08:35:31PM +0530, Umang Jain wrote:\n> >> On 3/9/21 8:05 PM, Marian Cichy wrote:\n> >>> If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the\n> >>> unique_ptr to the request-object gets reset and hence, its destructor\n> >>> is called. However, the wrap-object points to the same object and is\n> >>> still alive at this moment. When the task_run-function is finished, the\n> >>> destructor of the wrap-object is called, which in return calls the\n> >>> destructor of the request-object again.\n> >>>\n> >>> Also note the wrong comment, which claims that WrapRequest does not\n> >>> take ownership of the request, however, actually it already has\n> >>> ownership.\n> >>>\n> >>> Replacing request.reset() with request.release() doesn't call the\n> >>> destructor on the request-object and only one free happens at the end.\n> >>>\n> >>> Signed-off-by: Marian Cichy \n> >>> ---\n> >>> src/gstreamer/gstlibcamerasrc.cpp | 6 ++++--\n> >>> 1 file changed, 4 insertions(+), 2 deletions(-)\n> >>>\n> >>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> >>> index a8ed7652..b0194c2f 100644\n> >>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> >>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> >>> @@ -279,10 +279,12 @@ gst_libcamera_src_task_run(gpointer user_data)\n> >>> \t\t\t\t\t\t &buffer, nullptr);\n> >>> \t\tif (ret != GST_FLOW_OK) {\n> >>> \t\t\t/*\n> >>> -\t\t\t * RequestWrap does not take ownership, and we won't be\n> >>> +\t\t\t * RequestWrap has ownership, and we won't be\n> >>> \t\t\t * queueing this one due to lack of buffers.\n> >>> +\t\t\t * So the request will be freed when RequestWrap\n> >>> +\t\t\t * goes out of scope.\n> >>> \t\t\t */\n> >>> -\t\t\trequest.reset();\n> >>> +\t\t\trequest.release();\n> >> This is a bit problematic as `request` is still in-used beyond this\n> >> line(there is `if (request)` block below) and here it will set it as\n> >> NULL. You will need to take care of this situation using the RequestWrap\n> >> probably. However, one thing I still am not sure of ...\n> >>\n> >>> \t\t\tbreak;\n> >>> \t\t}\n> >>> \n> >> I am not sure if why `RequestWrap` is written intentionally in a way, to\n> >> transfer the ownership of the `Request`. This fact is quite evident\n> >> looking at the ~RequestWrap().  The larger picture here is (which is why\n> >> you hit this double-free issue) that, whenever RequestWrap is used -\n> >> always transfer ownership of Request. So, that's a 2-step manual in\n> >> order to use the wrap; if the latter is forgotten - we will always hit\n> >> double free issues.\n> >>\n> >> Can you please look how the RequestWrap is used elsewhere in the\n> >> gstreamer plugin and try to see why transfer of ownership is intented in\n> >> RequestWrap? If it's not essential, we should probably re-fine\n> >> RequestWrap to really be a \"wrapper\" and not try to steal the reference\n> >> - and take the additional responsibility of free-ing it.\n> > The request belongs to the gstreamer element, so we need to keep track\n> > of it and free it. Request instances are managed with std::unique_ptr<>,\n> > so the best option I think would be to simple move the request to the\n> > wrap, using a unique pointer there.\n> >\n> > How about this untested change ?\n> >\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 636c14dff64e..e86c3d7f74c7 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n> > #define GST_CAT_DEFAULT source_debug\n> > \n> > struct RequestWrap {\n> > -\tRequestWrap(Request *request);\n> > +\tRequestWrap(std::unique_ptr request);\n> > \t~RequestWrap();\n> > \n> > \tvoid attachBuffer(GstBuffer *buffer);\n> > \tGstBuffer *detachBuffer(Stream *stream);\n> > \n> > \t/* For ptr comparison only. */\n> > -\tRequest *request_;\n> > +\tstd::unique_ptr request_;\n\n*1 (marker for the discussion below)\n\n> > \tstd::map buffers_;\n> > };\n> > \n> > -RequestWrap::RequestWrap(Request *request)\n> > -\t: request_(request)\n> > +RequestWrap::RequestWrap(std::unique_ptr request)\n> > +\t: request_(std::move(request))\n> > {\n> > }\n> > \n> > @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap()\n> > \t\tif (item.second)\n> > \t\t\tgst_buffer_unref(item.second);\n> > \t}\n> > -\n> > -\tdelete request_;\n> > }\n> > \n> > void RequestWrap::attachBuffer(GstBuffer *buffer)\n> > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> > \tstd::unique_ptr wrap = std::move(requests_.front());\n> > \trequests_.pop();\n> > \n> > -\tg_return_if_fail(wrap->request_ == request);\n> > +\tg_return_if_fail(wrap->request_.get() == request);\n> > \n> > \tif ((request->status() == Request::RequestCancelled)) {\n> > \t\tGST_DEBUG_OBJECT(src_, \"Request was cancelled\");\n> > @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > \tGstLibcameraSrcState *state = self->state;\n> > \n> > -\tstd::unique_ptr request = state->cam_->createRequest();\n> > -\tauto wrap = std::make_unique(request.get());\n> > +\tauto wrap = std::make_unique(state->cam_->createRequest());\n> > \tfor (GstPad *srcpad : state->srcpads_) {\n> > \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n> > \t\tGstBuffer *buffer;\n> > @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > \t\t\t\t\t\t &buffer, nullptr);\n> > \t\tif (ret != GST_FLOW_OK) {\n> > \t\t\t/*\n> > -\t\t\t * RequestWrap does not take ownership, and we won't be\n> > -\t\t\t * queueing this one due to lack of buffers.\n> > +\t\t\t * RequestWrap has ownership of the rquest, and we\n> > +\t\t\t * won't be queueing this one due to lack of buffers.\n> > \t\t\t */\n> > -\t\t\trequest.reset();\n> > +\t\t\twrap.release();\n> > \t\t\tbreak;\n> > \t\t}\n> > \n> > \t\twrap->attachBuffer(buffer);\n> > \t}\n> > \n> > -\tif (request) {\n> > +\tif (wrap) {\n> > \t\tGLibLocker lock(GST_OBJECT(self));\n> > \t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> > -\t\tstate->cam_->queueRequest(request.get());\n> > +\t\tstate->cam_->queueRequest(wrap->request_.get());\n> \n> \n> Did you mean\n> \n> state->cam_->queueRequest(wrap->request_)\n> \n> the wrapped request is not a unique_ptr here.\n\nIsn't it ? *1 changes request_ from a Request * to a\nstd::unique_ptr.\n\n> With this change, I tested both the success and the error path and it \n> looks fine. However, I think there should be a printed message (maybe \n> GST_WARNING) when we get in the error path, as there is no indicator now \n> that buffer acquistion failed.\n\nGood idea.\n\nStupid question, how do you trigger that error ?\n\n> > \t\tstate->requests_.push(std::move(wrap));\n> > \n> > -\t\t/* The request will be deleted in the completion handler. */\n> > -\t\trequest.release();\n> > +\t\t/* The RequestWrap will be deleted in the completion handler. */\n> > \t}\n> > \n> > \tGstFlowReturn ret = GST_FLOW_OK;","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 B7B46BD80C\n\tfor ;\n\tWed, 10 Mar 2021 18:45:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13DAF68A9C;\n\tWed, 10 Mar 2021 19:45: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 D9DB760106\n\tfor ;\n\tWed, 10 Mar 2021 19:45:15 +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 482D49F0;\n\tWed, 10 Mar 2021 19:45:15 +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=\"ek6gw/6s\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615401915;\n\tbh=WGrRaFHYfOhGoSSeoRDdpKon9A9tA5/YxApNXCz3OWM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ek6gw/6stOozCOpayuugafXcpj9NvsnLXCH0DXK3GAO4fScmGD8J92thYe4jiT97u\n\tPnyHaYRjxXDx3KvS96PUdmFmsNGD4jF8NHmlxev1js45v34OwyxmTPZJt9guaPfvqW\n\tygq54AdNpVvKfJQhQ+fx8qEvx2R/8tKkusSBr/xw=","Date":"Wed, 10 Mar 2021 20:44:43 +0200","From":"Laurent Pinchart ","To":"Marian Cichy ","Message-ID":"","References":"<20210309143518.31405-1-m.cichy@pengutronix.de>\n\t<3a4776a0-239c-da2c-509c-46eac4aaae4c@uajain.com>\n\t\n\t<1850310f-4b44-e809-12ea-88ca3d4391a6@pengutronix.de>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<1850310f-4b44-e809-12ea-88ca3d4391a6@pengutronix.de>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, graphics@pengutronix.de","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15592,"web_url":"https://patchwork.libcamera.org/comment/15592/","msgid":"<2bb5e0f7-95b4-aec0-5b9b-d2f255e5def5@pengutronix.de>","date":"2021-03-10T18:48:23","subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","submitter":{"id":81,"url":"https://patchwork.libcamera.org/api/people/81/","name":"Marian Cichy","email":"mci@pengutronix.de"},"content":"On 3/10/21 7:44 PM, Laurent Pinchart wrote:\n> Hi Marian\n>\n> On Wed, Mar 10, 2021 at 06:53:30PM +0100, Marian Cichy wrote:\n>> On 3/10/21 6:30 PM, Laurent Pinchart wrote:\n>>> On Wed, Mar 10, 2021 at 08:35:31PM +0530, Umang Jain wrote:\n>>>> On 3/9/21 8:05 PM, Marian Cichy wrote:\n>>>>> If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the\n>>>>> unique_ptr to the request-object gets reset and hence, its destructor\n>>>>> is called. However, the wrap-object points to the same object and is\n>>>>> still alive at this moment. When the task_run-function is finished, the\n>>>>> destructor of the wrap-object is called, which in return calls the\n>>>>> destructor of the request-object again.\n>>>>>\n>>>>> Also note the wrong comment, which claims that WrapRequest does not\n>>>>> take ownership of the request, however, actually it already has\n>>>>> ownership.\n>>>>>\n>>>>> Replacing request.reset() with request.release() doesn't call the\n>>>>> destructor on the request-object and only one free happens at the end.\n>>>>>\n>>>>> Signed-off-by: Marian Cichy \n>>>>> ---\n>>>>> src/gstreamer/gstlibcamerasrc.cpp | 6 ++++--\n>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)\n>>>>>\n>>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>>>>> index a8ed7652..b0194c2f 100644\n>>>>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>>>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>>>>> @@ -279,10 +279,12 @@ gst_libcamera_src_task_run(gpointer user_data)\n>>>>> \t\t\t\t\t\t &buffer, nullptr);\n>>>>> \t\tif (ret != GST_FLOW_OK) {\n>>>>> \t\t\t/*\n>>>>> -\t\t\t * RequestWrap does not take ownership, and we won't be\n>>>>> +\t\t\t * RequestWrap has ownership, and we won't be\n>>>>> \t\t\t * queueing this one due to lack of buffers.\n>>>>> +\t\t\t * So the request will be freed when RequestWrap\n>>>>> +\t\t\t * goes out of scope.\n>>>>> \t\t\t */\n>>>>> -\t\t\trequest.reset();\n>>>>> +\t\t\trequest.release();\n>>>> This is a bit problematic as `request` is still in-used beyond this\n>>>> line(there is `if (request)` block below) and here it will set it as\n>>>> NULL. You will need to take care of this situation using the RequestWrap\n>>>> probably. However, one thing I still am not sure of ...\n>>>>\n>>>>> \t\t\tbreak;\n>>>>> \t\t}\n>>>>> \n>>>> I am not sure if why `RequestWrap` is written intentionally in a way, to\n>>>> transfer the ownership of the `Request`. This fact is quite evident\n>>>> looking at the ~RequestWrap().  The larger picture here is (which is why\n>>>> you hit this double-free issue) that, whenever RequestWrap is used -\n>>>> always transfer ownership of Request. So, that's a 2-step manual in\n>>>> order to use the wrap; if the latter is forgotten - we will always hit\n>>>> double free issues.\n>>>>\n>>>> Can you please look how the RequestWrap is used elsewhere in the\n>>>> gstreamer plugin and try to see why transfer of ownership is intented in\n>>>> RequestWrap? If it's not essential, we should probably re-fine\n>>>> RequestWrap to really be a \"wrapper\" and not try to steal the reference\n>>>> - and take the additional responsibility of free-ing it.\n>>> The request belongs to the gstreamer element, so we need to keep track\n>>> of it and free it. Request instances are managed with std::unique_ptr<>,\n>>> so the best option I think would be to simple move the request to the\n>>> wrap, using a unique pointer there.\n>>>\n>>> How about this untested change ?\n>>>\n>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>>> index 636c14dff64e..e86c3d7f74c7 100644\n>>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>>> @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n>>> #define GST_CAT_DEFAULT source_debug\n>>> \n>>> struct RequestWrap {\n>>> -\tRequestWrap(Request *request);\n>>> +\tRequestWrap(std::unique_ptr request);\n>>> \t~RequestWrap();\n>>> \n>>> \tvoid attachBuffer(GstBuffer *buffer);\n>>> \tGstBuffer *detachBuffer(Stream *stream);\n>>> \n>>> \t/* For ptr comparison only. */\n>>> -\tRequest *request_;\n>>> +\tstd::unique_ptr request_;\n> *1 (marker for the discussion below)\n>\n>>> \tstd::map buffers_;\n>>> };\n>>> \n>>> -RequestWrap::RequestWrap(Request *request)\n>>> -\t: request_(request)\n>>> +RequestWrap::RequestWrap(std::unique_ptr request)\n>>> +\t: request_(std::move(request))\n>>> {\n>>> }\n>>> \n>>> @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap()\n>>> \t\tif (item.second)\n>>> \t\t\tgst_buffer_unref(item.second);\n>>> \t}\n>>> -\n>>> -\tdelete request_;\n>>> }\n>>> \n>>> void RequestWrap::attachBuffer(GstBuffer *buffer)\n>>> @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n>>> \tstd::unique_ptr wrap = std::move(requests_.front());\n>>> \trequests_.pop();\n>>> \n>>> -\tg_return_if_fail(wrap->request_ == request);\n>>> +\tg_return_if_fail(wrap->request_.get() == request);\n>>> \n>>> \tif ((request->status() == Request::RequestCancelled)) {\n>>> \t\tGST_DEBUG_OBJECT(src_, \"Request was cancelled\");\n>>> @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n>>> \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n>>> \tGstLibcameraSrcState *state = self->state;\n>>> \n>>> -\tstd::unique_ptr request = state->cam_->createRequest();\n>>> -\tauto wrap = std::make_unique(request.get());\n>>> +\tauto wrap = std::make_unique(state->cam_->createRequest());\n>>> \tfor (GstPad *srcpad : state->srcpads_) {\n>>> \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n>>> \t\tGstBuffer *buffer;\n>>> @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data)\n>>> \t\t\t\t\t\t &buffer, nullptr);\n>>> \t\tif (ret != GST_FLOW_OK) {\n>>> \t\t\t/*\n>>> -\t\t\t * RequestWrap does not take ownership, and we won't be\n>>> -\t\t\t * queueing this one due to lack of buffers.\n>>> +\t\t\t * RequestWrap has ownership of the rquest, and we\n>>> +\t\t\t * won't be queueing this one due to lack of buffers.\n>>> \t\t\t */\n>>> -\t\t\trequest.reset();\n>>> +\t\t\twrap.release();\n>>> \t\t\tbreak;\n>>> \t\t}\n>>> \n>>> \t\twrap->attachBuffer(buffer);\n>>> \t}\n>>> \n>>> -\tif (request) {\n>>> +\tif (wrap) {\n>>> \t\tGLibLocker lock(GST_OBJECT(self));\n>>> \t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n>>> -\t\tstate->cam_->queueRequest(request.get());\n>>> +\t\tstate->cam_->queueRequest(wrap->request_.get());\n>>\n>> Did you mean\n>>\n>> state->cam_->queueRequest(wrap->request_)\n>>\n>> the wrapped request is not a unique_ptr here.\n> Isn't it ? *1 changes request_ from a Request * to a\n> std::unique_ptr.\n>\n>> With this change, I tested both the success and the error path and it\n>> looks fine. However, I think there should be a printed message (maybe\n>> GST_WARNING) when we get in the error path, as there is no indicator now\n>> that buffer acquistion failed.\n> Good idea.\n>\n> Stupid question, how do you trigger that error ?\n\n\nI had a situation with my special camera sensor and driver which \ntriggered it at first, some time ago, but I forgot what it was. Now I \njust triggered it by hard-coding ret = GST_FLOW_ERROR. :-)\n\n\n>\n>>> \t\tstate->requests_.push(std::move(wrap));\n>>> \n>>> -\t\t/* The request will be deleted in the completion handler. */\n>>> -\t\trequest.release();\n>>> +\t\t/* The RequestWrap will be deleted in the completion handler. */\n>>> \t}\n>>> \n>>> \tGstFlowReturn ret = GST_FLOW_OK;","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 51021BD80C\n\tfor ;\n\tWed, 10 Mar 2021 18:48:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDBBD68A9C;\n\tWed, 10 Mar 2021 19:48:24 +0100 (CET)","from metis.ext.pengutronix.de (metis.ext.pengutronix.de\n\t[IPv6:2001:67c:670:201:290:27ff:fe1d:cc33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FACA60106\n\tfor ;\n\tWed, 10 Mar 2021 19:48:24 +0100 (CET)","from gallifrey.ext.pengutronix.de\n\t([2001:67c:670:201:5054:ff:fe8d:eefb] helo=[127.0.0.1])\n\tby metis.ext.pengutronix.de with esmtp (Exim 4.92)\n\t(envelope-from )\n\tid 1lK3sd-0007eV-KN; Wed, 10 Mar 2021 19:48:23 +0100"],"To":"Laurent Pinchart ","References":"<20210309143518.31405-1-m.cichy@pengutronix.de>\n\t<3a4776a0-239c-da2c-509c-46eac4aaae4c@uajain.com>\n\t\n\t<1850310f-4b44-e809-12ea-88ca3d4391a6@pengutronix.de>\n\t","From":"Marian Cichy ","Message-ID":"<2bb5e0f7-95b4-aec0-5b9b-d2f255e5def5@pengutronix.de>","Date":"Wed, 10 Mar 2021 19:48:23 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.5.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","X-SA-Exim-Connect-IP":"2001:67c:670:201:5054:ff:fe8d:eefb","X-SA-Exim-Mail-From":"mci@pengutronix.de","X-SA-Exim-Scanned":"No (on metis.ext.pengutronix.de);\n\tSAEximRunCond expanded to false","X-PTX-Original-Recipient":"libcamera-devel@lists.libcamera.org","Subject":"Re: [libcamera-devel] [PATCH] libcamera: gst: Fix double-free when\n\tacquire_buffer fails","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, graphics@pengutronix.de","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]