From: Murali Vilayannur <muralivn@fb.com>
Date: Mon, 30 Oct 2017 05:07:46 +0000 (-0700)
Subject: Override TemporaryFile's default move constructor
X-Git-Tag: v2017.10.30.00
X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=66a77ad13be358dd2a3f50303586b0297d4637ee;ds=sidebyside

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 f89e1af5..f13b9208 100644
--- 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 b9481490..0f1ba3eb 100644
--- 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 72b9ccbf..27fc1346 100644
--- 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;
   {