Commit 33924a4c authored by Lewis Baker's avatar Lewis Baker Committed by Facebook Github Bot

Fix exception-safety bug in folly::Try<T>::operator=()

Summary:
If the value copy/move constructor threw an exception during copy/move assignment then the Try<T> object would not be left in the empty state.

This could lead to double-deletion of resources, or referencing uninitialised memory.

Reviewed By: yfeldblum

Differential Revision: D9425118

fbshipit-source-id: 6639a0903258ecb031c26264f418fa8794519be3
parent 773131b9
......@@ -55,13 +55,16 @@ Try<T>& Try<T>::operator=(Try<T>&& t) noexcept(
return *this;
}
this->~Try();
contains_ = t.contains_;
if (contains_ == Contains::VALUE) {
new (&value_)T(std::move(t.value_));
} else if (contains_ == Contains::EXCEPTION) {
destroy();
if (t.contains_ == Contains::VALUE) {
new (&value_) T(std::move(t.value_));
} else if (t.contains_ == Contains::EXCEPTION) {
new (&e_) exception_wrapper(std::move(t.e_));
}
contains_ = t.contains_;
return *this;
}
......@@ -85,13 +88,21 @@ Try<T>& Try<T>::operator=(const Try<T>& t) noexcept(
static_assert(
std::is_copy_constructible<T>::value,
"T must be copyable for Try<T> to be copyable");
this->~Try();
contains_ = t.contains_;
if (contains_ == Contains::VALUE) {
new (&value_)T(t.value_);
} else if (contains_ == Contains::EXCEPTION) {
if (this == &t) {
return *this;
}
destroy();
if (t.contains_ == Contains::VALUE) {
new (&value_) T(t.value_);
} else if (t.contains_ == Contains::EXCEPTION) {
new (&e_) exception_wrapper(t.e_);
}
contains_ = t.contains_;
return *this;
}
......@@ -140,6 +151,16 @@ void Try<T>::throwIfFailed() const {
}
}
template <class T>
void Try<T>::destroy() noexcept {
auto oldContains = folly::exchange(contains_, Contains::NOTHING);
if (LIKELY(oldContains == Contains::VALUE)) {
value_.~T();
} else if (UNLIKELY(oldContains == Contains::EXCEPTION)) {
e_.~exception_wrapper();
}
}
void Try<void>::throwIfFailed() const {
if (!hasValue_) {
e_.throw_exception();
......
......@@ -330,6 +330,8 @@ class Try {
}
private:
void destroy() noexcept;
Contains contains_;
union {
T value_;
......
......@@ -33,6 +33,7 @@ class A {
int x() const {
return x_;
}
private:
int x_;
};
......@@ -81,6 +82,96 @@ TEST(Try, in_place_nested) {
EXPECT_EQ(5, t_t_a.value().value().x());
}
TEST(Try, assignmentWithThrowingCopyConstructor) {
struct MyException : std::exception {};
struct ThrowingCopyConstructor {
int& counter_;
explicit ThrowingCopyConstructor(int& counter) : counter_(counter) {
++counter_;
}
[[noreturn]] ThrowingCopyConstructor(
const ThrowingCopyConstructor& other) noexcept(false)
: counter_(other.counter_) {
throw MyException{};
}
ThrowingCopyConstructor& operator=(const ThrowingCopyConstructor&) = delete;
~ThrowingCopyConstructor() {
--counter_;
}
};
int counter = 0;
{
Try<ThrowingCopyConstructor> t1{in_place, counter};
Try<ThrowingCopyConstructor> t2{in_place, counter};
EXPECT_EQ(2, counter);
EXPECT_THROW(t2 = t1, MyException);
EXPECT_EQ(1, counter);
EXPECT_FALSE(t2.hasValue());
EXPECT_TRUE(t1.hasValue());
}
EXPECT_EQ(0, counter);
{
Try<ThrowingCopyConstructor> t1{in_place, counter};
Try<ThrowingCopyConstructor> t2;
EXPECT_EQ(1, counter);
EXPECT_THROW(t2 = t1, MyException);
EXPECT_EQ(1, counter);
EXPECT_FALSE(t2.hasValue());
EXPECT_TRUE(t1.hasValue());
}
EXPECT_EQ(0, counter);
}
TEST(Try, assignmentWithThrowingMoveConstructor) {
struct MyException : std::exception {};
struct ThrowingMoveConstructor {
int& counter_;
explicit ThrowingMoveConstructor(int& counter) : counter_(counter) {
++counter_;
}
[[noreturn]] ThrowingMoveConstructor(
ThrowingMoveConstructor&& other) noexcept(false)
: counter_(other.counter_) {
throw MyException{};
}
ThrowingMoveConstructor& operator=(ThrowingMoveConstructor&&) = delete;
~ThrowingMoveConstructor() {
--counter_;
}
};
int counter = 0;
{
Try<ThrowingMoveConstructor> t1{in_place, counter};
Try<ThrowingMoveConstructor> t2{in_place, counter};
EXPECT_EQ(2, counter);
EXPECT_THROW(t2 = std::move(t1), MyException);
EXPECT_EQ(1, counter);
EXPECT_FALSE(t2.hasValue());
EXPECT_TRUE(t1.hasValue());
}
EXPECT_EQ(0, counter);
{
Try<ThrowingMoveConstructor> t1{in_place, counter};
Try<ThrowingMoveConstructor> t2;
EXPECT_EQ(1, counter);
EXPECT_THROW(t2 = std::move(t1), MyException);
EXPECT_EQ(1, counter);
EXPECT_FALSE(t2.hasValue());
EXPECT_TRUE(t1.hasValue());
}
EXPECT_EQ(0, counter);
}
TEST(Try, nothrow) {
using F = HasCtors<false>;
using T = HasCtors<true>;
......@@ -193,9 +284,7 @@ TEST(Try, moveOnly) {
}
TEST(Try, makeTryWith) {
auto func = []() {
return std::make_unique<int>(1);
};
auto func = []() { return std::make_unique<int>(1); };
auto result = makeTryWith(func);
EXPECT_TRUE(result.hasValue());
......@@ -212,18 +301,14 @@ TEST(Try, makeTryWithThrow) {
}
TEST(Try, makeTryWithVoid) {
auto func = []() {
return;
};
auto func = []() { return; };
auto result = makeTryWith(func);
EXPECT_TRUE(result.hasValue());
}
TEST(Try, makeTryWithVoidThrow) {
auto func = []() {
throw std::runtime_error("Runtime");
};
auto func = []() { throw std::runtime_error("Runtime"); };
auto result = makeTryWith(func);
EXPECT_TRUE(result.hasException<std::runtime_error>());
......
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