From patchwork Thu Mar 25 13:42:25 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kieran Bingham X-Patchwork-Id: 11715 X-Patchwork-Delegate: kieran.bingham@ideasonboard.com 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 6B5EFC32EA for ; Thu, 25 Mar 2021 13:42:45 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 2640D68D82; Thu, 25 Mar 2021 14:42:45 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="dYr77qpk"; dkim-atps=neutral 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 C7F7D6084F for ; Thu, 25 Mar 2021 14:42:37 +0100 (CET) Received: from Q.local (cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 71A678F0; Thu, 25 Mar 2021 14:42:37 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1616679757; bh=x4dfp0V8U+JXITHQd/RnB9wllql332QTPjl8OdeHnv8=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=dYr77qpkLKywaywu0N9duaQYKT3FjPeJ5b2utyCDUsQh84lU+eChwf+36jNipS1ph khRTFplOwnhovVchTRUahyYKMXUbAyLwDL9Cnpih51WTSkmAEKEUl6a4wUiPQuCyXh x0BX/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 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: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Request objects are created and owned by the application, but are of course 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 Debug build (disabled at Release build) assertions on the condition of these objects. Make sure the request fails an assertion immediately if used after free. Signed-off-by: Kieran Bingham --- I've removed the RFC from this, and I actually even more so believe this is a good facility to provide on the Request object. Mostly because the Request object is the only object which is given to libcamera from the application (yes, created by libcamera, but that's separate), and is expected to be free'd by the application. If an application free's requests while they are still in use within libcamera, the symptoms can be distinctly misleading and lead to rabbit holes. Therefore, I think the Request object is the one place where extra safety checking (in debug builds) is worth while. Yes, of course if an application free's a request while it's in use with libcamera - then it's the applications fault, not ours - but because of the nature of requests, this could be an easy trap to fall into - and I don't want to find that reported as bugs in libcamera. --- include/libcamera/request.h | 2 ++ src/libcamera/request.cpp | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 4cf5ff3f7d3b..965ffa6b45b2 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -79,6 +79,8 @@ private: const uint64_t cookie_; Status status_; bool cancelled_; + + uint32_t canary_; }; } /* namespace libcamera */ diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 7b7ef6814686..c4258480b12b 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -19,6 +19,8 @@ #include "libcamera/internal/log.h" #include "libcamera/internal/tracepoints.h" +#define REQUEST_CANARY 0x1F2E3D4C + /** * \file request.h * \brief Describes a frame capture request to be processed by a camera @@ -90,6 +92,8 @@ Request::Request(Camera *camera, uint64_t cookie) LIBCAMERA_TRACEPOINT(request_construct, this); + canary_ = REQUEST_CANARY; + LOG(Request, Debug) << "Created request - cookie: " << cookie_; } @@ -100,6 +104,8 @@ Request::~Request() delete metadata_; delete controls_; delete validator_; + + canary_ = 0; } /** @@ -114,6 +120,8 @@ Request::~Request() */ void Request::reuse(ReuseFlag flags) { + ASSERT(canary_ == REQUEST_CANARY); + LIBCAMERA_TRACEPOINT(request_reuse, this); pending_.clear(); @@ -179,6 +187,8 @@ void Request::reuse(ReuseFlag flags) */ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) { + ASSERT(canary_ == REQUEST_CANARY); + if (!stream) { LOG(Request, Error) << "Invalid stream reference"; return -EINVAL; @@ -214,6 +224,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) */ FrameBuffer *Request::findBuffer(const Stream *stream) const { + ASSERT(canary_ == REQUEST_CANARY); + const auto it = bufferMap_.find(stream); if (it == bufferMap_.end()) return nullptr; @@ -281,6 +293,7 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const */ void Request::complete() { + ASSERT(canary_ == REQUEST_CANARY); ASSERT(status_ == RequestPending); ASSERT(!hasPendingBuffers()); @@ -306,6 +319,8 @@ void Request::complete() */ bool Request::completeBuffer(FrameBuffer *buffer) { + ASSERT(canary_ == REQUEST_CANARY); + LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer); int ret = pending_.erase(buffer); @@ -326,6 +341,9 @@ std::string Request::toString() const /* Pending, Completed, Cancelled(X). */ static const char *statuses = "PCX"; + if (canary_ != REQUEST_CANARY) + return "Invalid Canary on Request"; + /* Example Output: Request(55:P:1/2:6523524) */ ss << "Request (" << sequence_ << ":" << statuses[status_] << ":" << pending_.size() << "/" << bufferMap_.size() << ":"