Commit 017cae98 authored by Murali Vilayannur's avatar Murali Vilayannur Committed by Facebook Github Bot

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
parent 1badabb2
......@@ -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,
......
......@@ -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_;
}
};
/**
......
......@@ -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;
{
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment