[libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd: Pass rvalue reference to constructor and reset()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 30 04:38:20 CET 2021
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 <laurent.pinchart at ideasonboard.com>
---
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<UniqueFD, 2> 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_)) {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list