This is an automated email from the ASF dual-hosted git repository.
jihuayu pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new d8aeed41c fix(common): correct UniqueFD move and reset ownership
semantics (#3692)
d8aeed41c is described below
commit d8aeed41c386b40a97dce278277b76d2a1726a67
Author: nkysg <[email protected]>
AuthorDate: Mon Oct 19 15:08:24 2026 +0800
fix(common): correct UniqueFD move and reset ownership semantics (#3692)
## Summary
Fix ownership-transfer issues in `UniqueFD`.
- Close the currently owned FD before move assignment takes ownership.
- Make self-move assignment safe.
- Prevent `Reset(Get())` from closing the FD and retaining a stale
descriptor value.
- Use `Release()` to centralize ownership transfer semantics.
These cases could otherwise lead to FD leaks, double-closes, or closing
an unrelated reused descriptor.
## Tests
- Added focused unit tests for move assignment, self-move assignment,
reset with the same FD, and release.
- `./x.py check format`
- `./x.py build --unittest`
- `./x.py test cpp` — 663 passed, 1 skipped
---
src/common/unique_fd.h | 21 ++++-------
tests/cppunit/unique_fd_test.cc | 83 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 91 insertions(+), 13 deletions(-)
diff --git a/src/common/unique_fd.h b/src/common/unique_fd.h
index 57fb46e0b..f59aed224 100644
--- a/src/common/unique_fd.h
+++ b/src/common/unique_fd.h
@@ -36,28 +36,23 @@ struct UniqueFD {
}
UniqueFD(const UniqueFD&) = delete;
- UniqueFD(UniqueFD&& f) : fd_(f.fd_) { f.fd_ = NullFD; }
+ UniqueFD(UniqueFD&& f) noexcept : fd_(f.Release()) {}
UniqueFD& operator=(const UniqueFD&) = delete;
- UniqueFD& operator=(UniqueFD&& f) {
- fd_ = f.fd_;
- f.fd_ = NullFD;
+ UniqueFD& operator=(UniqueFD&& f) noexcept {
+ if (this != &f) Reset(f.Release());
return *this;
}
- int Release() {
- int fd = fd_;
- fd_ = NullFD;
- return fd;
- }
+ int Release() noexcept { return std::exchange(fd_, NullFD); }
- void Reset(int fd = NullFD) {
- int old_fd = fd_;
- fd_ = fd;
+ void Reset(int fd = NullFD) noexcept {
+ if (fd == fd_) return;
+ const int old_fd = std::exchange(fd_, fd);
if (old_fd != NullFD) close(old_fd);
}
- void Close() { Reset(); }
+ void Close() noexcept { Reset(); }
void Swap(UniqueFD& other) { std::swap(fd_, other.fd_); }
diff --git a/tests/cppunit/unique_fd_test.cc b/tests/cppunit/unique_fd_test.cc
new file mode 100644
index 000000000..63c68c6a4
--- /dev/null
+++ b/tests/cppunit/unique_fd_test.cc
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "common/unique_fd.h"
+
+#include <fcntl.h>
+#include <gtest/gtest.h>
+#include <unistd.h>
+
+#include <cerrno>
+#include <utility>
+
+TEST(UniqueFD, MoveAssignmentClosesPreviousDescriptor) {
+ int first_pipe[2];
+ int second_pipe[2];
+ ASSERT_EQ(pipe(first_pipe), 0);
+ ASSERT_EQ(pipe(second_pipe), 0);
+
+ const int previous_fd = first_pipe[0];
+ UniqueFD target(previous_fd);
+ UniqueFD source(second_pipe[0]);
+ target = std::move(source);
+
+ errno = 0;
+ EXPECT_EQ(fcntl(previous_fd, F_GETFD), -1);
+ EXPECT_EQ(errno, EBADF);
+ EXPECT_EQ(target.Get(), second_pipe[0]);
+ EXPECT_FALSE(source); // NOLINT(bugprone-use-after-move): moved-from state
is under test.
+
+ close(first_pipe[1]);
+ close(second_pipe[1]);
+}
+
+TEST(UniqueFD, SelfMovePreservesDescriptor) {
+ int pipe_fds[2];
+ ASSERT_EQ(pipe(pipe_fds), 0);
+
+ UniqueFD fd(pipe_fds[0]);
+ fd = std::move(fd);
+
+ EXPECT_EQ(fcntl(fd.Get(), F_GETFD), 0);
+ close(pipe_fds[1]);
+}
+
+TEST(UniqueFD, ResetWithSameDescriptorDoesNotCloseIt) {
+ int pipe_fds[2];
+ ASSERT_EQ(pipe(pipe_fds), 0);
+
+ UniqueFD fd(pipe_fds[0]);
+ fd.Reset(fd.Get());
+
+ EXPECT_EQ(fcntl(fd.Get(), F_GETFD), 0);
+ close(pipe_fds[1]);
+}
+
+TEST(UniqueFD, ReleaseTransfersOwnership) {
+ int pipe_fds[2];
+ ASSERT_EQ(pipe(pipe_fds), 0);
+
+ UniqueFD fd(pipe_fds[0]);
+ const int released_fd = fd.Release();
+
+ EXPECT_FALSE(fd);
+ EXPECT_EQ(fcntl(released_fd, F_GETFD), 0);
+ EXPECT_EQ(close(released_fd), 0);
+ close(pipe_fds[1]);
+}
