[{"id":15596,"web_url":"https://patchwork.libcamera.org/comment/15596/","msgid":"<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>","date":"2021-03-11T11:35:39","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_buffer fails","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Marian,\n\nI noticed a few potential null/segfault issues with this approach\n\nOn 3/11/21 3:03 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> Instead of taking care of both, the request and the wrap-object, we can\n> move the request to the wrap which will then effectively take care of\n> the request object automatically.\n...take care of the request s/object/object-deletion/ maybe?\n\n>\n> Signed-off-by: Marian Cichy \n> Suggested-by: Laurent Pinchart \n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------\n> 1 file changed, 12 insertions(+), 16 deletions(-)\n>\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 636c14df..e86c3d7f 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. */\nI think you need to remove this comment too\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*1 for marker below\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\ns/rquest/request/\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) {\nI think this should be `if (wrap.get())` no? `wrap.release()` if hit, \nwill set nullptr to the _managed_ object, so we need to check the \nmanaged object for null\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());\nfrom *1, createRequest() call can return nullptr, hence you have a\n\nstd::make_unique(nullptr);\n\nwhich will set request_ as nullptr on the wrap object. It  is fine as \nlong as wrap->request_ is not used unlike here, in this block.\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 EDA33BD80C\n\tfor ;\n\tThu, 11 Mar 2021 11:35:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 57382602ED;\n\tThu, 11 Mar 2021 12:35:45 +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 44327602E8\n\tfor ;\n\tThu, 11 Mar 2021 12:35:43 +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=\"vNz0ZwDL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1615462542; bh=uwGZLCJnr8FTjVHJisW+n97z0grJX33cfH8s95IE8LY=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=vNz0ZwDLQ/uq/wGSrX+7mOKLko7lD95gFU4qRcVj9RW16JQ1E+q1vJKpv3K6gmCxT\n\tBWppCthnwj06u354omTxVHCTqUOjS6HF5t3NUAfKrBfzmDeK9pvH1WXNRsbrl6Wnlw\n\tkZ6qBGjAi8/hG5eaiZfSIbiK3ggQ111NTCndtCEHdY/CSeX09Lfk7HYTHm5nqCbAoq\n\tsb4yD7kV6z22KI6J86UzDCFXFj8wfvkcWjr2Wqce7hsaG6G7e9Ng8Lx8Csyl68V/24\n\tXTNNLSw06PMoq8xZBwv+evsQDIT9Yqsf/TxP3/C0Is8CundBekszdT8oNwQPqrNVf8\n\tRIdMtbOEwyCvA==","To":"Marian Cichy ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>","From":"Umang Jain ","Message-ID":"<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>","Date":"Thu, 11 Mar 2021 17:05:39 +0530","Mime-Version":"1.0","In-Reply-To":"<20210311093325.8933-1-m.cichy@pengutronix.de>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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":15598,"web_url":"https://patchwork.libcamera.org/comment/15598/","msgid":"","date":"2021-03-11T11:59:36","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_buffer fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:\n> \n> Hi Marian,\n> \n> I noticed a few potential null/segfault issues with this approach\n> \n> On 3/11/21 3:03 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> > Instead of taking care of both, the request and the wrap-object, we can\n> > move the request to the wrap which will then effectively take care of\n> > the request object automatically.\n>\n> ...take care of the request s/object/object-deletion/ maybe?\n> \n> >\n> > Signed-off-by: Marian Cichy \n> > Suggested-by: Laurent Pinchart \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------\n> > 1 file changed, 12 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 636c14df..e86c3d7f 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>\n> I think you need to remove this comment too\n\nIndeed :-)\n\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>\n> *1 for marker below\n>\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>\n> s/rquest/request/\n>\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>\n> I think this should be `if (wrap.get())` no? `wrap.release()` if hit, \n> will set nullptr to the _managed_ object, so we need to check the \n> managed object for null\n\nThis is what is happening already:\nhttps://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool\n\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> from *1, createRequest() call can return nullptr, hence you have a\n> \n> std::make_unique(nullptr);\n> \n> which will set request_ as nullptr on the wrap object. It  is fine as \n> long as wrap->request_ is not used unlike here, in this block.\n\nThat's indeed a problem. Maybe\n\n\tif (wrap && wrap->request_)\n\nabove ? Nicolas, any opinion ?\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 77DB3BD80C\n\tfor ;\n\tThu, 11 Mar 2021 12:00:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE4B668C69;\n\tThu, 11 Mar 2021 13:00:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C7D3602E8\n\tfor ;\n\tThu, 11 Mar 2021 13:00:11 +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 2EEE7879;\n\tThu, 11 Mar 2021 13:00:10 +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=\"Ij6OhUEE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615464010;\n\tbh=SccemkASzigARY98OQghqcO1LILcNTpcpSmXRkpKY1s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ij6OhUEEcOEYkZOUjDGliwd/ezGB32a44vVBpuZcM17QnAMPSCp77H1Bxz5eu8hOT\n\tlZxxUOjo9TRZeSSQe2dqIrPRgeMbLeA2PMwB2zniReSqZUkMniYCM7Umus9AziCKLR\n\tbZFp/n1rcwvCqY8tPZdLXsUpKd+QThAw2/3RSNcg=","Date":"Thu, 11 Mar 2021 13:59:36 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>\n\t<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org, graphics@pengutronix.de,\n\tMarian Cichy ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15599,"web_url":"https://patchwork.libcamera.org/comment/15599/","msgid":"<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>","date":"2021-03-11T12:17:59","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_buffer fails","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nOn 3/11/21 5:29 PM, Laurent Pinchart wrote:\n> Hello,\n>\n> On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:\n>> Hi Marian,\n>>\n>> I noticed a few potential null/segfault issues with this approach\n>>\n>> On 3/11/21 3:03 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>>> Instead of taking care of both, the request and the wrap-object, we can\n>>> move the request to the wrap which will then effectively take care of\n>>> the request object automatically.\n>> ...take care of the request s/object/object-deletion/ maybe?\n>>\n>>> Signed-off-by: Marian Cichy \n>>> Suggested-by: Laurent Pinchart \n>>> ---\n>>> src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------\n>>> 1 file changed, 12 insertions(+), 16 deletions(-)\n>>>\n>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>>> index 636c14df..e86c3d7f 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>> I think you need to remove this comment too\n> Indeed :-)\n>\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>> *1 for marker below\n>>\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>> s/rquest/request/\n>>\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>> I think this should be `if (wrap.get())` no? `wrap.release()` if hit,\n>> will set nullptr to the _managed_ object, so we need to check the\n>> managed object for null\n> This is what is happening already:\n> https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool\n>\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>> from *1, createRequest() call can return nullptr, hence you have a\n>>\n>> std::make_unique(nullptr);\n>>\n>> which will set request_ as nullptr on the wrap object. It  is fine as\n>> long as wrap->request_ is not used unlike here, in this block.\n> That's indeed a problem. Maybe\n>\n> \tif (wrap && wrap->request_)\n>\n> above ? Nicolas, any opinion ?\nOR null-check createRequest()'s returned object before wrapping it (i.e. \nensure RequestWrap doesn't wrap a nullptr from the start), so we don't \nhave to check everything for the wrap->request_ is valid or not.\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 A6316BD1F1\n\tfor ;\n\tThu, 11 Mar 2021 12:18:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 952D968C6B;\n\tThu, 11 Mar 2021 13:18:05 +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 C4BC6602E8\n\tfor ;\n\tThu, 11 Mar 2021 13:18:03 +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=\"j1zXyeCF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1615465083; bh=ZMRKyEpAgsMFL9dRrUIXFEnCmnbGFPESuCmcrxuhyoU=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=j1zXyeCFGFpmbXZop/7kzwRTGAb8l1L9ChM1lazUBxaLwPn1KhVSEQZzVn6sYDQoZ\n\tTx0zHmMvpCGzjMN1FU2uNdmlv+OMRKqW9FIsV9YLdpENBbowX9L3zCRGfA7JIha/q8\n\tiLXcNfDNptjq03WhcGXfOBlnb52q0SSSWFCkwPgw1ThZm+GDmJcesRD5d3eTtLTc8m\n\tSrIgqtsHf2bXBO5ZIyuHOvS+S4kKvn7WA2Q2LikJkbPwwc8LYHgghnM54TzS+wnxcD\n\trRj1WmcE4FXbmMJxGBZS+26Vq1g2RhtkaLA2e8ZQwycQXplhmPP8Lrf0lGCUdi2rM0\n\tZN2wl50fAM4TA==","To":"Laurent Pinchart ","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>\n\t<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>\n\t","From":"Umang Jain ","Message-ID":"<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>","Date":"Thu, 11 Mar 2021 17:47:59 +0530","Mime-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org, graphics@pengutronix.de,\n\tMarian Cichy ","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":15600,"web_url":"https://patchwork.libcamera.org/comment/15600/","msgid":"","date":"2021-03-11T12:22:42","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_buffer fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote:\n> On 3/11/21 5:29 PM, Laurent Pinchart wrote:\n> > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:\n> >> On 3/11/21 3:03 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> >>> Instead of taking care of both, the request and the wrap-object, we can\n> >>> move the request to the wrap which will then effectively take care of\n> >>> the request object automatically.\n> >> ...take care of the request s/object/object-deletion/ maybe?\n> >>\n> >>> Signed-off-by: Marian Cichy \n> >>> Suggested-by: Laurent Pinchart \n> >>> ---\n> >>> src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------\n> >>> 1 file changed, 12 insertions(+), 16 deletions(-)\n> >>>\n> >>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> >>> index 636c14df..e86c3d7f 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> >>\n> >> I think you need to remove this comment too\n> >\n> > Indeed :-)\n> >\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> >\n> >> *1 for marker below\n> >>\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> >>\n> >> s/rquest/request/\n> >>\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> >>\n> >> I think this should be `if (wrap.get())` no? `wrap.release()` if hit,\n> >> will set nullptr to the _managed_ object, so we need to check the\n> >> managed object for null\n> >\n> > This is what is happening already:\n> > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool\n> >\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> >> from *1, createRequest() call can return nullptr, hence you have a\n> >>\n> >> std::make_unique(nullptr);\n> >>\n> >> which will set request_ as nullptr on the wrap object. It  is fine as\n> >> long as wrap->request_ is not used unlike here, in this block.\n> >\n> > That's indeed a problem. Maybe\n> >\n> > \tif (wrap && wrap->request_)\n> >\n> > above ? Nicolas, any opinion ?\n>\n> OR null-check createRequest()'s returned object before wrapping it (i.e. \n> ensure RequestWrap doesn't wrap a nullptr from the start), so we don't \n> have to check everything for the wrap->request_ is valid or not.\n\nBut, as far as I understand, code further down this function still have\nto run in that case, we can't just do an early return using a null\ncheck. That's why I've asked for Nicolas' feedback, he knows this code\nmuch better than I do.\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 5A31CBD80C\n\tfor ;\n\tThu, 11 Mar 2021 12:23:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB8D068C69;\n\tThu, 11 Mar 2021 13:23:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E7F04602E8\n\tfor ;\n\tThu, 11 Mar 2021 13:23:16 +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 697A8879;\n\tThu, 11 Mar 2021 13:23:16 +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=\"eh3fc8Q7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615465396;\n\tbh=RmEQuUDpM0NGHp/JPK+iEG/0KP1DoTQAJmmejoTTNDM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eh3fc8Q70KmiA6iofPV2Re9tv6ksT+6kZn7Tm9f3hNHFb8xpAADws57KuQNnZdCC9\n\t+3bdMlyWYXtYSh2DuSCUI/+NCtkVy6/K7fk+5RGZdye1kOGIpDP8LxtvfMM4Xxmami\n\t5S8VLLE7pQeM0MtykJDv6ByLt4c6/U7TlFC2GCW0=","Date":"Thu, 11 Mar 2021 14:22:42 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>\n\t<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>\n\t\n\t<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org, graphics@pengutronix.de,\n\tMarian Cichy ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15601,"web_url":"https://patchwork.libcamera.org/comment/15601/","msgid":"","date":"2021-03-11T20:35:35","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_buffer fails","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le jeudi 11 mars 2021 à 13:59 +0200, Laurent Pinchart a écrit :\n> Hello,\n> \n> On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:\n> > \n> > Hi Marian,\n> > \n> > I noticed a few potential null/segfault issues with this approach\n> > \n> > On 3/11/21 3:03 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> > > Instead of taking care of both, the request and the wrap-object, we can\n> > > move the request to the wrap which will then effectively take care of\n> > > the request object automatically.\n> > \n> > ...take care of the request s/object/object-deletion/ maybe?\n> > \n> > > \n> > > Signed-off-by: Marian Cichy \n> > > Suggested-by: Laurent Pinchart \n> > > ---\n> > >   src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------\n> > >   1 file changed, 12 insertions(+), 16 deletions(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index 636c14df..e86c3d7f 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> > > -       RequestWrap(Request *request);\n> > > +       RequestWrap(std::unique_ptr request);\n> > >         ~RequestWrap();\n> > >   \n> > >         void attachBuffer(GstBuffer *buffer);\n> > >         GstBuffer *detachBuffer(Stream *stream);\n> > >   \n> > >         /* For ptr comparison only. */\n> > \n> > I think you need to remove this comment too\n> \n> Indeed :-)\n> \n> > > -       Request *request_;\n> > > +       std::unique_ptr request_;\n> > >         std::map buffers_;\n> > >   };\n> > >   \n> > > -RequestWrap::RequestWrap(Request *request)\n> > > -       : request_(request)\n> > > +RequestWrap::RequestWrap(std::unique_ptr request)\n> > > +       : request_(std::move(request))\n> > >   {\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> > >   \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> > >   \n> > > -       g_return_if_fail(wrap->request_ == request);\n> > > +       g_return_if_fail(wrap->request_.get() == request);\n> > >   \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> > >   \n> > > -       std::unique_ptr request = state->cam_->createRequest();\n> > > -       auto wrap = std::make_unique(request.get());\n> > > +       auto wrap =\n\nPlease keep seperate, and add missing nullptr check in between.\n\n> > > std::make_unique(state->cam_->createRequest());\n> > \n> > *1 for marker below\n> > \n> > >         for (GstPad *srcpad : state->srcpads_) {\n> > >                 GstLibcameraPool *pool =\n> > > 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\n> > > won't be\n> > > -                        * queueing this one due to lack of buffers.\n> > > +                        * RequestWrap has ownership of the rquest, and we\n> > \n> > s/rquest/request/\n> > \n> > > +                        * won't be queueing this one due to lack of\n> > > buffers.\n> > >                          */\n> > > -                       request.reset();\n> > > +                       wrap.release();\n> > >                         break;\n> > >                 }\n> > >   \n> > >                 wrap->attachBuffer(buffer);\n> > >         }\n> > >   \n> > > -       if (request) {\n> > > +       if (wrap) {\n> > \n> > I think this should be `if (wrap.get())` no? `wrap.release()` if hit, \n> > will set nullptr to the _managed_ object, so we need to check the \n> > managed object for null\n> \n> This is what is happening already:\n> https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool\n> \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> > from *1, createRequest() call can return nullptr, hence you have a\n> > \n> > std::make_unique(nullptr);\n> > \n> > which will set request_ as nullptr on the wrap object. It  is fine as \n> > long as wrap->request_ is not used unlike here, in this block.\n> \n> That's indeed a problem. Maybe\n> \n>         if (wrap && wrap->request_)\n> \n> above ? Nicolas, any opinion ?\n\nThis way we don't have to be defensive in so many places.\n\n> \n> > >                 state->requests_.push(std::move(wrap));\n> > >   \n> > > -               /* The request will be deleted in the completion handler.\n> > > */\n> > > -               request.release();\n> > > +               /* The RequestWrap will be deleted in the completion\n> > > handler. */\n> > >         }\n> > >   \n> > >         GstFlowReturn ret = GST_FLOW_OK;\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 A6156BD1F1\n\tfor ;\n\tThu, 11 Mar 2021 20:35:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D1FCD68C69;\n\tThu, 11 Mar 2021 21:35:47 +0100 (CET)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0AA5C602E8\n\tfor ;\n\tThu, 11 Mar 2021 21:35:45 +0100 (CET)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nicolas) with ESMTPSA id 03A771F466E1"],"Message-ID":"","From":"Nicolas Dufresne ","To":"Laurent Pinchart , Umang Jain\n\t","Date":"Thu, 11 Mar 2021 15:35:35 -0500","In-Reply-To":"","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>\n\t<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>\n\t","User-Agent":"Evolution 3.38.3 (3.38.3-1.fc33) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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,\n\tMarian Cichy ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15602,"web_url":"https://patchwork.libcamera.org/comment/15602/","msgid":"<979b3c4eec0e36efdb3d38069ef558f843a0c97e.camel@collabora.com>","date":"2021-03-11T20:36:56","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_buffer fails","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le jeudi 11 mars 2021 à 17:47 +0530, Umang Jain a écrit :\n> Hi Laurent,\n> \n> On 3/11/21 5:29 PM, Laurent Pinchart wrote:\n> > Hello,\n> > \n> > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:\n> > > Hi Marian,\n> > > \n> > > I noticed a few potential null/segfault issues with this approach\n> > > \n> > > On 3/11/21 3:03 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> > > > Instead of taking care of both, the request and the wrap-object, we can\n> > > > move the request to the wrap which will then effectively take care of\n> > > > the request object automatically.\n> > > ...take care of the request s/object/object-deletion/ maybe?\n> > > \n> > > > Signed-off-by: Marian Cichy \n> > > > Suggested-by: Laurent Pinchart \n> > > > ---\n> > > >    src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------\n> > > >    1 file changed, 12 insertions(+), 16 deletions(-)\n> > > > \n> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > index 636c14df..e86c3d7f 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> > > > -       RequestWrap(Request *request);\n> > > > +       RequestWrap(std::unique_ptr request);\n> > > >         ~RequestWrap();\n> > > >    \n> > > >         void attachBuffer(GstBuffer *buffer);\n> > > >         GstBuffer *detachBuffer(Stream *stream);\n> > > >    \n> > > >         /* For ptr comparison only. */\n> > > I think you need to remove this comment too\n> > Indeed :-)\n> > \n> > > > -       Request *request_;\n> > > > +       std::unique_ptr request_;\n> > > >         std::map buffers_;\n> > > >    };\n> > > >    \n> > > > -RequestWrap::RequestWrap(Request *request)\n> > > > -       : request_(request)\n> > > > +RequestWrap::RequestWrap(std::unique_ptr request)\n> > > > +       : request_(std::move(request))\n> > > >    {\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> > > >    \n> > > >    void RequestWrap::attachBuffer(GstBuffer *buffer)\n> > > > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request\n> > > > *request)\n> > > >         std::unique_ptr wrap =\n> > > > std::move(requests_.front());\n> > > >         requests_.pop();\n> > > >    \n> > > > -       g_return_if_fail(wrap->request_ == request);\n> > > > +       g_return_if_fail(wrap->request_.get() == request);\n> > > >    \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> > > >    \n> > > > -       std::unique_ptr request = state->cam_->createRequest();\n> > > > -       auto wrap = std::make_unique(request.get());\n> > > > +       auto wrap = std::make_unique(state->cam_-\n> > > > >createRequest());\n> > > *1 for marker below\n> > > \n> > > >         for (GstPad *srcpad : state->srcpads_) {\n> > > >                 GstLibcameraPool *pool =\n> > > > 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\n> > > > won't be\n> > > > -                        * queueing this one due to lack of buffers.\n> > > > +                        * RequestWrap has ownership of the rquest, and\n> > > > we\n> > > s/rquest/request/\n> > > \n> > > > +                        * won't be queueing this one due to lack of\n> > > > buffers.\n> > > >                          */\n> > > > -                       request.reset();\n> > > > +                       wrap.release();\n> > > >                         break;\n> > > >                 }\n> > > >    \n> > > >                 wrap->attachBuffer(buffer);\n> > > >         }\n> > > >    \n> > > > -       if (request) {\n> > > > +       if (wrap) {\n> > > I think this should be `if (wrap.get())` no? `wrap.release()` if hit,\n> > > will set nullptr to the _managed_ object, so we need to check the\n> > > managed object for null\n> > This is what is happening already:\n> > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool\n> > \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> > > from *1, createRequest() call can return nullptr, hence you have a\n> > > \n> > > std::make_unique(nullptr);\n> > > \n> > > which will set request_ as nullptr on the wrap object. It  is fine as\n> > > long as wrap->request_ is not used unlike here, in this block.\n> > That's indeed a problem. Maybe\n> > \n> >         if (wrap && wrap->request_)\n> > \n> > above ? Nicolas, any opinion ?\n> OR null-check createRequest()'s returned object before wrapping it (i.e. \n> ensure RequestWrap doesn't wrap a nullptr from the start), so we don't \n> have to check everything for the wrap->request_ is valid or not.\n\nAgreed! I've just replied the same in previous email.\n\n> > \n> > > >                 state->requests_.push(std::move(wrap));\n> > > >    \n> > > > -               /* The request will be deleted in the completion\n> > > > handler. */\n> > > > -               request.release();\n> > > > +               /* The RequestWrap will be deleted in the completion\n> > > > handler. */\n> > > >         }\n> > > >    \n> > > >         GstFlowReturn ret = GST_FLOW_OK;\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 59140BD1F1\n\tfor ;\n\tThu, 11 Mar 2021 20:37:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21B52602ED;\n\tThu, 11 Mar 2021 21:37:08 +0100 (CET)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 404DC602E8\n\tfor ;\n\tThu, 11 Mar 2021 21:37:06 +0100 (CET)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nicolas) with ESMTPSA id 716871F466E1"],"Message-ID":"<979b3c4eec0e36efdb3d38069ef558f843a0c97e.camel@collabora.com>","From":"Nicolas Dufresne ","To":"Umang Jain , Laurent Pinchart\n\t","Date":"Thu, 11 Mar 2021 15:36:56 -0500","In-Reply-To":"<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>\n\t<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>\n\t\n\t<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>","User-Agent":"Evolution 3.38.3 (3.38.3-1.fc33) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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,\n\tMarian Cichy ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15603,"web_url":"https://patchwork.libcamera.org/comment/15603/","msgid":"<019d3ff94f759aaa56832e6241d732527ee39e1c.camel@collabora.com>","date":"2021-03-11T21:02:20","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_buffer fails","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit :\n> Hi Umang,\n> \n> On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote:\n> > On 3/11/21 5:29 PM, Laurent Pinchart wrote:\n> > > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:\n> > > > On 3/11/21 3:03 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> > > > > Instead of taking care of both, the request and the wrap-object, we\n> > > > > can\n> > > > > move the request to the wrap which will then effectively take care of\n> > > > > the request object automatically.\n> > > > ...take care of the request s/object/object-deletion/ maybe?\n> > > > \n> > > > > Signed-off-by: Marian Cichy \n> > > > > Suggested-by: Laurent Pinchart \n> > > > > ---\n> > > > >    src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------\n> > > > >    1 file changed, 12 insertions(+), 16 deletions(-)\n> > > > > \n> > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > index 636c14df..e86c3d7f 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> > > > > -       RequestWrap(Request *request);\n> > > > > +       RequestWrap(std::unique_ptr request);\n> > > > >         ~RequestWrap();\n> > > > >    \n> > > > >         void attachBuffer(GstBuffer *buffer);\n> > > > >         GstBuffer *detachBuffer(Stream *stream);\n> > > > >    \n> > > > >         /* For ptr comparison only. */\n> > > > \n> > > > I think you need to remove this comment too\n> > > \n> > > Indeed :-)\n> > > \n> > > > > -       Request *request_;\n> > > > > +       std::unique_ptr request_;\n> > > > >         std::map buffers_;\n> > > > >    };\n> > > > >    \n> > > > > -RequestWrap::RequestWrap(Request *request)\n> > > > > -       : request_(request)\n> > > > > +RequestWrap::RequestWrap(std::unique_ptr request)\n> > > > > +       : request_(std::move(request))\n> > > > >    {\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> > > > >    \n> > > > >    void RequestWrap::attachBuffer(GstBuffer *buffer)\n> > > > > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request\n> > > > > *request)\n> > > > >         std::unique_ptr wrap =\n> > > > > std::move(requests_.front());\n> > > > >         requests_.pop();\n> > > > >    \n> > > > > -       g_return_if_fail(wrap->request_ == request);\n> > > > > +       g_return_if_fail(wrap->request_.get() == request);\n> > > > >    \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> > > > >    \n> > > > > -       std::unique_ptr request = state->cam_-\n> > > > > >createRequest();\n> > > > > -       auto wrap = std::make_unique(request.get());\n> > > > > +       auto wrap = std::make_unique(state->cam_-\n> > > > > >createRequest());\n> > > \n> > > > *1 for marker below\n> > > > \n> > > > >         for (GstPad *srcpad : state->srcpads_) {\n> > > > >                 GstLibcameraPool *pool =\n> > > > > gst_libcamera_pad_get_pool(srcpad);\n> > > > >                 GstBuffer *buffer;\n> > > > > @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > > > >                                                      &buffer,\n> > > > > nullptr);\n> > > > >                 if (ret != GST_FLOW_OK) {\n> > > > >                         /*\n> > > > > -                        * RequestWrap does not take ownership, and we\n> > > > > won't be\n> > > > > -                        * queueing this one due to lack of buffers.\n> > > > > +                        * RequestWrap has ownership of the rquest,\n> > > > > and we\n> > > > \n> > > > s/rquest/request/\n> > > > \n> > > > > +                        * won't be queueing this one due to lack of\n> > > > > buffers.\n> > > > >                          */\n> > > > > -                       request.reset();\n> > > > > +                       wrap.release();\n> > > > >                         break;\n> > > > >                 }\n> > > > >    \n> > > > >                 wrap->attachBuffer(buffer);\n> > > > >         }\n> > > > >    \n> > > > > -       if (request) {\n> > > > > +       if (wrap) {\n> > > > \n> > > > I think this should be `if (wrap.get())` no? `wrap.release()` if hit,\n> > > > will set nullptr to the _managed_ object, so we need to check the\n> > > > managed object for null\n> > > \n> > > This is what is happening already:\n> > > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool\n> > > \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> > > > from *1, createRequest() call can return nullptr, hence you have a\n> > > > \n> > > > std::make_unique(nullptr);\n> > > > \n> > > > which will set request_ as nullptr on the wrap object. It  is fine as\n> > > > long as wrap->request_ is not used unlike here, in this block.\n> > > \n> > > That's indeed a problem. Maybe\n> > > \n> > >         if (wrap && wrap->request_)\n> > > \n> > > above ? Nicolas, any opinion ?\n> > \n> > OR null-check createRequest()'s returned object before wrapping it (i.e. \n> > ensure RequestWrap doesn't wrap a nullptr from the start), so we don't \n> > have to check everything for the wrap->request_ is valid or not.\n> \n> But, as far as I understand, code further down this function still have\n> to run in that case, we can't just do an early return using a null\n> check. That's why I've asked for Nicolas' feedback, he knows this code\n> much better than I do.\n\nHmm, right. Something like this should do:\n\nif (!request) {\n\tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...;\n gst_task_stop(task);\n\treturn;\n}\n\n> \n> > > > >                 state->requests_.push(std::move(wrap));\n> > > > >    \n> > > > > -               /* The request will be deleted in the completion\n> > > > > handler. */\n> > > > > -               request.release();\n> > > > > +               /* The RequestWrap will be deleted in the completion\n> > > > > handler. */\n> > > > >         }\n> > > > >    \n> > > > >         GstFlowReturn ret = GST_FLOW_OK;\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 61FB2BD1F1\n\tfor ;\n\tThu, 11 Mar 2021 21:02:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6997602ED;\n\tThu, 11 Mar 2021 22:02:30 +0100 (CET)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD9EB602E8\n\tfor ;\n\tThu, 11 Mar 2021 22:02:29 +0100 (CET)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nicolas) with ESMTPSA id E0D071F466FC"],"Message-ID":"<019d3ff94f759aaa56832e6241d732527ee39e1c.camel@collabora.com>","From":"Nicolas Dufresne ","To":"Laurent Pinchart , Umang Jain\n\t","Date":"Thu, 11 Mar 2021 16:02:20 -0500","In-Reply-To":"","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>\n\t<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>\n\t\n\t<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>\n\t","User-Agent":"Evolution 3.38.3 (3.38.3-1.fc33) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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,\n\tMarian Cichy ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15606,"web_url":"https://patchwork.libcamera.org/comment/15606/","msgid":"","date":"2021-03-11T23:14:58","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_buffer fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Thu, Mar 11, 2021 at 04:02:20PM -0500, Nicolas Dufresne wrote:\n> Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit :\n> > On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote:\n> > > On 3/11/21 5:29 PM, Laurent Pinchart wrote:\n> > > > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:\n> > > > > On 3/11/21 3:03 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> > > > > > Instead of taking care of both, the request and the wrap-object, we can\n> > > > > > move the request to the wrap which will then effectively take care of\n> > > > > > the request object automatically.\n> > > > >\n> > > > > ...take care of the request s/object/object-deletion/ maybe?\n> > > > > \n> > > > > > Signed-off-by: Marian Cichy \n> > > > > > Suggested-by: Laurent Pinchart \n> > > > > > ---\n> > > > > >    src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------\n> > > > > >    1 file changed, 12 insertions(+), 16 deletions(-)\n> > > > > > \n> > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > index 636c14df..e86c3d7f 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> > > > > > -       RequestWrap(Request *request);\n> > > > > > +       RequestWrap(std::unique_ptr request);\n> > > > > >         ~RequestWrap();\n> > > > > >    \n> > > > > >         void attachBuffer(GstBuffer *buffer);\n> > > > > >         GstBuffer *detachBuffer(Stream *stream);\n> > > > > >    \n> > > > > >         /* For ptr comparison only. */\n> > > > > \n> > > > > I think you need to remove this comment too\n> > > > \n> > > > Indeed :-)\n> > > > \n> > > > > > -       Request *request_;\n> > > > > > +       std::unique_ptr request_;\n> > > > > >         std::map buffers_;\n> > > > > >    };\n> > > > > >    \n> > > > > > -RequestWrap::RequestWrap(Request *request)\n> > > > > > -       : request_(request)\n> > > > > > +RequestWrap::RequestWrap(std::unique_ptr request)\n> > > > > > +       : request_(std::move(request))\n> > > > > >    {\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> > > > > >    \n> > > > > >    void RequestWrap::attachBuffer(GstBuffer *buffer)\n> > > > > > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> > > > > >         std::unique_ptr wrap = std::move(requests_.front());\n> > > > > >         requests_.pop();\n> > > > > >    \n> > > > > > -       g_return_if_fail(wrap->request_ == request);\n> > > > > > +       g_return_if_fail(wrap->request_.get() == request);\n> > > > > >    \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> > > > > >    \n> > > > > > -       std::unique_ptr request = state->cam_->createRequest();\n> > > > > > -       auto wrap = std::make_unique(request.get());\n> > > > > > +       auto wrap = std::make_unique(state->cam_->createRequest());\n> > > > \n> > > > > *1 for marker below\n> > > > > \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> > > > > \n> > > > > s/rquest/request/\n> > > > > \n> > > > > > +                        * won't be queueing this one due to lack of buffers.\n> > > > > >                          */\n> > > > > > -                       request.reset();\n> > > > > > +                       wrap.release();\n> > > > > >                         break;\n> > > > > >                 }\n> > > > > >    \n> > > > > >                 wrap->attachBuffer(buffer);\n> > > > > >         }\n> > > > > >    \n> > > > > > -       if (request) {\n> > > > > > +       if (wrap) {\n> > > > > \n> > > > > I think this should be `if (wrap.get())` no? `wrap.release()` if hit,\n> > > > > will set nullptr to the _managed_ object, so we need to check the\n> > > > > managed object for null\n> > > > \n> > > > This is what is happening already:\n> > > > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool\n> > > > \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> > > > > from *1, createRequest() call can return nullptr, hence you have a\n> > > > > \n> > > > > std::make_unique(nullptr);\n> > > > > \n> > > > > which will set request_ as nullptr on the wrap object. It  is fine as\n> > > > > long as wrap->request_ is not used unlike here, in this block.\n> > > > \n> > > > That's indeed a problem. Maybe\n> > > > \n> > > >         if (wrap && wrap->request_)\n> > > > \n> > > > above ? Nicolas, any opinion ?\n> > > \n> > > OR null-check createRequest()'s returned object before wrapping it (i.e. \n> > > ensure RequestWrap doesn't wrap a nullptr from the start), so we don't \n> > > have to check everything for the wrap->request_ is valid or not.\n> > \n> > But, as far as I understand, code further down this function still have\n> > to run in that case, we can't just do an early return using a null\n> > check. That's why I've asked for Nicolas' feedback, he knows this code\n> > much better than I do.\n> \n> Hmm, right. Something like this should do:\n> \n> if (!request) {\n> \tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...;\n> gst_task_stop(task);\n> \treturn;\n> }\n\nSomething along those lines ?\n\ncommit be081f85bbab0c8b3b5f08d73c5bda504be7f2e6\nAuthor: Marian Cichy \nDate: Thu Mar 11 10:33:25 2021 +0100\n\n libcamera: gst: Fix double-free when acquire_buffer fails\n \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 Instead of taking care of both, the request and the wrap-object, we can\n move the request to the wrap which will then effectively take care of\n the request object automatically.\n \n Signed-off-by: Marian Cichy \n Suggested-by: Laurent Pinchart \n Signed-off-by: Laurent Pinchart \n\ndiff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\nindex 636c14dff64e..7b9671200f87 100644\n--- a/src/gstreamer/gstlibcamerasrc.cpp\n+++ b/src/gstreamer/gstlibcamerasrc.cpp\n@@ -52,19 +52,18 @@ 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 +73,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 +161,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@@ -269,7 +266,18 @@ gst_libcamera_src_task_run(gpointer 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+\tif (!request) {\n+\t\tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n+\t\t\t\t (\"Failed to allocate request for camera '%s'.\",\n+\t\t\t\t state->cam_->id().c_str()),\n+\t\t\t\t (\"libcamera::Camera::createRequest() failed\"));\n+\t\tgst_task_stop(self->task);\n+\t\treturn;\n+\t}\n+\n+\tstd::unique_ptr wrap =\n+\t\tstd::make_unique(std::move(request));\n+\n \tfor (GstPad *srcpad : state->srcpads_) {\n \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n \t\tGstBuffer *buffer;\n@@ -279,24 +287,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;\n\nMarian, would you be able to test this ?\n\nShould the same be done in case wrap becomes null, due to a call to\ngst_buffer_pool_acquire_buffer() failing, insted of continuing to\nprocessing the flow ? The lack of symmetry between the two codes paths\nbothers me, either because there's an issue there, or because I don't\nunderstand why they need to be different :-) This can be addressed in a\nsubsequent patch, as it's not an issue introduced by this patch.\n\n> > > > > >                 state->requests_.push(std::move(wrap));\n> > > > > >    \n> > > > > > -               /* The request will be deleted in the completion handler. */\n> > > > > > -               request.release();\n> > > > > > +               /* The RequestWrap will be deleted in the completion handler. */\n> > > > > >         }\n> > > > > >    \n> > > > > >         GstFlowReturn 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 5845DBD80C\n\tfor ;\n\tThu, 11 Mar 2021 23:15:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93DC268C6B;\n\tFri, 12 Mar 2021 00:15:36 +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 D430260106\n\tfor ;\n\tFri, 12 Mar 2021 00:15:34 +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 102DE88F;\n\tFri, 12 Mar 2021 00:15:33 +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=\"F+q41tvL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615504533;\n\tbh=UWI0AZPl9WxERbANc5tofeBJm4oqrN2LlvPZoF6ZfKE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F+q41tvLbR51lcGgak830IJr5jOTK+U++V9AKmtdyaWdQRWWAftN8TJHqtaFvY7Tm\n\tIMjWLoc/3LHpT/iWeKlzmyo2O9z2wuAS6o8eUDfiqcwoJFvF8zgHJN0G7s456MhEkw\n\t6O8AZCpxKMQN8IEqviKlFnmo7VLFZ7tiygSjtedc=","Date":"Fri, 12 Mar 2021 01:14:58 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Message-ID":"","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>\n\t<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>\n\t\n\t<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>\n\t\n\t<019d3ff94f759aaa56832e6241d732527ee39e1c.camel@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<019d3ff94f759aaa56832e6241d732527ee39e1c.camel@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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,\n\tMarian Cichy ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15627,"web_url":"https://patchwork.libcamera.org/comment/15627/","msgid":"<6903edf223efdc41389553bec906deb6416d6566.camel@collabora.com>","date":"2021-03-12T15:19:44","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_buffer fails","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le vendredi 12 mars 2021 à 01:14 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> On Thu, Mar 11, 2021 at 04:02:20PM -0500, Nicolas Dufresne wrote:\n> > Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit :\n> > > On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote:\n> > > > On 3/11/21 5:29 PM, Laurent Pinchart wrote:\n> > > > > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:\n> > > > > > On 3/11/21 3:03 PM, Marian Cichy wrote:\n> > > > > > > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails,\n> > > > > > > the\n> > > > > > > unique_ptr to the request-object gets reset and hence, its\n> > > > > > > destructor\n> > > > > > > is called. However, the wrap-object points to the same object and\n> > > > > > > is\n> > > > > > > still alive at this moment. When the task_run-function is\n> > > > > > > finished, the\n> > > > > > > destructor of the wrap-object is called, which in return calls the\n> > > > > > > destructor of the request-object again.\n> > > > > > > \n> > > > > > > Instead of taking care of both, the request and the wrap-object,\n> > > > > > > we can\n> > > > > > > move the request to the wrap which will then effectively take care\n> > > > > > > of\n> > > > > > > the request object automatically.\n> > > > > > \n> > > > > > ...take care of the request s/object/object-deletion/ maybe?\n> > > > > > \n> > > > > > > Signed-off-by: Marian Cichy \n> > > > > > > Suggested-by: Laurent Pinchart \n> > > > > > > ---\n> > > > > > >    src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++------------\n> > > > > > > ----\n> > > > > > >    1 file changed, 12 insertions(+), 16 deletions(-)\n> > > > > > > \n> > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > > index 636c14df..e86c3d7f 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> > > > > > > -       RequestWrap(Request *request);\n> > > > > > > +       RequestWrap(std::unique_ptr request);\n> > > > > > >         ~RequestWrap();\n> > > > > > >    \n> > > > > > >         void attachBuffer(GstBuffer *buffer);\n> > > > > > >         GstBuffer *detachBuffer(Stream *stream);\n> > > > > > >    \n> > > > > > >         /* For ptr comparison only. */\n> > > > > > \n> > > > > > I think you need to remove this comment too\n> > > > > \n> > > > > Indeed :-)\n> > > > > \n> > > > > > > -       Request *request_;\n> > > > > > > +       std::unique_ptr request_;\n> > > > > > >         std::map buffers_;\n> > > > > > >    };\n> > > > > > >    \n> > > > > > > -RequestWrap::RequestWrap(Request *request)\n> > > > > > > -       : request_(request)\n> > > > > > > +RequestWrap::RequestWrap(std::unique_ptr request)\n> > > > > > > +       : request_(std::move(request))\n> > > > > > >    {\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> > > > > > >    \n> > > > > > >    void RequestWrap::attachBuffer(GstBuffer *buffer)\n> > > > > > > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request\n> > > > > > > *request)\n> > > > > > >         std::unique_ptr wrap =\n> > > > > > > std::move(requests_.front());\n> > > > > > >         requests_.pop();\n> > > > > > >    \n> > > > > > > -       g_return_if_fail(wrap->request_ == request);\n> > > > > > > +       g_return_if_fail(wrap->request_.get() == request);\n> > > > > > >    \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> > > > > > >    \n> > > > > > > -       std::unique_ptr request = state->cam_-\n> > > > > > > >createRequest();\n> > > > > > > -       auto wrap = std::make_unique(request.get());\n> > > > > > > +       auto wrap = std::make_unique(state->cam_-\n> > > > > > > >createRequest());\n> > > > > \n> > > > > > *1 for marker below\n> > > > > > \n> > > > > > >         for (GstPad *srcpad : state->srcpads_) {\n> > > > > > >                 GstLibcameraPool *pool =\n> > > > > > > gst_libcamera_pad_get_pool(srcpad);\n> > > > > > >                 GstBuffer *buffer;\n> > > > > > > @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer\n> > > > > > > user_data)\n> > > > > > >                                                      &buffer,\n> > > > > > > nullptr);\n> > > > > > >                 if (ret != GST_FLOW_OK) {\n> > > > > > >                         /*\n> > > > > > > -                        * RequestWrap does not take ownership,\n> > > > > > > and we won't be\n> > > > > > > -                        * queueing this one due to lack of\n> > > > > > > buffers.\n> > > > > > > +                        * RequestWrap has ownership of the\n> > > > > > > rquest, and we\n> > > > > > \n> > > > > > s/rquest/request/\n> > > > > > \n> > > > > > > +                        * won't be queueing this one due to lack\n> > > > > > > of buffers.\n> > > > > > >                          */\n> > > > > > > -                       request.reset();\n> > > > > > > +                       wrap.release();\n> > > > > > >                         break;\n> > > > > > >                 }\n> > > > > > >    \n> > > > > > >                 wrap->attachBuffer(buffer);\n> > > > > > >         }\n> > > > > > >    \n> > > > > > > -       if (request) {\n> > > > > > > +       if (wrap) {\n> > > > > > \n> > > > > > I think this should be `if (wrap.get())` no? `wrap.release()` if\n> > > > > > hit,\n> > > > > > will set nullptr to the _managed_ object, so we need to check the\n> > > > > > managed object for null\n> > > > > \n> > > > > This is what is happening already:\n> > > > > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool\n> > > > > \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> > > > > > from *1, createRequest() call can return nullptr, hence you have a\n> > > > > > \n> > > > > > std::make_unique(nullptr);\n> > > > > > \n> > > > > > which will set request_ as nullptr on the wrap object. It  is fine\n> > > > > > as\n> > > > > > long as wrap->request_ is not used unlike here, in this block.\n> > > > > \n> > > > > That's indeed a problem. Maybe\n> > > > > \n> > > > >         if (wrap && wrap->request_)\n> > > > > \n> > > > > above ? Nicolas, any opinion ?\n> > > > \n> > > > OR null-check createRequest()'s returned object before wrapping it (i.e.\n> > > > ensure RequestWrap doesn't wrap a nullptr from the start), so we don't \n> > > > have to check everything for the wrap->request_ is valid or not.\n> > > \n> > > But, as far as I understand, code further down this function still have\n> > > to run in that case, we can't just do an early return using a null\n> > > check. That's why I've asked for Nicolas' feedback, he knows this code\n> > > much better than I do.\n> > \n> > Hmm, right. Something like this should do:\n> > \n> > if (!request) {\n> >         GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...;\n> >         gst_task_stop(task);\n> >         return;\n> > }\n> \n> Something along those lines ?\n> \n> commit be081f85bbab0c8b3b5f08d73c5bda504be7f2e6\n> Author: Marian Cichy \n> Date:   Thu Mar 11 10:33:25 2021 +0100\n> \n>     libcamera: gst: Fix double-free when acquire_buffer fails\n>     \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>     Instead of taking care of both, the request and the wrap-object, we can\n>     move the request to the wrap which will then effectively take care of\n>     the request object automatically.\n>     \n>     Signed-off-by: Marian Cichy \n>     Suggested-by: Laurent Pinchart \n>     Signed-off-by: Laurent Pinchart \n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> b/src/gstreamer/gstlibcamerasrc.cpp\n> index 636c14dff64e..7b9671200f87 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -52,19 +52,18 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n>  #define GST_CAT_DEFAULT source_debug\n>  \n>  struct RequestWrap {\n> -       RequestWrap(Request *request);\n> +       RequestWrap(std::unique_ptr request);\n>         ~RequestWrap();\n>  \n>         void attachBuffer(GstBuffer *buffer);\n>         GstBuffer *detachBuffer(Stream *stream);\n>  \n> -       /* For ptr comparison only. */\n> -       Request *request_;\n> +       std::unique_ptr request_;\n>         std::map buffers_;\n>  };\n>  \n> -RequestWrap::RequestWrap(Request *request)\n> -       : request_(request)\n> +RequestWrap::RequestWrap(std::unique_ptr request)\n> +       : request_(std::move(request))\n>  {\n>  }\n>  \n> @@ -74,8 +73,6 @@ RequestWrap::~RequestWrap()\n>                 if (item.second)\n>                         gst_buffer_unref(item.second);\n>         }\n> -\n> -       delete request_;\n>  }\n>  \n>  void RequestWrap::attachBuffer(GstBuffer *buffer)\n> @@ -164,7 +161,7 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n>         std::unique_ptr wrap = std::move(requests_.front());\n>         requests_.pop();\n>  \n> -       g_return_if_fail(wrap->request_ == request);\n> +       g_return_if_fail(wrap->request_.get() == request);\n>  \n>         if ((request->status() == Request::RequestCancelled)) {\n>                 GST_DEBUG_OBJECT(src_, \"Request was cancelled\");\n> @@ -269,7 +266,18 @@ gst_libcamera_src_task_run(gpointer user_data)\n>         GstLibcameraSrcState *state = self->state;\n>  \n>         std::unique_ptr request = state->cam_->createRequest();\n> -       auto wrap = std::make_unique(request.get());\n> +       if (!request) {\n> +               GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n> +                                 (\"Failed to allocate request for camera\n> '%s'.\",\n> +                                  state->cam_->id().c_str()),\n> +                                 (\"libcamera::Camera::createRequest()\n> failed\"));\n> +               gst_task_stop(self->task);\n> +               return;\n> +       }\n> +\n> +       std::unique_ptr wrap =\n> +               std::make_unique(std::move(request));\n> +\n>         for (GstPad *srcpad : state->srcpads_) {\n>                 GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n>                 GstBuffer *buffer;\n> @@ -279,24 +287,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\n> be\n> -                        * queueing this one due to lack of buffers.\n> +                        * RequestWrap has ownership of the rquest, and we\n request\n> +                        * won't be queueing this one due to lack of buffers.\n>                          */\n> -                       request.reset();\n> +                       wrap.release();\n>                         break;\n>                 }\n>  \n>                 wrap->attachBuffer(buffer);\n>         }\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>                 state->requests_.push(std::move(wrap));\n>  \n> -               /* The request will be deleted in the completion handler. */\n> -               request.release();\n> +               /* The RequestWrap will be deleted in the completion handler.\n> */\n>         }\n>  \n>         GstFlowReturn ret = GST_FLOW_OK;\n\nReviewed-by: Nicolas Dufresne \n\n> \n> Marian, would you be able to test this ?\n> \n> Should the same be done in case wrap becomes null, due to a call to\n> gst_buffer_pool_acquire_buffer() failing, insted of continuing to\n> processing the flow ? The lack of symmetry between the two codes paths\n> bothers me, either because there's an issue there, or because I don't\n> understand why they need to be different :-) This can be addressed in a\n> subsequent patch, as it's not an issue introduced by this patch.\n\nI think this code is a bit incomplete, specially since it supports multiple\npads, while you cannot yet request a second pad. In general, my idea was that\nwe'd try and ensure we have at least one pending request per stream/pad before\npausing the task. Pausing the task, which just mean our task function will stop\nbeing called repeatedly until gst_libcamera_resume_task() is called.\n\nFor the case we run-out of buffer in our pool (which is not as dramatic as\nfailing to create a request), we should only stop / fail if there is no pending\nrequest already queued for that pad. I'll take care of this one, it also need a\nlocking fix, and reversing the pending condition check lower (which is upside\ndown).\n\n> \n> > > > > > >                 state->requests_.push(std::move(wrap));\n> > > > > > >    \n> > > > > > > -               /* The request will be deleted in the completion\n> > > > > > > handler. */\n> > > > > > > -               request.release();\n> > > > > > > +               /* The RequestWrap will be deleted in the\n> > > > > > > completion handler. */\n> > > > > > >         }\n> > > > > > >    \n> > > > > > >         GstFlowReturn ret = GST_FLOW_OK;\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 A6A0EBD80C\n\tfor ;\n\tFri, 12 Mar 2021 15:19:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A06568C72;\n\tFri, 12 Mar 2021 16:19:57 +0100 (CET)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 937B168C68\n\tfor ;\n\tFri, 12 Mar 2021 16:19:55 +0100 (CET)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nicolas) with ESMTPSA id 92EF01F46E45"],"Message-ID":"<6903edf223efdc41389553bec906deb6416d6566.camel@collabora.com>","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Date":"Fri, 12 Mar 2021 10:19:44 -0500","In-Reply-To":"","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>\n\t<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>\n\t\n\t<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>\n\t\n\t<019d3ff94f759aaa56832e6241d732527ee39e1c.camel@collabora.com>\n\t","User-Agent":"Evolution 3.38.3 (3.38.3-1.fc33) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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,\n\tMarian Cichy ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15630,"web_url":"https://patchwork.libcamera.org/comment/15630/","msgid":"<935c9bb8-059f-f129-c83e-49b7ed93ef66@pengutronix.de>","date":"2021-03-12T17:59:11","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_buffer fails","submitter":{"id":81,"url":"https://patchwork.libcamera.org/api/people/81/","name":"Marian Cichy","email":"mci@pengutronix.de"},"content":"On 3/12/21 12:14 AM, Laurent Pinchart wrote:\n> Hi Nicolas,\n>\n> On Thu, Mar 11, 2021 at 04:02:20PM -0500, Nicolas Dufresne wrote:\n>> Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit :\n>>> On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote:\n>>>> On 3/11/21 5:29 PM, Laurent Pinchart wrote:\n>>>>> On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:\n>>>>>> On 3/11/21 3:03 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>>>>>>> Instead of taking care of both, the request and the wrap-object, we can\n>>>>>>> move the request to the wrap which will then effectively take care of\n>>>>>>> the request object automatically.\n>>>>>> ...take care of the request s/object/object-deletion/ maybe?\n>>>>>>\n>>>>>>> Signed-off-by: Marian Cichy \n>>>>>>> Suggested-by: Laurent Pinchart \n>>>>>>> ---\n>>>>>>>    src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------\n>>>>>>>    1 file changed, 12 insertions(+), 16 deletions(-)\n>>>>>>>\n>>>>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n>>>>>>> b/src/gstreamer/gstlibcamerasrc.cpp\n>>>>>>> index 636c14df..e86c3d7f 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>>>>>>> -       RequestWrap(Request *request);\n>>>>>>> +       RequestWrap(std::unique_ptr request);\n>>>>>>>         ~RequestWrap();\n>>>>>>> \n>>>>>>>         void attachBuffer(GstBuffer *buffer);\n>>>>>>>         GstBuffer *detachBuffer(Stream *stream);\n>>>>>>> \n>>>>>>>         /* For ptr comparison only. */\n>>>>>> I think you need to remove this comment too\n>>>>> Indeed :-)\n>>>>>\n>>>>>>> -       Request *request_;\n>>>>>>> +       std::unique_ptr request_;\n>>>>>>>         std::map buffers_;\n>>>>>>>    };\n>>>>>>> \n>>>>>>> -RequestWrap::RequestWrap(Request *request)\n>>>>>>> -       : request_(request)\n>>>>>>> +RequestWrap::RequestWrap(std::unique_ptr request)\n>>>>>>> +       : request_(std::move(request))\n>>>>>>>    {\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>>>>>>> \n>>>>>>>    void RequestWrap::attachBuffer(GstBuffer *buffer)\n>>>>>>> @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n>>>>>>>         std::unique_ptr wrap = std::move(requests_.front());\n>>>>>>>         requests_.pop();\n>>>>>>> \n>>>>>>> -       g_return_if_fail(wrap->request_ == request);\n>>>>>>> +       g_return_if_fail(wrap->request_.get() == request);\n>>>>>>> \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>>>>>>> \n>>>>>>> -       std::unique_ptr request = state->cam_->createRequest();\n>>>>>>> -       auto wrap = std::make_unique(request.get());\n>>>>>>> +       auto wrap = std::make_unique(state->cam_->createRequest());\n>>>>>> *1 for marker below\n>>>>>>\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>>>>>> s/rquest/request/\n>>>>>>\n>>>>>>> +                        * won't be queueing this one due to lack of buffers.\n>>>>>>>                          */\n>>>>>>> -                       request.reset();\n>>>>>>> +                       wrap.release();\n>>>>>>>                         break;\n>>>>>>>                 }\n>>>>>>> \n>>>>>>>                 wrap->attachBuffer(buffer);\n>>>>>>>         }\n>>>>>>> \n>>>>>>> -       if (request) {\n>>>>>>> +       if (wrap) {\n>>>>>> I think this should be `if (wrap.get())` no? `wrap.release()` if hit,\n>>>>>> will set nullptr to the _managed_ object, so we need to check the\n>>>>>> managed object for null\n>>>>> This is what is happening already:\n>>>>> https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool\n>>>>>\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>>>>>> from *1, createRequest() call can return nullptr, hence you have a\n>>>>>>\n>>>>>> std::make_unique(nullptr);\n>>>>>>\n>>>>>> which will set request_ as nullptr on the wrap object. It  is fine as\n>>>>>> long as wrap->request_ is not used unlike here, in this block.\n>>>>> That's indeed a problem. Maybe\n>>>>>\n>>>>>         if (wrap && wrap->request_)\n>>>>>\n>>>>> above ? Nicolas, any opinion ?\n>>>> OR null-check createRequest()'s returned object before wrapping it (i.e.\n>>>> ensure RequestWrap doesn't wrap a nullptr from the start), so we don't\n>>>> have to check everything for the wrap->request_ is valid or not.\n>>> But, as far as I understand, code further down this function still have\n>>> to run in that case, we can't just do an early return using a null\n>>> check. That's why I've asked for Nicolas' feedback, he knows this code\n>>> much better than I do.\n>> Hmm, right. Something like this should do:\n>>\n>> if (!request) {\n>> \tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...;\n>> gst_task_stop(task);\n>> \treturn;\n>> }\n> Something along those lines ?\n>\n> commit be081f85bbab0c8b3b5f08d73c5bda504be7f2e6\n> Author: Marian Cichy \n> Date: Thu Mar 11 10:33:25 2021 +0100\n>\n> libcamera: gst: Fix double-free when acquire_buffer fails\n> \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> Instead of taking care of both, the request and the wrap-object, we can\n> move the request to the wrap which will then effectively take care of\n> the request object automatically.\n> \n> Signed-off-by: Marian Cichy \n> Suggested-by: Laurent Pinchart \n> Signed-off-by: Laurent Pinchart \n>\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 636c14dff64e..7b9671200f87 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -52,19 +52,18 @@ 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 +73,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 +161,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> @@ -269,7 +266,18 @@ gst_libcamera_src_task_run(gpointer 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> +\tif (!request) {\n> +\t\tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n> +\t\t\t\t (\"Failed to allocate request for camera '%s'.\",\n> +\t\t\t\t state->cam_->id().c_str()),\n> +\t\t\t\t (\"libcamera::Camera::createRequest() failed\"));\n> +\t\tgst_task_stop(self->task);\n> +\t\treturn;\n> +\t}\n> +\n> +\tstd::unique_ptr wrap =\n> +\t\tstd::make_unique(std::move(request));\n> +\n> \tfor (GstPad *srcpad : state->srcpads_) {\n> \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n> \t\tGstBuffer *buffer;\n> @@ -279,24 +287,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;\n>\n> Marian, would you be able to test this ?\n>\n> Should the same be done in case wrap becomes null, due to a call to\n> gst_buffer_pool_acquire_buffer() failing, insted of continuing to\n> processing the flow ? The lack of symmetry between the two codes paths\n> bothers me, either because there's an issue there, or because I don't\n> understand why they need to be different :-) This can be addressed in a\n> subsequent patch, as it's not an issue introduced by this patch.\n\n\nHi Laurent,\n\nI tested this, using:\n\n\n@@ -267,6 +265,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n  {\n         GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n         GstLibcameraSrcState *state = self->state;\n+       static int cnt = 0;\n\n         std::unique_ptr request = state->cam_->createRequest();\n         auto wrap = std::make_unique(request.get());\n@@ -275,7 +274,10 @@ gst_libcamera_src_task_run(gpointer user_data)\n                 GstBuffer *buffer;\n                 GstFlowReturn ret;\n\n-               ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),\n+               if (++cnt == 10)\n+                       ret = GST_FLOW_ERROR;\n+               else\n+                       ret = \ngst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),\n\n\nas expected, the code skips the failing buffer, than continues it works, \nno problems whatsoever. So it's fine.\n\nI see there is still ongoing discussion with you and Nicolas, so I'd say \nI wait before sending V3 until it's clear?\n\nMarian\n\n\n>>>>>>>                 state->requests_.push(std::move(wrap));\n>>>>>>> \n>>>>>>> -               /* The request will be deleted in the completion handler. */\n>>>>>>> -               request.release();\n>>>>>>> +               /* The RequestWrap will be deleted in the completion handler. */\n>>>>>>>         }\n>>>>>>> \n>>>>>>>         GstFlowReturn 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 86C54BD1F1\n\tfor ;\n\tFri, 12 Mar 2021 17:59:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E7DAE68C6B;\n\tFri, 12 Mar 2021 18:59:15 +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 254EA68C66\n\tfor ;\n\tFri, 12 Mar 2021 18:59:14 +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 1lKm49-0008E6-GX; Fri, 12 Mar 2021 18:59:13 +0100"],"To":"Laurent Pinchart ,\n\tNicolas Dufresne ","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>\n\t<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>\n\t\n\t<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>\n\t\n\t<019d3ff94f759aaa56832e6241d732527ee39e1c.camel@collabora.com>\n\t","From":"Marian Cichy ","Message-ID":"<935c9bb8-059f-f129-c83e-49b7ed93ef66@pengutronix.de>","Date":"Fri, 12 Mar 2021 18:59:11 +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 v2] libcamera: gst: Fix double-free\n\twhen acquire_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":15632,"web_url":"https://patchwork.libcamera.org/comment/15632/","msgid":"","date":"2021-03-12T20:56:57","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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 Fri, Mar 12, 2021 at 06:59:11PM +0100, Marian Cichy wrote:\n> On 3/12/21 12:14 AM, Laurent Pinchart wrote:\n> > On Thu, Mar 11, 2021 at 04:02:20PM -0500, Nicolas Dufresne wrote:\n> >> Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit :\n> >>> On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote:\n> >>>> On 3/11/21 5:29 PM, Laurent Pinchart wrote:\n> >>>>> On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:\n> >>>>>> On 3/11/21 3:03 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> >>>>>>> Instead of taking care of both, the request and the wrap-object, we can\n> >>>>>>> move the request to the wrap which will then effectively take care of\n> >>>>>>> the request object automatically.\n> >>>>>> ...take care of the request s/object/object-deletion/ maybe?\n> >>>>>>\n> >>>>>>> Signed-off-by: Marian Cichy \n> >>>>>>> Suggested-by: Laurent Pinchart \n> >>>>>>> ---\n> >>>>>>>    src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------\n> >>>>>>>    1 file changed, 12 insertions(+), 16 deletions(-)\n> >>>>>>>\n> >>>>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> >>>>>>> b/src/gstreamer/gstlibcamerasrc.cpp\n> >>>>>>> index 636c14df..e86c3d7f 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> >>>>>>> -       RequestWrap(Request *request);\n> >>>>>>> +       RequestWrap(std::unique_ptr request);\n> >>>>>>>         ~RequestWrap();\n> >>>>>>> \n> >>>>>>>         void attachBuffer(GstBuffer *buffer);\n> >>>>>>>         GstBuffer *detachBuffer(Stream *stream);\n> >>>>>>> \n> >>>>>>>         /* For ptr comparison only. */\n> >>>>>> I think you need to remove this comment too\n> >>>>> Indeed :-)\n> >>>>>\n> >>>>>>> -       Request *request_;\n> >>>>>>> +       std::unique_ptr request_;\n> >>>>>>>         std::map buffers_;\n> >>>>>>>    };\n> >>>>>>> \n> >>>>>>> -RequestWrap::RequestWrap(Request *request)\n> >>>>>>> -       : request_(request)\n> >>>>>>> +RequestWrap::RequestWrap(std::unique_ptr request)\n> >>>>>>> +       : request_(std::move(request))\n> >>>>>>>    {\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> >>>>>>> \n> >>>>>>>    void RequestWrap::attachBuffer(GstBuffer *buffer)\n> >>>>>>> @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> >>>>>>>         std::unique_ptr wrap = std::move(requests_.front());\n> >>>>>>>         requests_.pop();\n> >>>>>>> \n> >>>>>>> -       g_return_if_fail(wrap->request_ == request);\n> >>>>>>> +       g_return_if_fail(wrap->request_.get() == request);\n> >>>>>>> \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> >>>>>>> \n> >>>>>>> -       std::unique_ptr request = state->cam_->createRequest();\n> >>>>>>> -       auto wrap = std::make_unique(request.get());\n> >>>>>>> +       auto wrap = std::make_unique(state->cam_->createRequest());\n> >>>>>> *1 for marker below\n> >>>>>>\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> >>>>>> s/rquest/request/\n> >>>>>>\n> >>>>>>> +                        * won't be queueing this one due to lack of buffers.\n> >>>>>>>                          */\n> >>>>>>> -                       request.reset();\n> >>>>>>> +                       wrap.release();\n> >>>>>>>                         break;\n> >>>>>>>                 }\n> >>>>>>> \n> >>>>>>>                 wrap->attachBuffer(buffer);\n> >>>>>>>         }\n> >>>>>>> \n> >>>>>>> -       if (request) {\n> >>>>>>> +       if (wrap) {\n> >>>>>> I think this should be `if (wrap.get())` no? `wrap.release()` if hit,\n> >>>>>> will set nullptr to the _managed_ object, so we need to check the\n> >>>>>> managed object for null\n> >>>>> This is what is happening already:\n> >>>>> https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool\n> >>>>>\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> >>>>>> from *1, createRequest() call can return nullptr, hence you have a\n> >>>>>>\n> >>>>>> std::make_unique(nullptr);\n> >>>>>>\n> >>>>>> which will set request_ as nullptr on the wrap object. It  is fine as\n> >>>>>> long as wrap->request_ is not used unlike here, in this block.\n> >>>>> That's indeed a problem. Maybe\n> >>>>>\n> >>>>>         if (wrap && wrap->request_)\n> >>>>>\n> >>>>> above ? Nicolas, any opinion ?\n> >>>> OR null-check createRequest()'s returned object before wrapping it (i.e.\n> >>>> ensure RequestWrap doesn't wrap a nullptr from the start), so we don't\n> >>>> have to check everything for the wrap->request_ is valid or not.\n> >>> But, as far as I understand, code further down this function still have\n> >>> to run in that case, we can't just do an early return using a null\n> >>> check. That's why I've asked for Nicolas' feedback, he knows this code\n> >>> much better than I do.\n> >> Hmm, right. Something like this should do:\n> >>\n> >> if (!request) {\n> >> \tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...;\n> >> gst_task_stop(task);\n> >> \treturn;\n> >> }\n> > Something along those lines ?\n> >\n> > commit be081f85bbab0c8b3b5f08d73c5bda504be7f2e6\n> > Author: Marian Cichy \n> > Date: Thu Mar 11 10:33:25 2021 +0100\n> >\n> > libcamera: gst: Fix double-free when acquire_buffer fails\n> > \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> > Instead of taking care of both, the request and the wrap-object, we can\n> > move the request to the wrap which will then effectively take care of\n> > the request object automatically.\n> > \n> > Signed-off-by: Marian Cichy \n> > Suggested-by: Laurent Pinchart \n> > Signed-off-by: Laurent Pinchart \n> >\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 636c14dff64e..7b9671200f87 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -52,19 +52,18 @@ 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 +73,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 +161,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> > @@ -269,7 +266,18 @@ gst_libcamera_src_task_run(gpointer 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> > +\tif (!request) {\n> > +\t\tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n> > +\t\t\t\t (\"Failed to allocate request for camera '%s'.\",\n> > +\t\t\t\t state->cam_->id().c_str()),\n> > +\t\t\t\t (\"libcamera::Camera::createRequest() failed\"));\n> > +\t\tgst_task_stop(self->task);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tstd::unique_ptr wrap =\n> > +\t\tstd::make_unique(std::move(request));\n> > +\n> > \tfor (GstPad *srcpad : state->srcpads_) {\n> > \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n> > \t\tGstBuffer *buffer;\n> > @@ -279,24 +287,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;\n> >\n> > Marian, would you be able to test this ?\n> >\n> > Should the same be done in case wrap becomes null, due to a call to\n> > gst_buffer_pool_acquire_buffer() failing, insted of continuing to\n> > processing the flow ? The lack of symmetry between the two codes paths\n> > bothers me, either because there's an issue there, or because I don't\n> > understand why they need to be different :-) This can be addressed in a\n> > subsequent patch, as it's not an issue introduced by this patch.\n> \n> \n> Hi Laurent,\n> \n> I tested this, using:\n> \n> \n> @@ -267,6 +265,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n>  {\n>         GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n>         GstLibcameraSrcState *state = self->state;\n> +       static int cnt = 0;\n> \n>         std::unique_ptr request = state->cam_->createRequest();\n>         auto wrap = std::make_unique(request.get());\n> @@ -275,7 +274,10 @@ gst_libcamera_src_task_run(gpointer user_data)\n>                 GstBuffer *buffer;\n>                 GstFlowReturn ret;\n> \n> -               ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),\n> +               if (++cnt == 10)\n> +                       ret = GST_FLOW_ERROR;\n> +               else\n> +                       ret = \n> gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),\n> \n> \n> as expected, the code skips the failing buffer, than continues it works, \n> no problems whatsoever. So it's fine.\n> \n> I see there is still ongoing discussion with you and Nicolas, so I'd say \n> I wait before sending V3 until it's clear?\n\nNicolas gave a Reviewed-by on this, so I think the other issues can be\naddressed on top. I'll push this.\n\n> >>>>>>>                 state->requests_.push(std::move(wrap));\n> >>>>>>> \n> >>>>>>> -               /* The request will be deleted in the completion handler. */\n> >>>>>>> -               request.release();\n> >>>>>>> +               /* The RequestWrap will be deleted in the completion handler. */\n> >>>>>>>         }\n> >>>>>>> \n> >>>>>>>         GstFlowReturn 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 644D2BD80C\n\tfor ;\n\tFri, 12 Mar 2021 20:57:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1DA8368C6A;\n\tFri, 12 Mar 2021 21:57:41 +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 7BD1668AA1\n\tfor ;\n\tFri, 12 Mar 2021 21:57:39 +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 9DA2388F;\n\tFri, 12 Mar 2021 21:57:37 +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=\"Grsv4XON\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615582659;\n\tbh=a6izPmwjVHASLYpoYyHGZHGQ3BXcwQ/UneTvKAOoLPQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Grsv4XONeP9pcogkPdgEWyXeAhX21i8gGiXErhwB16UrW0WqaWJUQVWQdghOOgxHy\n\thhcxXs6uUqJbpXZWxXgA3cA5z50kPTLowta+Cly0D6tP5v0pDZL5DHQmGQi/Qts6cp\n\tJSWK7wCd6EUYAtm5ltQaZEHes7McJXF9/lZX5DxU=","Date":"Fri, 12 Mar 2021 22:56:57 +0200","From":"Laurent Pinchart ","To":"Marian Cichy ","Message-ID":"","References":"<20210311093325.8933-1-m.cichy@pengutronix.de>\n\t<4378a6f3-ca34-cd3d-acc2-bd75d948d3d9@uajain.com>\n\t\n\t<41c9bdd6-bf67-3ca4-d99b-64e13be72e97@uajain.com>\n\t\n\t<019d3ff94f759aaa56832e6241d732527ee39e1c.camel@collabora.com>\n\t\n\t<935c9bb8-059f-f129-c83e-49b7ed93ef66@pengutronix.de>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<935c9bb8-059f-f129-c83e-49b7ed93ef66@pengutronix.de>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free\n\twhen acquire_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,\n\tNicolas Dufresne , \n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]