fix observed double-deletes of futures interrupt handlers

Summary:
The state machine for the interrupt has a has-handler state which holds the handler pointer and a terminal state which holds nothing. When a handler has been stored and an exception is raised, the state machine exchanges the handler and transitions the state from has-handler to terminal, and then invokes the handler and decrements its refcount, possibly deleting it. A concurrent continuation of a future can load the interrupt and, if it is a handler, increment its refcount.

There is a possible race: the handler is loaded in both threads, its refcount decremented in the first thread and incremeneted in the second thread, and then the first thread observes an historical refcount of zero and deletes the handler. Fix this race by holding onto the handler until the core is destroyed.

Reviewed By: davidtgoldblatt

Differential Revision: D30017199

fbshipit-source-id: b6c474c630719ef8fd1cb88fa982045a523590dd

folly/futures/detail/Core.cpp

@@ -337,15 +337,14 @@ void CoreBase::raise(exception_wrapper e) {
       FOLLY_FALLTHROUGH;
     }
     case InterruptHasHandler: { // invoke the stored handler
+      auto pointer = interrupt & ~InterruptMask;
       auto exchanged = interrupt_.compare_exchange_strong(
-          interrupt, InterruptTerminal, std::memory_order_relaxed);
+          interrupt, pointer | InterruptTerminal, std::memory_order_relaxed);
       if (!exchanged) { // ignore all calls after the first
         return;
       }
-      auto handler =
-          reinterpret_cast<InterruptHandler*>(interrupt & ~InterruptHasHandler);
+      auto handler = reinterpret_cast<InterruptHandler*>(pointer);
       handler->handle(e);
-      handler->release();
       return;
     }
     case InterruptHasObject: // ignore all calls after the first
@@ -360,10 +359,21 @@ void CoreBase::initCopyInterruptHandlerFrom(const CoreBase& other) {
   auto interrupt = other.interrupt_.load(std::memory_order_acquire);
   switch (interrupt & InterruptMask) {
     case InterruptHasHandler: { // copy the handler
-      auto handler =
-          reinterpret_cast<InterruptHandler*>(interrupt & ~InterruptHasHandler);
+      auto pointer = interrupt & ~InterruptMask;
+      auto handler = reinterpret_cast<InterruptHandler*>(pointer);
       handler->acquire();
-      interrupt_.store(interrupt, std::memory_order_release);
+      interrupt_.store(
+          pointer | InterruptHasHandler, std::memory_order_release);
+      break;
+    }
+    case InterruptTerminal: { // copy the handler, if any
+      auto pointer = interrupt & ~InterruptMask;
+      auto handler = reinterpret_cast<InterruptHandler*>(pointer);
+      if (handler) {
+        handler->acquire();
+        interrupt_.store(
+            pointer | InterruptHasHandler, std::memory_order_release);
+      }
       break;
     }
   }
@@ -401,19 +411,25 @@ CoreBase::CoreBase(State state, unsigned char attached)
 
 CoreBase::~CoreBase() {
   auto interrupt = interrupt_.load(std::memory_order_acquire);
+  auto pointer = interrupt & ~InterruptMask;
   switch (interrupt & InterruptMask) {
     case InterruptHasHandler: {
-      auto handler =
-          reinterpret_cast<InterruptHandler*>(interrupt & ~InterruptHasHandler);
+      auto handler = reinterpret_cast<InterruptHandler*>(pointer);
       handler->release();
       break;
     }
     case InterruptHasObject: {
-      auto object =
-          reinterpret_cast<exception_wrapper*>(interrupt & ~InterruptHasObject);
+      auto object = reinterpret_cast<exception_wrapper*>(pointer);
       delete object;
       break;
     }
+    case InterruptTerminal: {
+      auto handler = reinterpret_cast<InterruptHandler*>(pointer);
+      if (handler) {
+        handler->release();
+      }
+      break;
+    }
   }
 }
 

folly/futures/detail/Core.h

@@ -464,8 +464,8 @@ class CoreBase {
         if (!exchanged) {
           terminate_with<std::logic_error>("set-interrupt-handler race");
         }
-        auto object = reinterpret_cast<exception_wrapper*>(
-            interrupt & ~InterruptHasObject);
+        auto pointer = interrupt & ~InterruptMask;
+        auto object = reinterpret_cast<exception_wrapper*>(pointer);
         fn(as_const(*object));
         delete object;
         return;