Fix lifetime issue in coro::Materialize with active union member

Summary:
`folly::coro::Materialize` uses a `union` of `ManualLifetime` objects to store either the result (value) or the error of an async operation. However, it accesses these fields without ever activating them by in-place constructing the `ManualLifetime` objects. Likewise, when changing the active field, the `ManualLifetime` objects must by destroyed. It doesn't matter that these operations are no-ops. The must be there to avoid UB, which can manifest at higher optimization levels.

I fix this by providing two functions: `activate` and `deactivate`, for use constructing and destructing values in `ManualLifetime` objects when those objects are fields in a `union`.

Reviewed By: yfeldblum, kirkshoop

Differential Revision: D22527895

fbshipit-source-id: 8cc1c3fae75672387d977dddeff61c82093abb9a

folly/experimental/coro/Materialize.h

@@ -48,9 +48,9 @@ class CallbackRecord {
     auto selector =
         std::exchange(that->selector_, CallbackRecordSelector::Invalid);
     if (selector == CallbackRecordSelector::Value) {
-      that->value_.destruct();
+      detail::deactivate(that->value_);
     } else if (selector == CallbackRecordSelector::Error) {
-      that->error_.destruct();
+      detail::deactivate(that->error_);
     }
   }
   template <class OtherReference>
@@ -58,9 +58,9 @@ class CallbackRecord {
       CallbackRecord* that,
       const CallbackRecord<OtherReference>& other) {
     if (other.hasValue()) {
-      that->value_.construct(other.value_.get());
+      detail::activate(that->value_, other.value_.get());
     } else if (other.hasError()) {
-      that->error_.construct(other.error_.get());
+      detail::activate(that->error_, other.error_.get());
     }
     that->selector_ = other.selector_;
   }
@@ -69,9 +69,9 @@ class CallbackRecord {
       CallbackRecord* that,
       CallbackRecord<OtherReference>&& other) {
     if (other.hasValue()) {
-      that->value_.construct(std::move(other.value_).get());
+      detail::activate(that->value_, std::move(other.value_).get());
     } else if (other.hasError()) {
-      that->error_.construct(std::move(other.error_).get());
+      detail::activate(that->error_, std::move(other.error_).get());
     }
     that->selector_ = other.selector_;
   }
@@ -87,7 +87,7 @@ class CallbackRecord {
   CallbackRecord(const std::in_place_index_t<0>&, V&& v) noexcept(
       std::is_nothrow_constructible_v<T, V>)
       : CallbackRecord() {
-    value_.construct(std::forward<V>(v));
+    detail::activate(value_, std::forward<V>(v));
     selector_ = CallbackRecordSelector::Value;
   }
   explicit CallbackRecord(const std::in_place_index_t<1>&) noexcept
@@ -96,7 +96,7 @@ class CallbackRecord {
       const std::in_place_index_t<2>&,
       folly::exception_wrapper e) noexcept
       : CallbackRecord() {
-    error_.construct(std::move(e));
+    detail::activate(error_, std::move(e));
     selector_ = CallbackRecordSelector::Error;
   }
 

folly/experimental/coro/detail/ManualLifetime.h

@@ -17,8 +17,11 @@
 #pragma once
 
 #include <memory>
+#include <new>
 #include <type_traits>
 
+#include <folly/ScopeGuard.h>
+
 namespace folly {
 namespace coro {
 namespace detail {
@@ -40,7 +43,7 @@ class ManualLifetime {
       std::enable_if_t<std::is_constructible<T, Args...>::value, int> = 0>
   void construct(Args&&... args) noexcept(
       noexcept(std::is_nothrow_constructible<T, Args...>::value)) {
-    new (static_cast<void*>(std::addressof(value_)))
+    ::new (static_cast<void*>(std::addressof(value_)))
         T(static_cast<Args&&>(args)...);
   }
 
@@ -121,6 +124,33 @@ class ManualLifetime<void> {
   void get() const noexcept {}
 };
 
+// For use when the ManualLifetime is a member of a union. First,
+// it in-place constructs the ManualLifetime, making it the active
+// member of the union. Then it calls 'construct' on it to construct
+// the value inside it.
+template <
+    typename T,
+    typename... Args,
+    std::enable_if_t<std::is_constructible<T, Args...>::value, int> = 0>
+void activate(ManualLifetime<T>& box, Args&&... args) noexcept(
+    std::is_nothrow_constructible<T, Args...>::value) {
+  auto* p = ::new (&box) ManualLifetime<T>{};
+  // Use ScopeGuard to destruct the ManualLifetime if the 'construct' throws.
+  auto guard = makeGuard([p]() noexcept { p->~ManualLifetime(); });
+  p->construct(static_cast<Args&&>(args)...);
+  guard.dismiss();
+}
+
+// For use when the ManualLifetime is a member of a union. First,
+// it calls 'destruct' on the ManualLifetime to destroy the value
+// inside it. Then it calls the destructor of the ManualLifetime
+// object itself.
+template <typename T>
+void deactivate(ManualLifetime<T>& box) noexcept {
+  box.destruct();
+  box.~ManualLifetime();
+}
+
 } // namespace detail
 } // namespace coro
 } // namespace folly