Patch Detail
Show a patch.
GET /api/1.1/patches/18226/?format=api
{ "id": 18226, "url": "https://patchwork.libcamera.org/api/1.1/patches/18226/?format=api", "web_url": "https://patchwork.libcamera.org/patch/18226/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20230130180244.2150205-3-kieran.bingham@ideasonboard.com>", "date": "2023-01-30T18:02:44", "name": "[libcamera-devel,2/2] libcamera: request: A request canary", "commit_ref": null, "pull_url": null, "state": "new", "archived": false, "hash": "fc34886bfdf0199c01679471eb02ef330fabfd22", "submitter": { "id": 4, "url": "https://patchwork.libcamera.org/api/1.1/people/4/?format=api", "name": "Kieran Bingham", "email": "kieran.bingham@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/18226/mbox/", "series": [ { "id": 3732, "url": "https://patchwork.libcamera.org/api/1.1/series/3732/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=3732", "date": "2023-01-30T18:02:42", "name": "Camera/Request reliability", "version": 1, "mbox": "https://patchwork.libcamera.org/series/3732/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/18226/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/18226/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 76702C329C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 30 Jan 2023 18:02:52 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CFE2625EC;\n\tMon, 30 Jan 2023 19:02:51 +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 CF82A60482\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 30 Jan 2023 19:02:48 +0100 (CET)", "from Monstersaurus.local\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 63F26D5F;\n\tMon, 30 Jan 2023 19:02:48 +0100 (CET)" ], "DKIM-Signature": [ "v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675101771;\n\tbh=47XYufcDxTkadwUrLh+hOs5D7q5hSw7aJs5za3bwjkk=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=gILnagd+nWM6T1s53C9KVCxAlsrqFnt+WK3bEzlKoiXDK0J7p89YTPvK1Y7kf2Aoo\n\tjCwOchWVOOjdg5vXuc8sQz067hjXqO7FNp69UPwSG8/gaxegdiEd1W2yhjd21GfGSA\n\tT3rRwgKMGiz7Fb87BdLXrDBDXJbxiuPudB6ogvbuOCoixJPRTFs/Nz7/VGTPSGHY89\n\tEXQDgoVst3FdMiiOhZVFakXsF6VjxM10XOSeo6f8TxQsikoCHVe2X6qjA9pbzY85Pq\n\t1wNzjIHs/j3D4bi//QxbcScNMcuiY7RbJNVrzTFb37ZSIMkfezjHR5Yyv0W9K2KBne\n\tf5PtG790ODDQQ==", "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675101768;\n\tbh=47XYufcDxTkadwUrLh+hOs5D7q5hSw7aJs5za3bwjkk=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=Ul60Hy8K3ndm3+sK09aCAP2T92lNkfhKlgP7J39kbdwKaTdWUBe6wm1aZitEfmt9d\n\tVfV7XMy6S+D2ohk3RnUSRUZj/lqSZEqP0pVfPXCgLW7NL6aNhAsapXPjZPEwVSayDt\n\t55ntRrSal+cdCTCPtPDLiV4Vq57fQb7vQlUEkR2Q=" ], "Authentication-Results": "lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Ul60Hy8K\"; dkim-atps=neutral", "To": "libcamera devel <libcamera-devel@lists.libcamera.org>", "Date": "Mon, 30 Jan 2023 18:02:44 +0000", "Message-Id": "<20230130180244.2150205-3-kieran.bingham@ideasonboard.com>", "X-Mailer": "git-send-email 2.34.1", "In-Reply-To": "<20230130180244.2150205-1-kieran.bingham@ideasonboard.com>", "References": "<20230130180244.2150205-1-kieran.bingham@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH 2/2] libcamera: request: A request canary", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "From": "Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>", "Reply-To": "Kieran Bingham <kieran.bingham@ideasonboard.com>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "Request objects are created and owned by the application, but are\nutilised 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\nextended assertions on the condition of these objects.\n\nMake sure the request fails an assertion immediately if used after free.\n\nFurther more, this allows us to immediately reject invalid Request\nobjects directly from the Camera queueRequest API.\n\nSigned-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n---\n\nv2?\n - Added canary to both the public and private request objects.\n - Added validation to the Camera queueRequest().\n\nSigned-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n---\n include/libcamera/internal/request.h | 2 ++\n include/libcamera/request.h | 4 +++\n src/libcamera/camera.cpp | 5 +++\n src/libcamera/request.cpp | 54 ++++++++++++++++++++++++++--\n 4 files changed, 63 insertions(+), 2 deletions(-)", "diff": "diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\nindex 8c92a27a95e5..981ad184fffa 100644\n--- a/include/libcamera/internal/request.h\n+++ b/include/libcamera/internal/request.h\n@@ -59,6 +59,8 @@ private:\n \tstd::unordered_set<FrameBuffer *> pending_;\n \tstd::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;\n \tstd::unique_ptr<Timer> timer_;\n+\n+\tuint32_t canary_;\n };\n \n } /* namespace libcamera */\ndiff --git a/include/libcamera/request.h b/include/libcamera/request.h\nindex dffde1536cad..8e377de14b12 100644\n--- a/include/libcamera/request.h\n+++ b/include/libcamera/request.h\n@@ -65,6 +65,8 @@ public:\n \n \tstd::string toString() const;\n \n+\tbool canary() const;\n+\n private:\n \tLIBCAMERA_DISABLE_COPY(Request)\n \n@@ -74,6 +76,8 @@ private:\n \n \tconst uint64_t cookie_;\n \tStatus status_;\n+\n+\tint32_t canary_;\n };\n \n std::ostream &operator<<(std::ostream &out, const Request &r);\ndiff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\nindex 48bf19b28979..3b72098cce59 100644\n--- a/src/libcamera/camera.cpp\n+++ b/src/libcamera/camera.cpp\n@@ -1116,6 +1116,11 @@ int Camera::queueRequest(Request *request)\n {\n \tPrivate *const d = _d();\n \n+\tif (request->canary()) {\n+\t\tLOG(Camera, Error) << \"Invalid request\";\n+\t\treturn -EINVAL;\n+\t}\n+\n \tint ret = d->isAccessAllowed(Private::CameraRunning);\n \tif (ret < 0)\n \t\treturn ret;\ndiff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\nindex 949c556fa437..cfe732765f86 100644\n--- a/src/libcamera/request.cpp\n+++ b/src/libcamera/request.cpp\n@@ -23,6 +23,8 @@\n #include \"libcamera/internal/framebuffer.h\"\n #include \"libcamera/internal/tracepoints.h\"\n \n+#define REQUEST_CANARY 0x1F2E3D4C\n+\n /**\n * \\file libcamera/request.h\n * \\brief Describes a frame capture request to be processed by a camera\n@@ -48,13 +50,14 @@ LOG_DEFINE_CATEGORY(Request)\n * \\param camera The Camera that creates the request\n */\n Request::Private::Private(Camera *camera)\n-\t: camera_(camera), cancelled_(false)\n+\t: camera_(camera), cancelled_(false), canary_(REQUEST_CANARY)\n {\n }\n \n Request::Private::~Private()\n {\n \tdoCancelRequest();\n+\tcanary_ = 0;\n }\n \n /**\n@@ -114,6 +117,7 @@ void Request::Private::complete()\n {\n \tRequest *request = _o<Request>();\n \n+\tASSERT(canary_ == REQUEST_CANARY);\n \tASSERT(request->status() == RequestPending);\n \tASSERT(!hasPendingBuffers());\n \n@@ -128,6 +132,8 @@ void Request::Private::doCancelRequest()\n {\n \tRequest *request = _o<Request>();\n \n+\tASSERT(canary_ == REQUEST_CANARY);\n+\n \tfor (FrameBuffer *buffer : pending_) {\n \t\tbuffer->_d()->cancel();\n \t\tcamera_->bufferCompleted.emit(request, buffer);\n@@ -149,6 +155,8 @@ void Request::Private::doCancelRequest()\n */\n void Request::Private::cancel()\n {\n+\tASSERT(canary_ == REQUEST_CANARY);\n+\n \tLIBCAMERA_TRACEPOINT(request_cancel, this);\n \n \tRequest *request = _o<Request>();\n@@ -346,7 +354,7 @@ void Request::Private::timeout()\n */\n Request::Request(Camera *camera, uint64_t cookie)\n \t: Extensible(std::make_unique<Private>(camera)),\n-\t cookie_(cookie), status_(RequestPending)\n+\t cookie_(cookie), status_(RequestPending), canary_(REQUEST_CANARY)\n {\n \tcontrols_ = new ControlList(controls::controls,\n \t\t\t\t camera->_d()->validator());\n@@ -367,6 +375,8 @@ Request::~Request()\n \n \tdelete metadata_;\n \tdelete controls_;\n+\n+\tcanary_ = 0;\n }\n \n /**\n@@ -381,6 +391,11 @@ Request::~Request()\n */\n void Request::reuse(ReuseFlag flags)\n {\n+\tif (canary_ != REQUEST_CANARY) {\n+\t\tLOG(Request, Error) << \"Invalid Request object\";\n+\t\treturn;\n+\t}\n+\n \tLIBCAMERA_TRACEPOINT(request_reuse, this);\n \n \t_d()->reset();\n@@ -462,6 +477,11 @@ void Request::reuse(ReuseFlag flags)\n int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,\n \t\t std::unique_ptr<Fence> fence)\n {\n+\tif (canary_ != REQUEST_CANARY) {\n+\t\tLOG(Request, Error) << \"Attempt to add buffer to invalid request\";\n+\t\treturn -EINVAL;\n+\t}\n+\n \tif (!stream) {\n \t\tLOG(Request, Error) << \"Invalid stream reference\";\n \t\treturn -EINVAL;\n@@ -509,6 +529,11 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,\n */\n FrameBuffer *Request::findBuffer(const Stream *stream) const\n {\n+\tif (canary_ != REQUEST_CANARY) {\n+\t\tLOG(Request, Error) << \"Invalid Request object\";\n+\t\treturn nullptr;\n+\t}\n+\n \tconst auto it = bufferMap_.find(stream);\n \tif (it == bufferMap_.end())\n \t\treturn nullptr;\n@@ -571,6 +596,7 @@ uint32_t Request::sequence() const\n */\n bool Request::hasPendingBuffers() const\n {\n+\tASSERT(canary_ == REQUEST_CANARY);\n \treturn !_d()->pending_.empty();\n }\n \n@@ -590,6 +616,25 @@ std::string Request::toString() const\n \treturn ss.str();\n }\n \n+/**\n+ * \\brief Identify if the Request object is valid and alive\n+ *\n+ * This provides a means of checking if the request is a valid request object.\n+ * While Requests are constructed by libcamera, they are owned and may be freed\n+ * by applications. It can be all to easy too release a Request object while it\n+ * is still in use by libcamera - or attempt to requeue invalid or deleted\n+ * requests.\n+ *\n+ * The canary provides an insight that the object is not valid and shall be\n+ * rejected by libcamera API calls.\n+ *\n+ * \\return True if the canary has died, and the object shall not be trusted\n+ */\n+bool Request::canary() const\n+{\n+\treturn canary_ != REQUEST_CANARY;\n+}\n+\n /**\n * \\brief Insert a text representation of a Request into an output stream\n * \\param[in] out The output stream\n@@ -601,6 +646,11 @@ std::ostream &operator<<(std::ostream &out, const Request &r)\n \t/* Pending, Completed, Cancelled(X). */\n \tstatic const char *statuses = \"PCX\";\n \n+\tif (r.canary()) {\n+\t\tout << \"Request(Invalid Canary)\";\n+\t\treturn out;\n+\t}\n+\n \t/* Example Output: Request(55:P:1/2:6523524) */\n \tout << \"Request(\" << r.sequence() << \":\" << statuses[r.status()] << \":\"\n \t << r._d()->pending_.size() << \"/\" << r.buffers().size() << \":\"\n", "prefixes": [ "libcamera-devel", "2/2" ] }