{"id":11715,"url":"https://patchwork.libcamera.org/api/1.1/patches/11715/?format=json","web_url":"https://patchwork.libcamera.org/patch/11715/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/1.1/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20210325134231.1400051-8-kieran.bingham@ideasonboard.com>","date":"2021-03-25T13:42:25","name":"[libcamera-devel,v3,05/11] libcamera: request: A request canary","commit_ref":null,"pull_url":null,"state":"accepted","archived":false,"hash":"6facb27083b8c7ae0ecad58910d04f0a99c475f9","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/1.1/people/4/?format=json","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"delegate":{"id":11,"url":"https://patchwork.libcamera.org/api/1.1/users/11/?format=json","username":"kbingham","first_name":"Kieran","last_name":"Bingham","email":"kieran.bingham@ideasonboard.com"},"mbox":"https://patchwork.libcamera.org/patch/11715/mbox/","series":[{"id":1832,"url":"https://patchwork.libcamera.org/api/1.1/series/1832/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=1832","date":"2021-03-25T13:42:18","name":"[libcamera-devel,v3,01/11] utils: ipc: proxy: Track IPA with a state machine","version":1,"mbox":"https://patchwork.libcamera.org/series/1832/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/11715/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/11715/checks/","tags":{},"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 6B5EFC32EA\n\tfor ;\n\tThu, 25 Mar 2021 13:42:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2640D68D82;\n\tThu, 25 Mar 2021 14:42:45 +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 C7F7D6084F\n\tfor ;\n\tThu, 25 Mar 2021 14:42:37 +0100 (CET)","from Q.local (cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net\n\t[86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 71A678F0;\n\tThu, 25 Mar 2021 14:42: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=\"dYr77qpk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616679757;\n\tbh=x4dfp0V8U+JXITHQd/RnB9wllql332QTPjl8OdeHnv8=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=dYr77qpkLKywaywu0N9duaQYKT3FjPeJ5b2utyCDUsQh84lU+eChwf+36jNipS1ph\n\tkhRTFplOwnhovVchTRUahyYKMXUbAyLwDL9Cnpih51WTSkmAEKEUl6a4wUiPQuCyXh\n\tx0BX/wEazD5JOtPjlw/PpGroqKvUF0c06Ba5qdkc=","From":"Kieran Bingham ","To":"libcamera devel ","Date":"Thu, 25 Mar 2021 13:42:25 +0000","Message-Id":"<20210325134231.1400051-8-kieran.bingham@ideasonboard.com>","X-Mailer":"git-send-email 2.25.1","In-Reply-To":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Subject":"[libcamera-devel] [PATCH v3 05/11] libcamera: request: A request\n\tcanary","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "},"content":"Request objects are created and owned by the application, but are of\ncourse utilised widely throughout the internals of libcamera.\n\nIf the application free's the requests while they are still active\nwithin libcamera a use after free will occur. While this can be trapped\nby tools such as valgrind, given the importance of this object and the\nrelationship of external ownership, it may have some value to provide\nDebug build (disabled at Release build) assertions on the condition of\nthese objects.\n\nMake sure the request fails an assertion immediately if used after free.\n\nSigned-off-by: Kieran Bingham \n\n---\nI've removed the RFC from this, and I actually even more so believe this\nis a good facility to provide on the Request object.\n\nMostly because the Request object is the only object which is given to\nlibcamera from the application (yes, created by libcamera, but that's\nseparate), and is expected to be free'd by the application.\n\nIf an application free's requests while they are still in use within\nlibcamera, the symptoms can be distinctly misleading and lead to rabbit\nholes.\n\nTherefore, I think the Request object is the one place where extra\nsafety checking (in debug builds) is worth while.\n\nYes, of course if an application free's a request while it's in use with\nlibcamera - then it's the applications fault, not ours - but because of\nthe nature of requests, this could be an easy trap to fall into - and I\ndon't want to find that reported as bugs in libcamera.\n---\n include/libcamera/request.h | 2 ++\n src/libcamera/request.cpp | 18 ++++++++++++++++++\n 2 files changed, 20 insertions(+)","diff":"diff --git a/include/libcamera/request.h b/include/libcamera/request.h\nindex 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 */\ndiff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\nindex 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 \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 \t/* Example Output: Request(55:P:1/2:6523524) */\n \tss << \"Request (\" << sequence_ << \":\" << statuses[status_] << \":\"\n \t << pending_.size() << \"/\" << bufferMap_.size() << \":\"\n","prefixes":["libcamera-devel","v3","05/11"]}