From 321359e0cb441c65748427e2ef49b162340bbe04 Mon Sep 17 00:00:00 2001 From: Eric Niebler Date: Thu, 10 Mar 2016 13:25:07 -0800 Subject: [PATCH] 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 --- folly/ScopeGuard.h | 79 +++++++++++++++++++------ folly/detail/UncaughtExceptionCounter.h | 8 +-- folly/test/ScopeGuardTest.cpp | 19 ++++++ 3 files changed, 85 insertions(+), 21 deletions(-) diff --git a/folly/ScopeGuard.h b/folly/ScopeGuard.h index 1fb7769d..3822e4d1 100644 --- a/folly/ScopeGuard.h +++ b/folly/ScopeGuard.h @@ -20,6 +20,8 @@ #include #include #include +#include +#include #include #include @@ -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 + static const T& asConst(const T& t) noexcept { + return t; } bool dismissed_; @@ -88,15 +93,37 @@ class ScopeGuardImplBase { template 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::value) + : ScopeGuardImpl( + asConst(fn), + makeFailsafe(std::is_nothrow_copy_constructible{}, + &fn)) {} + + explicit ScopeGuardImpl(const FunctionType& fn) noexcept( + std::is_nothrow_copy_constructible::value) + : ScopeGuardImpl( + fn, + makeFailsafe(std::is_nothrow_copy_constructible{}, + &fn)) {} + + explicit ScopeGuardImpl(FunctionType&& fn) noexcept( + std::is_nothrow_move_constructible::value) + : ScopeGuardImpl( + std::move_if_noexcept(fn), + makeFailsafe(std::is_nothrow_move_constructible{}, + &fn)) {} + + ScopeGuardImpl(ScopeGuardImpl&& other) noexcept( + std::is_nothrow_move_constructible::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 + static auto makeFailsafe(std::false_type, Fn* fn) noexcept + -> ScopeGuardImpl { + return ScopeGuardImpl{std::ref(*fn)}; + } + + template + explicit ScopeGuardImpl(Fn&& fn, ScopeGuardImplBase&& failsafe) + : ScopeGuardImplBase{}, function_(std::forward(fn)) { + failsafe.dismiss(); + } + + void* operator new(std::size_t) = delete; void execute() noexcept { function_(); } @@ -115,7 +158,9 @@ class ScopeGuardImpl : public ScopeGuardImplBase { template ScopeGuardImpl::type> -makeGuard(FunctionType&& fn) { +makeGuard(FunctionType&& fn) noexcept( + std::is_nothrow_constructible::type, + FunctionType>::value) { return ScopeGuardImpl::type>( std::forward(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_; diff --git a/folly/detail/UncaughtExceptionCounter.h b/folly/detail/UncaughtExceptionCounter.h index c30e865c..7399e18e 100644 --- a/folly/detail/UncaughtExceptionCounter.h +++ b/folly/detail/UncaughtExceptionCounter.h @@ -51,11 +51,11 @@ namespace folly { namespace detail { */ class UncaughtExceptionCounter { public: - UncaughtExceptionCounter() - : exceptionCount_(getUncaughtExceptionCount()) {} + UncaughtExceptionCounter() noexcept + : exceptionCount_(getUncaughtExceptionCount()) {} - UncaughtExceptionCounter(const UncaughtExceptionCounter& other) - : exceptionCount_(other.exceptionCount_) {} + UncaughtExceptionCounter(const UncaughtExceptionCounter& other) noexcept + : exceptionCount_(other.exceptionCount_) {} bool isNewUncaughtException() noexcept { return getUncaughtExceptionCount() > exceptionCount_; diff --git a/folly/test/ScopeGuardTest.cpp b/folly/test/ScopeGuardTest.cpp index 5310eff7..93507da2 100644 --- a/folly/test/ScopeGuardTest.cpp +++ b/folly/test/ScopeGuardTest.cpp @@ -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); +} -- 2.34.1