Override TemporaryFile's default move constructor

Summary:
A default move constructor/move assignment operator caused a bunch
of bugs in my test that was depending on the move constructor to either
not close the underlying file descriptor or to ensure that the TemporaryFile
object doesn't have a dangling reference to a file descriptor that has already
been closed. The current implementation caused both the moved' from and to
object to share the same underlying file descriptor and when the former object
is destroyed it ends up closing the file descriptor leaving the latter
with a dangling file descriptor which could be reused for some other
socket(s)/file descriptor by the kernel and ends up causing seg-faults
when the latter object is eventually destroyed due to it releasing
a file descriptor that now belongs to some other resource.

I changed the move constructor/move assignment operator
to have the former semantics to not close the underlying file descriptor of
the move'd from object (by releasing it and assigning it to the move'd to
object). I am not sure if anyone would ever want the alternative
semantics because the TemporaryFile object would not be usable
without a valid underlying file descriptor

Reviewed By: yfeldblum

Differential Revision: D6182075

fbshipit-source-id: bc809d704449c1c1182d76cdfa2f7d17b38a719a

diff --git a/folly/experimental/TestUtil.cpp b/folly/experimental/TestUtil.cpp
index f89e1af5d8730c25bd93ec07bb03f9fdc35f7813..f13b9208a4a31e826ac88e34d1056423d9de302a 100644 (file)
--- a/folly/experimental/TestUtil.cpp
+++ b/folly/experimental/TestUtil.cpp
@@ -87,10 +87,10 @@ const fs::path& TemporaryFile::path() const {
   return path_;
 }
 
-TemporaryFile::~TemporaryFile() {
+void TemporaryFile::reset() {
   if (fd_ != -1 && closeOnDestruction_) {
     if (::close(fd_) == -1) {
-      PLOG(ERROR) << "close failed";
+      PLOG(ERROR) << "close failed (fd = " << fd_ << "): ";
     }
   }
 
@@ -105,6 +105,10 @@ TemporaryFile::~TemporaryFile() {
   }
 }
 
+TemporaryFile::~TemporaryFile() {
+  reset();
+}
+
 TemporaryDirectory::TemporaryDirectory(
     StringPiece namePrefix,
     fs::path dir,

diff --git a/folly/experimental/TestUtil.h b/folly/experimental/TestUtil.h
index b9481490df843c8d73bd3493693bce17b8deabcb..0f1ba3ebf2dcb4a22f490137392aaf9a2ed170f4 100644 (file)
--- a/folly/experimental/TestUtil.h
+++ b/folly/experimental/TestUtil.h
@@ -50,19 +50,37 @@ class TemporaryFile {
                          bool closeOnDestruction = true);
   ~TemporaryFile();
 
-  // Movable, but not copiable
-  TemporaryFile(TemporaryFile&&) = default;
-  TemporaryFile& operator=(TemporaryFile&&) = default;
+  // Movable, but not copyable
+  TemporaryFile(TemporaryFile&& other) noexcept {
+    reset();
+    assign(other);
+  }
+
+  TemporaryFile& operator=(TemporaryFile&& other) {
+    if (this != &other) {
+      reset();
+      assign(other);
+    }
+    return *this;
+  }
 
   void close();
   int fd() const { return fd_; }
   const fs::path& path() const;
+  void reset();
 
  private:
   Scope scope_;
   bool closeOnDestruction_;
   int fd_;
   fs::path path_;
+
+  void assign(TemporaryFile& other) {
+    scope_ = other.scope_;
+    closeOnDestruction_ = other.closeOnDestruction_;
+    fd_ = std::exchange(other.fd_, -1);
+    path_ = other.path_;
+  }
 };
 
 /**

diff --git a/folly/experimental/test/TestUtilTest.cpp b/folly/experimental/test/TestUtilTest.cpp
index 72b9ccbf201acfeceedce393f49d61607ddb94b8..27fc13467811d48ed0382b035b8d3fd6b3bfa9ea 100644 (file)
--- a/folly/experimental/test/TestUtilTest.cpp
+++ b/folly/experimental/test/TestUtilTest.cpp
@@ -85,6 +85,35 @@ TEST(TemporaryFile, NoSuchPath) {
                std::system_error);
 }
 
+TEST(TemporaryFile, moveAssignment) {
+  TemporaryFile f;
+  int fd;
+
+  EXPECT_TRUE(f.path().is_absolute());
+  {
+    TemporaryFile g("Foo", ".");
+    EXPECT_NE(g.fd(), -1);
+    fd = g.fd();
+    f = std::move(g);
+  }
+  EXPECT_EQ(fs::path("."), f.path().parent_path());
+  EXPECT_EQ(f.fd(), fd);
+
+  TemporaryFile h = TemporaryFile("FooBar", ".");
+  EXPECT_NE(h.fd(), -1);
+}
+
+TEST(TemporaryFile, moveCtor) {
+  struct FooBar {
+    TemporaryFile f_;
+    explicit FooBar(TemporaryFile&& f) : f_(std::move(f)) {}
+  };
+  TemporaryFile g("Foo");
+  FooBar fb(std::move(g));
+  EXPECT_EQ(g.fd(), -1);
+  EXPECT_NE(fb.f_.fd(), -1);
+}
+
 void testTemporaryDirectory(TemporaryDirectory::Scope scope) {
   fs::path path;
   {