[{"id":15924,"web_url":"https://patchwork.libcamera.org/comment/15924/","msgid":"","date":"2021-03-26T02:51:41","subject":"Re: [libcamera-devel] [PATCH v3 05/11] libcamera: request: A\n\trequest canary","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Thu, Mar 25, 2021 at 01:42:25PM +0000, Kieran Bingham wrote:\n> Request objects are created and owned by the application, but are of\n> course utilised widely throughout the internals of libcamera.\n> \n> If the application free's the requests while they are still active\n> within libcamera a use after free will occur. While this can be trapped\n> by tools such as valgrind, given the importance of this object and the\n> relationship of external ownership, it may have some value to provide\n> Debug build (disabled at Release build) assertions on the condition of\n> these objects.\n> \n> Make sure the request fails an assertion immediately if used after free.\n> \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> I've removed the RFC from this, and I actually even more so believe this\n> is a good facility to provide on the Request object.\n> \n> Mostly because the Request object is the only object which is given to\n> libcamera from the application (yes, created by libcamera, but that's\n> separate), and is expected to be free'd by the application.\n> \n> If an application free's requests while they are still in use within\n> libcamera, the symptoms can be distinctly misleading and lead to rabbit\n> holes.\n> \n> Therefore, I think the Request object is the one place where extra\n> safety checking (in debug builds) is worth while.\n\nShould the canary_ member be compiled out in non-debug builds ?\n\n> Yes, of course if an application free's a request while it's in use with\n> libcamera - then it's the applications fault, not ours - but because of\n> the nature of requests, this could be an easy trap to fall into - and I\n> don't want to find that reported as bugs in libcamera.\n\nWhen Request will become Extensible, do you think the canary_ field\ncould be moved to the Private class ? That would make me feel better\nabout this feature :-)\n\nActually, maybe we could use the d_ pointer and o_ pointer to replace\nthe canary, but checking the point to each other ?\n\n> ---\n> include/libcamera/request.h | 2 ++\n> src/libcamera/request.cpp | 18 ++++++++++++++++++\n> 2 files changed, 20 insertions(+)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 4cf5ff3f7d3b..965ffa6b45b2 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -79,6 +79,8 @@ private:\n> \tconst uint64_t cookie_;\n> \tStatus status_;\n> \tbool cancelled_;\n> +\n> +\tuint32_t canary_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 7b7ef6814686..c4258480b12b 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -19,6 +19,8 @@\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/tracepoints.h\"\n> \n> +#define REQUEST_CANARY 0x1F2E3D4C\n> +\n> /**\n> * \\file request.h\n> * \\brief Describes a frame capture request to be processed by a camera\n> @@ -90,6 +92,8 @@ Request::Request(Camera *camera, uint64_t cookie)\n> \n> \tLIBCAMERA_TRACEPOINT(request_construct, this);\n> \n> +\tcanary_ = REQUEST_CANARY;\n\nYou can move this to the member initializer list.\n\n> +\n> \tLOG(Request, Debug) << \"Created request - cookie: \" << cookie_;\n> }\n> \n> @@ -100,6 +104,8 @@ Request::~Request()\n> \tdelete metadata_;\n> \tdelete controls_;\n> \tdelete validator_;\n> +\n> +\tcanary_ = 0;\n> }\n> \n> /**\n> @@ -114,6 +120,8 @@ Request::~Request()\n> */\n> void Request::reuse(ReuseFlag flags)\n> {\n> +\tASSERT(canary_ == REQUEST_CANARY);\n> +\n> \tLIBCAMERA_TRACEPOINT(request_reuse, this);\n> \n> \tpending_.clear();\n> @@ -179,6 +187,8 @@ void Request::reuse(ReuseFlag flags)\n> */\n> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> {\n> +\tASSERT(canary_ == REQUEST_CANARY);\n> +\n> \tif (!stream) {\n> \t\tLOG(Request, Error) << \"Invalid stream reference\";\n> \t\treturn -EINVAL;\n> @@ -214,6 +224,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> */\n> FrameBuffer *Request::findBuffer(const Stream *stream) const\n> {\n> +\tASSERT(canary_ == REQUEST_CANARY);\n> +\n> \tconst auto it = bufferMap_.find(stream);\n> \tif (it == bufferMap_.end())\n> \t\treturn nullptr;\n> @@ -281,6 +293,7 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> */\n> void Request::complete()\n> {\n> +\tASSERT(canary_ == REQUEST_CANARY);\n> \tASSERT(status_ == RequestPending);\n> \tASSERT(!hasPendingBuffers());\n> \n> @@ -306,6 +319,8 @@ void Request::complete()\n> */\n> bool Request::completeBuffer(FrameBuffer *buffer)\n> {\n> +\tASSERT(canary_ == REQUEST_CANARY);\n> +\n> \tLIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n> \n> \tint ret = pending_.erase(buffer);\n> @@ -326,6 +341,9 @@ std::string Request::toString() const\n> \t/* Pending, Completed, Cancelled(X). */\n> \tstatic const char *statuses = \"PCX\";\n> \n> +\tif (canary_ != REQUEST_CANARY)\n> +\t\treturn \"Invalid Canary on Request\";\n\nAs all bets are off in this case, should we ASSERT() to catch this\ncondition as early as possible ?\n\n> +\n> \t/* Example Output: Request(55:P:1/2:6523524) */\n> \tss << \"Request (\" << sequence_ << \":\" << statuses[status_] << \":\"\n> \t << pending_.size() << \"/\" << bufferMap_.size() << \":\"","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 C67D3C32EA\n\tfor ;\n\tFri, 26 Mar 2021 02:52:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F6AA68D66;\n\tFri, 26 Mar 2021 03:52:26 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03257602E3\n\tfor ;\n\tFri, 26 Mar 2021 03:52:25 +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 8C806443;\n\tFri, 26 Mar 2021 03:52:24 +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=\"FlVVoFoC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616727144;\n\tbh=JOlOoTHHuRzMtP7sXEAN3hYbeOCl6Dsngfidrm6ED0A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FlVVoFoCeueFXifyqhdfPmXho/pBN9jlQfxmoV1UCA+hy5Fmo6fLbSrRCSSZIvqDB\n\tQNHmDnzXdaqicapvs/yBBA52fGlW7fb9wgTFl8pu/2m1YIaKA03RF498YfBXsZCV9H\n\tLGh9O1u4qxfmfAHoGiHwQjn0MDNAY43XIWNGZYhY=","Date":"Fri, 26 Mar 2021 04:51:41 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>\n\t<20210325134231.1400051-8-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210325134231.1400051-8-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 05/11] libcamera: request: A\n\trequest canary","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15949,"web_url":"https://patchwork.libcamera.org/comment/15949/","msgid":"<0d480da3-560a-bb3a-09a7-40db65738784@ideasonboard.com>","date":"2021-03-26T15:11:35","subject":"Re: [libcamera-devel] [PATCH v3 05/11] libcamera: request: A\n\trequest canary","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 26/03/2021 02:51, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Thu, Mar 25, 2021 at 01:42:25PM +0000, Kieran Bingham wrote:\n>> Request objects are created and owned by the application, but are of\n>> course utilised widely throughout the internals of libcamera.\n>>\n>> If the application free's the requests while they are still active\n>> within libcamera a use after free will occur. While this can be trapped\n>> by tools such as valgrind, given the importance of this object and the\n>> relationship of external ownership, it may have some value to provide\n>> Debug build (disabled at Release build) assertions on the condition of\n>> these objects.\n>>\n>> Make sure the request fails an assertion immediately if used after free.\n>>\n>> Signed-off-by: Kieran Bingham \n>>\n>> ---\n>> I've removed the RFC from this, and I actually even more so believe this\n>> is a good facility to provide on the Request object.\n>>\n>> Mostly because the Request object is the only object which is given to\n>> libcamera from the application (yes, created by libcamera, but that's\n>> separate), and is expected to be free'd by the application.\n>>\n>> If an application free's requests while they are still in use within\n>> libcamera, the symptoms can be distinctly misleading and lead to rabbit\n>> holes.\n>>\n>> Therefore, I think the Request object is the one place where extra\n>> safety checking (in debug builds) is worth while.\n> \n> Should the canary_ member be compiled out in non-debug builds ?\n\nIt could be. It's small so I don't think it's an expensive waste of\nmemory - but I don't expect it to be used in Release builds.\n\nThis is just to satisfy the assertions.\n\n\n>> Yes, of course if an application free's a request while it's in use with\n>> libcamera - then it's the applications fault, not ours - but because of\n>> the nature of requests, this could be an easy trap to fall into - and I\n>> don't want to find that reported as bugs in libcamera.\n> \n> When Request will become Extensible, do you think the canary_ field\n> could be moved to the Private class ? That would make me feel better\n> about this feature :-)\n\nCertainly, I'd have no issue with that, as long as we guarantee that\nwhen the Request is released, so is the Private, but I think we can say\nthat's fine :-D\n\n> Actually, maybe we could use the d_ pointer and o_ pointer to replace\n> the canary, but checking the point to each other ?\n\nAh, I think I see what you mean. Yes, I think there is a possibility\nthere. As long as it doesn't then mean we try to access an invalid\nunique_ptr and crash before we can ASSERT() ;-)\n\nIt may be that this then gives us a flexible method to apply the same\nassertions on any public object (which is why we would use extensible in\nthe first place) and could add a layer of protection to other public\nobjects. Perhaps things like Buffers where the application might have\nownership, and could release them at the wrong time ...\n\nI'll investigate when I get back to getting Request Extensible.\n\n\n>> ---\n>> include/libcamera/request.h | 2 ++\n>> src/libcamera/request.cpp | 18 ++++++++++++++++++\n>> 2 files changed, 20 insertions(+)\n>>\n>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>> index 4cf5ff3f7d3b..965ffa6b45b2 100644\n>> --- a/include/libcamera/request.h\n>> +++ b/include/libcamera/request.h\n>> @@ -79,6 +79,8 @@ private:\n>> \tconst uint64_t cookie_;\n>> \tStatus status_;\n>> \tbool cancelled_;\n>> +\n>> +\tuint32_t canary_;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>> index 7b7ef6814686..c4258480b12b 100644\n>> --- a/src/libcamera/request.cpp\n>> +++ b/src/libcamera/request.cpp\n>> @@ -19,6 +19,8 @@\n>> #include \"libcamera/internal/log.h\"\n>> #include \"libcamera/internal/tracepoints.h\"\n>> \n>> +#define REQUEST_CANARY 0x1F2E3D4C\n>> +\n>> /**\n>> * \\file request.h\n>> * \\brief Describes a frame capture request to be processed by a camera\n>> @@ -90,6 +92,8 @@ Request::Request(Camera *camera, uint64_t cookie)\n>> \n>> \tLIBCAMERA_TRACEPOINT(request_construct, this);\n>> \n>> +\tcanary_ = REQUEST_CANARY;\n> \n> You can move this to the member initializer list.\n> \n>> +\n>> \tLOG(Request, Debug) << \"Created request - cookie: \" << cookie_;\n>> }\n>> \n>> @@ -100,6 +104,8 @@ Request::~Request()\n>> \tdelete metadata_;\n>> \tdelete controls_;\n>> \tdelete validator_;\n>> +\n>> +\tcanary_ = 0;\n>> }\n>> \n>> /**\n>> @@ -114,6 +120,8 @@ Request::~Request()\n>> */\n>> void Request::reuse(ReuseFlag flags)\n>> {\n>> +\tASSERT(canary_ == REQUEST_CANARY);\n>> +\n>> \tLIBCAMERA_TRACEPOINT(request_reuse, this);\n>> \n>> \tpending_.clear();\n>> @@ -179,6 +187,8 @@ void Request::reuse(ReuseFlag flags)\n>> */\n>> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>> {\n>> +\tASSERT(canary_ == REQUEST_CANARY);\n>> +\n>> \tif (!stream) {\n>> \t\tLOG(Request, Error) << \"Invalid stream reference\";\n>> \t\treturn -EINVAL;\n>> @@ -214,6 +224,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>> */\n>> FrameBuffer *Request::findBuffer(const Stream *stream) const\n>> {\n>> +\tASSERT(canary_ == REQUEST_CANARY);\n>> +\n>> \tconst auto it = bufferMap_.find(stream);\n>> \tif (it == bufferMap_.end())\n>> \t\treturn nullptr;\n>> @@ -281,6 +293,7 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n>> */\n>> void Request::complete()\n>> {\n>> +\tASSERT(canary_ == REQUEST_CANARY);\n>> \tASSERT(status_ == RequestPending);\n>> \tASSERT(!hasPendingBuffers());\n>> \n>> @@ -306,6 +319,8 @@ void Request::complete()\n>> */\n>> bool Request::completeBuffer(FrameBuffer *buffer)\n>> {\n>> +\tASSERT(canary_ == REQUEST_CANARY);\n>> +\n>> \tLIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n>> \n>> \tint ret = pending_.erase(buffer);\n>> @@ -326,6 +341,9 @@ std::string Request::toString() const\n>> \t/* Pending, Completed, Cancelled(X). */\n>> \tstatic const char *statuses = \"PCX\";\n>> \n>> +\tif (canary_ != REQUEST_CANARY)\n>> +\t\treturn \"Invalid Canary on Request\";\n> \n> As all bets are off in this case, should we ASSERT() to catch this\n> condition as early as possible ?\n> \n>> +\n>> \t/* Example Output: Request(55:P:1/2:6523524) */\n>> \tss << \"Request (\" << sequence_ << \":\" << statuses[status_] << \":\"\n>> \t << pending_.size() << \"/\" << bufferMap_.size() << \":\"\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 501A4C32EB\n\tfor ;\n\tFri, 26 Mar 2021 15:11:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92D4268D6D;\n\tFri, 26 Mar 2021 16:11:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 72CF3602D7\n\tfor ;\n\tFri, 26 Mar 2021 16:11:39 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CA499443;\n\tFri, 26 Mar 2021 16:11:38 +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=\"OZku2aaV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616771498;\n\tbh=EnxWoVSDEDqJ+j1We365RaGCit3eYEBtERIDY4nnHDw=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=OZku2aaVcu+KdGau2M3pz1wRrc/M3rMotv4yAWmOdovnWGboJaYLUCZsnL6AFoyzm\n\tm0vHU3oTvfB4wzMOuyEWYty4yqxAzJmiPAz3uziHN0AkIK8ZClXRgjPWGpJ3Ag8BZK\n\tdyvXB1FUNbmyxvT/igHqb+GDfevuzPCJ+Sjl/uts=","To":"Laurent Pinchart ","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>\n\t<20210325134231.1400051-8-kieran.bingham@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<0d480da3-560a-bb3a-09a7-40db65738784@ideasonboard.com>","Date":"Fri, 26 Mar 2021 15:11:35 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 05/11] libcamera: request: A\n\trequest canary","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16016,"web_url":"https://patchwork.libcamera.org/comment/16016/","msgid":"","date":"2021-03-29T11:41:38","subject":"Re: [libcamera-devel] [PATCH v3 05/11] libcamera: request: A\n\trequest canary","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 26/03/2021 02:51, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Thu, Mar 25, 2021 at 01:42:25PM +0000, Kieran Bingham wrote:\n>> Request objects are created and owned by the application, but are of\n>> course utilised widely throughout the internals of libcamera.\n>>\n>> If the application free's the requests while they are still active\n>> within libcamera a use after free will occur. While this can be trapped\n>> by tools such as valgrind, given the importance of this object and the\n>> relationship of external ownership, it may have some value to provide\n>> Debug build (disabled at Release build) assertions on the condition of\n>> these objects.\n>>\n>> Make sure the request fails an assertion immediately if used after free.\n>>\n>> Signed-off-by: Kieran Bingham \n>>\n>> ---\n>> I've removed the RFC from this, and I actually even more so believe this\n>> is a good facility to provide on the Request object.\n>>\n>> Mostly because the Request object is the only object which is given to\n>> libcamera from the application (yes, created by libcamera, but that's\n>> separate), and is expected to be free'd by the application.\n>>\n>> If an application free's requests while they are still in use within\n>> libcamera, the symptoms can be distinctly misleading and lead to rabbit\n>> holes.\n>>\n>> Therefore, I think the Request object is the one place where extra\n>> safety checking (in debug builds) is worth while.\n> \n> Should the canary_ member be compiled out in non-debug builds ?\n> \n>> Yes, of course if an application free's a request while it's in use with\n>> libcamera - then it's the applications fault, not ours - but because of\n>> the nature of requests, this could be an easy trap to fall into - and I\n>> don't want to find that reported as bugs in libcamera.\n> \n> When Request will become Extensible, do you think the canary_ field\n> could be moved to the Private class ? That would make me feel better\n> about this feature :-)\n> \n> Actually, maybe we could use the d_ pointer and o_ pointer to replace\n> the canary, but checking the point to each other ?\n\nThis is awkward, as the d pointer is stored in a unique_ptr, so\nperforming the validation would itself cause an out-of-bounds\naccess/segfault.\n\nThe canary is supposed to spot the break, not be the break.\n\nI've dropped this from this series for the moment to allow the rest to\nprogress.\n\n\n\n>> ---\n>> include/libcamera/request.h | 2 ++\n>> src/libcamera/request.cpp | 18 ++++++++++++++++++\n>> 2 files changed, 20 insertions(+)\n>>\n>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>> index 4cf5ff3f7d3b..965ffa6b45b2 100644\n>> --- a/include/libcamera/request.h\n>> +++ b/include/libcamera/request.h\n>> @@ -79,6 +79,8 @@ private:\n>> \tconst uint64_t cookie_;\n>> \tStatus status_;\n>> \tbool cancelled_;\n>> +\n>> +\tuint32_t canary_;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>> index 7b7ef6814686..c4258480b12b 100644\n>> --- a/src/libcamera/request.cpp\n>> +++ b/src/libcamera/request.cpp\n>> @@ -19,6 +19,8 @@\n>> #include \"libcamera/internal/log.h\"\n>> #include \"libcamera/internal/tracepoints.h\"\n>> \n>> +#define REQUEST_CANARY 0x1F2E3D4C\n>> +\n>> /**\n>> * \\file request.h\n>> * \\brief Describes a frame capture request to be processed by a camera\n>> @@ -90,6 +92,8 @@ Request::Request(Camera *camera, uint64_t cookie)\n>> \n>> \tLIBCAMERA_TRACEPOINT(request_construct, this);\n>> \n>> +\tcanary_ = REQUEST_CANARY;\n> \n> You can move this to the member initializer list.\n\nDone.\n\n> \n>> +\n>> \tLOG(Request, Debug) << \"Created request - cookie: \" << cookie_;\n>> }\n>> \n>> @@ -100,6 +104,8 @@ Request::~Request()\n>> \tdelete metadata_;\n>> \tdelete controls_;\n>> \tdelete validator_;\n>> +\n>> +\tcanary_ = 0;\n>> }\n>> \n>> /**\n>> @@ -114,6 +120,8 @@ Request::~Request()\n>> */\n>> void Request::reuse(ReuseFlag flags)\n>> {\n>> +\tASSERT(canary_ == REQUEST_CANARY);\n>> +\n>> \tLIBCAMERA_TRACEPOINT(request_reuse, this);\n>> \n>> \tpending_.clear();\n>> @@ -179,6 +187,8 @@ void Request::reuse(ReuseFlag flags)\n>> */\n>> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>> {\n>> +\tASSERT(canary_ == REQUEST_CANARY);\n>> +\n>> \tif (!stream) {\n>> \t\tLOG(Request, Error) << \"Invalid stream reference\";\n>> \t\treturn -EINVAL;\n>> @@ -214,6 +224,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>> */\n>> FrameBuffer *Request::findBuffer(const Stream *stream) const\n>> {\n>> +\tASSERT(canary_ == REQUEST_CANARY);\n>> +\n>> \tconst auto it = bufferMap_.find(stream);\n>> \tif (it == bufferMap_.end())\n>> \t\treturn nullptr;\n>> @@ -281,6 +293,7 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n>> */\n>> void Request::complete()\n>> {\n>> +\tASSERT(canary_ == REQUEST_CANARY);\n>> \tASSERT(status_ == RequestPending);\n>> \tASSERT(!hasPendingBuffers());\n>> \n>> @@ -306,6 +319,8 @@ void Request::complete()\n>> */\n>> bool Request::completeBuffer(FrameBuffer *buffer)\n>> {\n>> +\tASSERT(canary_ == REQUEST_CANARY);\n>> +\n>> \tLIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n>> \n>> \tint ret = pending_.erase(buffer);\n>> @@ -326,6 +341,9 @@ std::string Request::toString() const\n>> \t/* Pending, Completed, Cancelled(X). */\n>> \tstatic const char *statuses = \"PCX\";\n>> \n>> +\tif (canary_ != REQUEST_CANARY)\n>> +\t\treturn \"Invalid Canary on Request\";\n> \n> As all bets are off in this case, should we ASSERT() to catch this\n> condition as early as possible ?\n\nPerhaps, I had wanted a visual representation when it failed.\nBut a stack trace is that as well ;-)\n\n> \n>> +\n>> \t/* Example Output: Request(55:P:1/2:6523524) */\n>> \tss << \"Request (\" << sequence_ << \":\" << statuses[status_] << \":\"\n>> \t << pending_.size() << \"/\" << bufferMap_.size() << \":\"\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 BCBEDC32EA\n\tfor ;\n\tMon, 29 Mar 2021 11:41:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 384E768782;\n\tMon, 29 Mar 2021 13:41:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 368A46877D\n\tfor ;\n\tMon, 29 Mar 2021 13:41:42 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A6B49292;\n\tMon, 29 Mar 2021 13:41:41 +0200 (CEST)"],"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=\"Mo3PYxWv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617018101;\n\tbh=//Bsh5hDz8/VaUnI5gUwehSDqgo326eOQloRFCSiZj8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Mo3PYxWv2d/pLyCIxm78o8AuIw7Jb3a4hFpMUrGvePbmkmaHxFQL2n52qzskusNS3\n\tGSrngABfWlkU5yjpIexmIJ5zkhMKP3WC11jXRPGQX0f1Poy5ojhxxm5NcoH/asQj/m\n\t733x0pEoQNHHnf3UcHP1wC4bypQ/etOYlrUaBmrc=","To":"Laurent Pinchart ","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>\n\t<20210325134231.1400051-8-kieran.bingham@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 29 Mar 2021 12:41:38 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 05/11] libcamera: request: A\n\trequest canary","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]