Override TemporaryFile's default move constructor v2017.10.30.00
authorMurali Vilayannur <muralivn@fb.com>
Mon, 30 Oct 2017 05:07:46 +0000 (22:07 -0700)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Mon, 30 Oct 2017 05:20:08 +0000 (22:20 -0700)
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

folly/experimental/TestUtil.cpp
folly/experimental/TestUtil.h
folly/experimental/test/TestUtilTest.cpp

index f89e1af..f13b920 100644 (file)
@@ -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,
index b948149..0f1ba3e 100644 (file)
@@ -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_;
+  }
 };
 
 /**
index 72b9ccb..27fc134 100644 (file)
@@ -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;
   {