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

diff --git a/folly/ScopeGuard.h b/folly/ScopeGuard.h
index 1fb7769dd83ef74666c22b53c182fad59807fec4..3822e4d130d777f81edb060d5df583ee4bc21581 100644 (file)
--- a/folly/ScopeGuard.h
+++ b/folly/ScopeGuard.h
@@ -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_;

diff --git a/folly/detail/UncaughtExceptionCounter.h b/folly/detail/UncaughtExceptionCounter.h
index c30e865c5dd793877f6436fafb527f820d2f7a5f..7399e18e1f362f9bb492be3898376c860122e71d 100644 (file)
--- 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 5310eff765af8c001d3b6520b1ceff7982c83f5f..93507da2d55aa5fb9873bbe3853eeacf0699e153 100644 (file)
--- 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);
+}