From patchwork Fri Mar 12 06:11:31 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kieran Bingham X-Patchwork-Id: 11568 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 30C57BD1F1 for ; Fri, 12 Mar 2021 06:11:45 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id CE4F168C84; Fri, 12 Mar 2021 07:11:44 +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="h0290R9X"; 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 01B0468C6B for ; Fri, 12 Mar 2021 07:11:39 +0100 (CET) Received: from localhost.localdomain (cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C912F95; Fri, 12 Mar 2021 07:11:38 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1615529498; bh=CXpckEd6t9K6vgSZfAjDek218hD66ksQ8YyosdKLQ20=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=h0290R9XE7bqzekZ+WghhU1f7gnFJ2Iqx5hpFGterajV/L/9nSJO/0v0Imo0KTVX3 h3wXH5U76ZV7XPDdNfrILkfVO6vcccdeUuR4fPu5ccHAH738HnRpfuG/mBD9D+H+5d ysLIZrTTvvpgMbgCyUzlk1Z+BhHWoTamCxIq3BmE= From: Kieran Bingham To: libcamera devel Date: Fri, 12 Mar 2021 06:11:31 +0000 Message-Id: <20210312061131.854849-9-kieran.bingham@ideasonboard.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210312061131.854849-1-kieran.bingham@ideasonboard.com> References: <20210312061131.854849-1-kieran.bingham@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 8/8] [RFC-Only] 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 --- RFC only, as I was going to dispose of this patch. We added it while debugging the IPU3 stability, and it may prove useful to catch errors if requests are used after they are released. However this may be redundant as pipeline handlers should guarantee that requests are fully completed when they stop(). Of course the IPU3 wasn't meeting that requirement, and we do not have a specific assertion that validates that requirement on all pipeline handlers. So perhaps this canary might serve as a beneficial guard? If not, at least posting it will mean it can be used in the future if it comes up again, or the concept could be applied to other objects if appropriate. include/libcamera/request.h | 2 ++ src/libcamera/request.cpp | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 59d7f4bac0d2..fc56d63c8c67 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -78,6 +78,8 @@ private: const uint64_t cookie_; Status status_; bool cancelled_; + + int canary_; }; } /* namespace libcamera */ diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 12c2e7d425f9..83169a11e1e5 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -18,6 +18,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 @@ -89,6 +91,8 @@ Request::Request(Camera *camera, uint64_t cookie) LIBCAMERA_TRACEPOINT(request_construct, this); + canary_ = REQUEST_CANARY; + LOG(Request, Debug) << "Created request - cookie: " << cookie_; } @@ -99,6 +103,8 @@ Request::~Request() delete metadata_; delete controls_; delete validator_; + + canary_ = 0; } /** @@ -113,6 +119,8 @@ Request::~Request() */ void Request::reuse(ReuseFlag flags) { + ASSERT(canary_ == REQUEST_CANARY); + LIBCAMERA_TRACEPOINT(request_reuse, this); pending_.clear(); @@ -176,6 +184,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; @@ -211,6 +221,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; @@ -262,6 +274,7 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const */ void Request::complete() { + ASSERT(canary_ == REQUEST_CANARY); ASSERT(status_ == RequestPending); ASSERT(!hasPendingBuffers()); @@ -289,6 +302,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);