From patchwork Tue Nov 30 03:38:20 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 14877 X-Patchwork-Delegate: laurent.pinchart@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 07BDEC3255 for ; Tue, 30 Nov 2021 03:39:16 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id A479760727; Tue, 30 Nov 2021 04:39:16 +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="YANpMEkU"; 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 082AC605D2 for ; Tue, 30 Nov 2021 04:38:59 +0100 (CET) Received: from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id A346B11C5 for ; Tue, 30 Nov 2021 04:38:58 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1638243538; bh=SqUv1cjL1iMmgnUdTeHgH5AAculkGF2lt3yu8foY4iY=; h=From:To:Subject:Date:In-Reply-To:References:From; b=YANpMEkU2RhdJwT7T9CyL+da0WhSF2ltglF3I9ot3rXBb+1NvEtN9tVtSbO1P+VSp GRWuqsXKo4X/MzTP0263sW68jdAYjI7s9Zbj0E//crSQWRBJ40a64mgPRc2PLkNWnw avLphdxlUxVX/9XKgZDFWrhytHY9iYenWex5+iw8= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Tue, 30 Nov 2021 05:38:20 +0200 Message-Id: <20211130033820.18235-23-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.32.0 In-Reply-To: <20211130033820.18235-1-laurent.pinchart@ideasonboard.com> References: <20211130033820.18235-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd: Pass rvalue reference to constructor and reset() 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" To avoid the risk of using a file descriptor whose ownership has been transferred to a UniqueFD instance, pass an rvalue reference to the constructor taking an int, and to the reset() function. The fd argument is set to -1 upon return, making incorrect usage of the file descriptor impossible. Signed-off-by: Laurent Pinchart --- I'm not entirely sure about this patch. If desired, it will get squashed with the one introducing UniqueFD. --- include/libcamera/base/unique_fd.h | 5 +++-- src/libcamera/base/shared_fd.cpp | 2 +- src/libcamera/base/unique_fd.cpp | 10 ++++++++-- src/libcamera/ipc_unixsocket.cpp | 4 ++-- src/libcamera/pipeline/raspberrypi/dma_heaps.cpp | 15 ++++++--------- src/libcamera/process.cpp | 4 ++-- src/libcamera/v4l2_videodevice.cpp | 2 +- test/unique-fd.cpp | 10 ++++++---- 8 files changed, 29 insertions(+), 23 deletions(-) diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h index ae4d96b75797..a5ffb1a435dc 100644 --- a/include/libcamera/base/unique_fd.h +++ b/include/libcamera/base/unique_fd.h @@ -22,9 +22,10 @@ public: { } - explicit UniqueFD(int fd) + explicit UniqueFD(int &&fd) : fd_(fd) { + fd = -1; } UniqueFD(UniqueFD &&other) @@ -50,7 +51,7 @@ public: return fd; } - void reset(int fd = -1); + void reset(int &&fd = -1); void swap(UniqueFD &other) { diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp index 0c8b93a47f85..8aecd34038bd 100644 --- a/src/libcamera/base/shared_fd.cpp +++ b/src/libcamera/base/shared_fd.cpp @@ -260,7 +260,7 @@ UniqueFD SharedFD::dup() const << "Failed to dup() fd: " << strerror(-ret); } - return UniqueFD(dupFd); + return UniqueFD(std::move(dupFd)); } SharedFD::Descriptor::Descriptor(int fd, bool duplicate) diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp index 83d6919cf623..7a961dbc01d0 100644 --- a/src/libcamera/base/unique_fd.cpp +++ b/src/libcamera/base/unique_fd.cpp @@ -38,9 +38,12 @@ LOG_DEFINE_CATEGORY(UniqueFD) */ /** - * \fn UniqueFD::UniqueFD(int fd) + * \fn UniqueFD::UniqueFD(int &&fd) * \brief Construct a UniqueFD that owns \a fd * \param[in] fd A file descriptor to manage + * + * Ownership of the file descriptor is transferred to the UniqueFD instance. + * Upon return \a fd is set to -1. */ /** @@ -88,11 +91,12 @@ LOG_DEFINE_CATEGORY(UniqueFD) * \param[in] fd The new file descriptor to manage * * Close the managed file descriptor, if any, and replace it with the new \a fd. + * Upon return \a fd is set to -1. * * Self-resetting (passing an \a fd already managed by this instance) is invalid * and results in undefined behaviour. */ -void UniqueFD::reset(int fd) +void UniqueFD::reset(int &&fd) { ASSERT(!isValid() || fd != fd_); @@ -100,6 +104,8 @@ void UniqueFD::reset(int fd) if (fd >= 0) close(fd); + + fd = -1; } /** diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp index 1980d374cea8..32bd6533d192 100644 --- a/src/libcamera/ipc_unixsocket.cpp +++ b/src/libcamera/ipc_unixsocket.cpp @@ -103,8 +103,8 @@ UniqueFD IPCUnixSocket::create() } std::array socketFds{ - UniqueFD(sockets[0]), - UniqueFD(sockets[1]), + UniqueFD(std::move(sockets[0])), + UniqueFD(std::move(sockets[1])), }; if (bind(std::move(socketFds[0])) < 0) diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp index 69831dabbe75..f2a95300d187 100644 --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp @@ -37,16 +37,13 @@ namespace RPi { DmaHeap::DmaHeap() { for (const char *name : heapNames) { - int ret = ::open(name, O_RDWR, 0); - if (ret < 0) { - ret = errno; - LOG(RPI, Debug) << "Failed to open " << name << ": " - << strerror(ret); - continue; - } + dmaHeapHandle_ = UniqueFD(open(name, O_RDWR, 0)); + if (dmaHeapHandle_.isValid()) + break; - dmaHeapHandle_ = UniqueFD(ret); - break; + int err = errno; + LOG(RPI, Debug) << "Failed to open " << name << ": " + << strerror(err); } if (!dmaHeapHandle_.isValid()) diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp index 0e6b4e1dde58..dc8576d934b6 100644 --- a/src/libcamera/process.cpp +++ b/src/libcamera/process.cpp @@ -134,8 +134,8 @@ ProcessManager::ProcessManager() LOG(Process, Fatal) << "Failed to initialize pipe for signal handling"; - pipe_[0] = UniqueFD(pipe[0]); - pipe_[1] = UniqueFD(pipe[1]); + pipe_[0] = UniqueFD(std::move(pipe[0])); + pipe_[1] = UniqueFD(std::move(pipe[1])); sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read); sigEvent_->activated.connect(this, &ProcessManager::sighandler); diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index b4b89e2759b9..9d0d0077f4e4 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -1396,7 +1396,7 @@ UniqueFD V4L2VideoDevice::exportDmabufFd(unsigned int index, return {}; } - return UniqueFD(expbuf.fd); + return UniqueFD(std::move(expbuf.fd)); } /** diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp index eb3b591fec28..7e94e3ca450e 100644 --- a/test/unique-fd.cpp +++ b/test/unique-fd.cpp @@ -39,7 +39,8 @@ protected: } /* Test creating UniqueFD from numerical file descriptor. */ - UniqueFD fd2(fd_); + int numFd = fd_; + UniqueFD fd2(std::move(numFd)); if (!fd2.isValid() || fd2.get() != fd_) { std::cout << "Failed fd check (fd constructor)" << std::endl; @@ -113,7 +114,7 @@ protected: } /* Test release. */ - int numFd = fd2.release(); + numFd = fd2.release(); if (fd2.isValid() || fd2.get() != -1) { std::cout << "Failed fd check (release)" << std::endl; @@ -133,7 +134,7 @@ protected: } /* Test reset assignment. */ - fd.reset(numFd); + fd.reset(std::move(numFd)); if (!fd.isValid() || fd.get() != fd_) { std::cout << "Failed fd check (reset assignment)" << std::endl; @@ -168,7 +169,8 @@ protected: } { - UniqueFD fd4(fd_); + numFd = fd_; + UniqueFD fd4(std::move(numFd)); } if (isValidFd(fd_)) {