From patchwork Mon Jan 30 18:02:44 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kieran Bingham X-Patchwork-Id: 18226 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 76702C329C for ; Mon, 30 Jan 2023 18:02:52 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 3CFE2625EC; Mon, 30 Jan 2023 19:02:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1675101771; bh=47XYufcDxTkadwUrLh+hOs5D7q5hSw7aJs5za3bwjkk=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=gILnagd+nWM6T1s53C9KVCxAlsrqFnt+WK3bEzlKoiXDK0J7p89YTPvK1Y7kf2Aoo jCwOchWVOOjdg5vXuc8sQz067hjXqO7FNp69UPwSG8/gaxegdiEd1W2yhjd21GfGSA T3rRwgKMGiz7Fb87BdLXrDBDXJbxiuPudB6ogvbuOCoixJPRTFs/Nz7/VGTPSGHY89 EXQDgoVst3FdMiiOhZVFakXsF6VjxM10XOSeo6f8TxQsikoCHVe2X6qjA9pbzY85Pq 1wNzjIHs/j3D4bi//QxbcScNMcuiY7RbJNVrzTFb37ZSIMkfezjHR5Yyv0W9K2KBne f5PtG790ODDQQ== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id CF82A60482 for ; Mon, 30 Jan 2023 19:02:48 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="Ul60Hy8K"; dkim-atps=neutral Received: from Monstersaurus.local (cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 63F26D5F; Mon, 30 Jan 2023 19:02:48 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1675101768; bh=47XYufcDxTkadwUrLh+hOs5D7q5hSw7aJs5za3bwjkk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Ul60Hy8K3ndm3+sK09aCAP2T92lNkfhKlgP7J39kbdwKaTdWUBe6wm1aZitEfmt9d VfV7XMy6S+D2ohk3RnUSRUZj/lqSZEqP0pVfPXCgLW7NL6aNhAsapXPjZPEwVSayDt 55ntRrSal+cdCTCPtPDLiV4Vq57fQb7vQlUEkR2Q= To: libcamera devel 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 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: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Kieran Bingham via libcamera-devel From: Kieran Bingham Reply-To: Kieran Bingham Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Request objects are created and owned by the application, but are utilised widely throughout the internals of libcamera. If the application free's the requests while they are still active within libcamera a use after free will occur. While this can be trapped by tools such as valgrind, given the importance of this object and the relationship of external ownership, it may have some value to provide extended assertions on the condition of these objects. Make sure the request fails an assertion immediately if used after free. Further more, this allows us to immediately reject invalid Request objects directly from the Camera queueRequest API. Signed-off-by: Kieran Bingham Reviewed-by: Paul Elder --- v2? - Added canary to both the public and private request objects. - Added validation to the Camera queueRequest(). Signed-off-by: Kieran Bingham --- include/libcamera/internal/request.h | 2 ++ include/libcamera/request.h | 4 +++ src/libcamera/camera.cpp | 5 +++ src/libcamera/request.cpp | 54 ++++++++++++++++++++++++++-- 4 files changed, 63 insertions(+), 2 deletions(-) diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h index 8c92a27a95e5..981ad184fffa 100644 --- a/include/libcamera/internal/request.h +++ b/include/libcamera/internal/request.h @@ -59,6 +59,8 @@ private: std::unordered_set pending_; std::map> notifiers_; std::unique_ptr timer_; + + uint32_t canary_; }; } /* namespace libcamera */ diff --git a/include/libcamera/request.h b/include/libcamera/request.h index dffde1536cad..8e377de14b12 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -65,6 +65,8 @@ public: std::string toString() const; + bool canary() const; + private: LIBCAMERA_DISABLE_COPY(Request) @@ -74,6 +76,8 @@ private: const uint64_t cookie_; Status status_; + + int32_t canary_; }; std::ostream &operator<<(std::ostream &out, const Request &r); diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 48bf19b28979..3b72098cce59 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1116,6 +1116,11 @@ int Camera::queueRequest(Request *request) { Private *const d = _d(); + if (request->canary()) { + LOG(Camera, Error) << "Invalid request"; + return -EINVAL; + } + int ret = d->isAccessAllowed(Private::CameraRunning); if (ret < 0) return ret; diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 949c556fa437..cfe732765f86 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -23,6 +23,8 @@ #include "libcamera/internal/framebuffer.h" #include "libcamera/internal/tracepoints.h" +#define REQUEST_CANARY 0x1F2E3D4C + /** * \file libcamera/request.h * \brief Describes a frame capture request to be processed by a camera @@ -48,13 +50,14 @@ LOG_DEFINE_CATEGORY(Request) * \param camera The Camera that creates the request */ Request::Private::Private(Camera *camera) - : camera_(camera), cancelled_(false) + : camera_(camera), cancelled_(false), canary_(REQUEST_CANARY) { } Request::Private::~Private() { doCancelRequest(); + canary_ = 0; } /** @@ -114,6 +117,7 @@ void Request::Private::complete() { Request *request = _o(); + ASSERT(canary_ == REQUEST_CANARY); ASSERT(request->status() == RequestPending); ASSERT(!hasPendingBuffers()); @@ -128,6 +132,8 @@ void Request::Private::doCancelRequest() { Request *request = _o(); + ASSERT(canary_ == REQUEST_CANARY); + for (FrameBuffer *buffer : pending_) { buffer->_d()->cancel(); camera_->bufferCompleted.emit(request, buffer); @@ -149,6 +155,8 @@ void Request::Private::doCancelRequest() */ void Request::Private::cancel() { + ASSERT(canary_ == REQUEST_CANARY); + LIBCAMERA_TRACEPOINT(request_cancel, this); Request *request = _o(); @@ -346,7 +354,7 @@ void Request::Private::timeout() */ Request::Request(Camera *camera, uint64_t cookie) : Extensible(std::make_unique(camera)), - cookie_(cookie), status_(RequestPending) + cookie_(cookie), status_(RequestPending), canary_(REQUEST_CANARY) { controls_ = new ControlList(controls::controls, camera->_d()->validator()); @@ -367,6 +375,8 @@ Request::~Request() delete metadata_; delete controls_; + + canary_ = 0; } /** @@ -381,6 +391,11 @@ Request::~Request() */ void Request::reuse(ReuseFlag flags) { + if (canary_ != REQUEST_CANARY) { + LOG(Request, Error) << "Invalid Request object"; + return; + } + LIBCAMERA_TRACEPOINT(request_reuse, this); _d()->reset(); @@ -462,6 +477,11 @@ void Request::reuse(ReuseFlag flags) int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, std::unique_ptr fence) { + if (canary_ != REQUEST_CANARY) { + LOG(Request, Error) << "Attempt to add buffer to invalid request"; + return -EINVAL; + } + if (!stream) { LOG(Request, Error) << "Invalid stream reference"; return -EINVAL; @@ -509,6 +529,11 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, */ FrameBuffer *Request::findBuffer(const Stream *stream) const { + if (canary_ != REQUEST_CANARY) { + LOG(Request, Error) << "Invalid Request object"; + return nullptr; + } + const auto it = bufferMap_.find(stream); if (it == bufferMap_.end()) return nullptr; @@ -571,6 +596,7 @@ uint32_t Request::sequence() const */ bool Request::hasPendingBuffers() const { + ASSERT(canary_ == REQUEST_CANARY); return !_d()->pending_.empty(); } @@ -590,6 +616,25 @@ std::string Request::toString() const return ss.str(); } +/** + * \brief Identify if the Request object is valid and alive + * + * This provides a means of checking if the request is a valid request object. + * While Requests are constructed by libcamera, they are owned and may be freed + * by applications. It can be all to easy too release a Request object while it + * is still in use by libcamera - or attempt to requeue invalid or deleted + * requests. + * + * The canary provides an insight that the object is not valid and shall be + * rejected by libcamera API calls. + * + * \return True if the canary has died, and the object shall not be trusted + */ +bool Request::canary() const +{ + return canary_ != REQUEST_CANARY; +} + /** * \brief Insert a text representation of a Request into an output stream * \param[in] out The output stream @@ -601,6 +646,11 @@ std::ostream &operator<<(std::ostream &out, const Request &r) /* Pending, Completed, Cancelled(X). */ static const char *statuses = "PCX"; + if (r.canary()) { + out << "Request(Invalid Canary)"; + return out; + } + /* Example Output: Request(55:P:1/2:6523524) */ out << "Request(" << r.sequence() << ":" << statuses[r.status()] << ":" << r._d()->pending_.size() << "/" << r.buffers().size() << ":"