Commit 321359e0 authored by Eric Niebler's avatar Eric Niebler Committed by Facebook Github Bot 9

Fix an exception safety hole in ScopeGuard

Summary: Address the exception safety hole described in https://fburl.com/scopeguard-oops. Addnditional noexcept to the places that need it.

Reviewed By: dcolascione

Differential Revision: D3033015

fb-gh-sync-id: 8dfec103bbc86abef425585371994756d3df0a14
shipit-source-id: 8dfec103bbc86abef425585371994756d3df0a14
parent e84e7fad
......@@ -20,6 +20,8 @@
#include <cstddef>
#include <functional>
#include <new>
#include <type_traits>
#include <utility>
#include <folly/Preprocessor.h>
#include <folly/detail/UncaughtExceptionCounter.h>
......@@ -74,12 +76,15 @@ class ScopeGuardImplBase {
}
protected:
ScopeGuardImplBase()
: dismissed_(false) {}
ScopeGuardImplBase() noexcept : dismissed_(false) {}
ScopeGuardImplBase(ScopeGuardImplBase&& other) noexcept
: dismissed_(other.dismissed_) {
other.dismissed_ = true;
static ScopeGuardImplBase makeEmptyScopeGuard() noexcept {
return ScopeGuardImplBase{};
}
template <typename T>
static const T& asConst(const T& t) noexcept {
return t;
}
bool dismissed_;
......@@ -88,15 +93,37 @@ class ScopeGuardImplBase {
template <typename FunctionType>
class ScopeGuardImpl : public ScopeGuardImplBase {
public:
explicit ScopeGuardImpl(const FunctionType& fn)
: function_(fn) {}
explicit ScopeGuardImpl(FunctionType&& fn)
: function_(std::move(fn)) {}
ScopeGuardImpl(ScopeGuardImpl&& other)
: ScopeGuardImplBase(std::move(other))
, function_(std::move(other.function_)) {
explicit ScopeGuardImpl(FunctionType& fn) noexcept(
std::is_nothrow_copy_constructible<FunctionType>::value)
: ScopeGuardImpl(
asConst(fn),
makeFailsafe(std::is_nothrow_copy_constructible<FunctionType>{},
&fn)) {}
explicit ScopeGuardImpl(const FunctionType& fn) noexcept(
std::is_nothrow_copy_constructible<FunctionType>::value)
: ScopeGuardImpl(
fn,
makeFailsafe(std::is_nothrow_copy_constructible<FunctionType>{},
&fn)) {}
explicit ScopeGuardImpl(FunctionType&& fn) noexcept(
std::is_nothrow_move_constructible<FunctionType>::value)
: ScopeGuardImpl(
std::move_if_noexcept(fn),
makeFailsafe(std::is_nothrow_move_constructible<FunctionType>{},
&fn)) {}
ScopeGuardImpl(ScopeGuardImpl&& other) noexcept(
std::is_nothrow_move_constructible<FunctionType>::value)
: function_(std::move_if_noexcept(other.function_)) {
// If the above line attempts a copy and the copy throws, other is
// left owning the cleanup action and will execute it (or not) depending
// on the value of other.dismissed_. The following lines only execute
// if the move/copy succeeded, in which case *this assumes ownership of
// the cleanup action and dismisses other.
dismissed_ = other.dismissed_;
other.dismissed_ = true;
}
~ScopeGuardImpl() noexcept {
......@@ -106,7 +133,23 @@ class ScopeGuardImpl : public ScopeGuardImplBase {
}
private:
void* operator new(size_t) = delete;
static ScopeGuardImplBase makeFailsafe(std::true_type, const void*) noexcept {
return makeEmptyScopeGuard();
}
template <typename Fn>
static auto makeFailsafe(std::false_type, Fn* fn) noexcept
-> ScopeGuardImpl<decltype(std::ref(*fn))> {
return ScopeGuardImpl<decltype(std::ref(*fn))>{std::ref(*fn)};
}
template <typename Fn>
explicit ScopeGuardImpl(Fn&& fn, ScopeGuardImplBase&& failsafe)
: ScopeGuardImplBase{}, function_(std::forward<Fn>(fn)) {
failsafe.dismiss();
}
void* operator new(std::size_t) = delete;
void execute() noexcept { function_(); }
......@@ -115,7 +158,9 @@ class ScopeGuardImpl : public ScopeGuardImplBase {
template <typename FunctionType>
ScopeGuardImpl<typename std::decay<FunctionType>::type>
makeGuard(FunctionType&& fn) {
makeGuard(FunctionType&& fn) noexcept(
std::is_nothrow_constructible<typename std::decay<FunctionType>::type,
FunctionType>::value) {
return ScopeGuardImpl<typename std::decay<FunctionType>::type>(
std::forward<FunctionType>(fn));
}
......@@ -166,7 +211,7 @@ class ScopeGuardForNewException {
private:
ScopeGuardForNewException(const ScopeGuardForNewException& other) = delete;
void* operator new(size_t) = delete;
void* operator new(std::size_t) = delete;
FunctionType function_;
UncaughtExceptionCounter exceptionCounter_;
......
......@@ -51,10 +51,10 @@ namespace folly { namespace detail {
*/
class UncaughtExceptionCounter {
public:
UncaughtExceptionCounter()
UncaughtExceptionCounter() noexcept
: exceptionCount_(getUncaughtExceptionCount()) {}
UncaughtExceptionCounter(const UncaughtExceptionCounter& other)
UncaughtExceptionCounter(const UncaughtExceptionCounter& other) noexcept
: exceptionCount_(other.exceptionCount_) {}
bool isNewUncaughtException() noexcept {
......
......@@ -289,3 +289,22 @@ TEST(ScopeGuard, TEST_SCOPE_SUCCESS_THROW) {
};
EXPECT_THROW(lambda(), std::runtime_error);
}
TEST(ScopeGuard, TEST_THROWING_CLEANUP_ACTION) {
struct ThrowingCleanupAction {
explicit ThrowingCleanupAction(int& scopeExitExecuted)
: scopeExitExecuted_(scopeExitExecuted) {}
ThrowingCleanupAction(const ThrowingCleanupAction& other)
: scopeExitExecuted_(other.scopeExitExecuted_) {
throw std::runtime_error("whoa");
}
void operator()() { ++scopeExitExecuted_; }
private:
int& scopeExitExecuted_;
};
int scopeExitExecuted = 0;
ThrowingCleanupAction onExit(scopeExitExecuted);
EXPECT_THROW(makeGuard(onExit), std::runtime_error);
EXPECT_EQ(scopeExitExecuted, 1);
}
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