make msan happy with exception_wrapper

Summary:
When reading a `std::exception_ptr` out of a moved-from `folly::exception_wrapper`, we end up reading a `std::exception_ptr` field of a deleted `ExceptionPtr` object. This was causing MSAN much consternation.

The fix is to test for emptiness in `to_exception_ptr` and return a default-constructed `std::exception_ptr`.

This commit also adds extra test cases, since `to_exception_ptr` does something slightly different depending on whether the `exception_wrapper` object is `const` or not.

Reviewed By: yfeldblum

Differential Revision: D19403528

fbshipit-source-id: fde74a5c4b2ddb2025703432c1ac5a19d26b9426

folly/ExceptionWrapper-inl.h

@@ -425,10 +425,12 @@ inline Ex const* exception_wrapper::get_exception() const noexcept {
   return object;
 }
 
-inline std::exception_ptr const&
-exception_wrapper::to_exception_ptr() noexcept {
-  // Computing an exception_ptr is expensive so cache the result.
-  return (*this = vptr_->get_exception_ptr_(this)).eptr_.ptr_;
+inline std::exception_ptr exception_wrapper::to_exception_ptr() noexcept {
+  if (*this) {
+    // Computing an exception_ptr is expensive so cache the result.
+    return (*this = vptr_->get_exception_ptr_(this)).eptr_.ptr_;
+  }
+  return {};
 }
 inline std::exception_ptr exception_wrapper::to_exception_ptr() const noexcept {
   return vptr_->get_exception_ptr_(this).eptr_.ptr_;

folly/ExceptionWrapper.h

@@ -498,7 +498,7 @@ class exception_wrapper final {
   //! \note The non-const overload of this function mutates `*this` to cache the
   //!     computed `std::exception_ptr`; that is, this function may cause
   //!     `has_exception_ptr()` to change from `false` to `true`.
-  std::exception_ptr const& to_exception_ptr() noexcept;
+  std::exception_ptr to_exception_ptr() noexcept;
   //! \overload
   std::exception_ptr to_exception_ptr() const noexcept;
 

folly/test/ExceptionWrapperTest.cpp

@@ -109,6 +109,8 @@ TEST(ExceptionWrapper, members) {
   EXPECT_FALSE(bool(ew));
   EXPECT_EQ(ew.what(), "");
   EXPECT_EQ(ew.class_name(), "");
+  EXPECT_EQ(nullptr, folly::as_const(ew).to_exception_ptr());
+  EXPECT_EQ(nullptr, ew.to_exception_ptr());
   ew = make_exception_wrapper<std::runtime_error>("payload");
   EXPECT_TRUE(bool(ew));
   EXPECT_EQ(ew.what(), kRuntimeErrorClassName + ": payload");
@@ -236,6 +238,7 @@ TEST(ExceptionWrapper, from_exception_ptr_exn) {
   auto ep = std::make_exception_ptr(std::runtime_error("foo"));
   auto ew = exception_wrapper::from_exception_ptr(ep);
   EXPECT_TRUE(bool(ew));
+  EXPECT_EQ(ep, folly::as_const(ew).to_exception_ptr());
   EXPECT_EQ(ep, ew.to_exception_ptr());
   EXPECT_TRUE(ew.is_compatible_with<std::runtime_error>());
 }
@@ -244,6 +247,7 @@ TEST(ExceptionWrapper, from_exception_ptr_any) {
   auto ep = std::make_exception_ptr<int>(12);
   auto ew = exception_wrapper::from_exception_ptr(ep);
   EXPECT_TRUE(bool(ew));
+  EXPECT_EQ(ep, folly::as_const(ew).to_exception_ptr());
   EXPECT_EQ(ep, ew.to_exception_ptr());
   EXPECT_TRUE(ew.is_compatible_with<int>());
 }
@@ -256,6 +260,8 @@ TEST(ExceptionWrapper, with_exception_ptr_empty) {
   EXPECT_EQ(nullptr, ew.get_exception<std::exception>());
   EXPECT_EQ(nullptr, ew.get_exception<int>());
   EXPECT_FALSE(ew.has_exception_ptr());
+  EXPECT_EQ(nullptr, folly::as_const(ew).to_exception_ptr());
+  EXPECT_FALSE(ew.has_exception_ptr());
   EXPECT_EQ(nullptr, ew.to_exception_ptr());
   EXPECT_FALSE(ew.has_exception_ptr());
   EXPECT_EQ("", ew.class_name());
@@ -274,6 +280,8 @@ TEST(ExceptionWrapper, with_shared_ptr_test) {
   EXPECT_STREQ("foo", ew.get_exception<std::exception>()->what());
   EXPECT_EQ(nullptr, ew.get_exception<int>());
   EXPECT_FALSE(ew.has_exception_ptr());
+  EXPECT_NE(nullptr, folly::as_const(ew).to_exception_ptr());
+  EXPECT_FALSE(ew.has_exception_ptr());
   EXPECT_NE(nullptr, ew.to_exception_ptr());
   EXPECT_TRUE(ew.has_exception_ptr());
   EXPECT_EQ(kRuntimeErrorClassName, ew.class_name());
@@ -289,6 +297,7 @@ TEST(ExceptionWrapper, with_shared_ptr_test) {
   EXPECT_EQ(nullptr, ew.get_exception());
   EXPECT_EQ(nullptr, ew.get_exception<std::exception>());
   EXPECT_EQ(nullptr, ew.get_exception<int>());
+  EXPECT_EQ(nullptr, folly::as_const(ew).to_exception_ptr());
   EXPECT_EQ(nullptr, ew.to_exception_ptr());
   EXPECT_EQ("", ew.class_name());
   EXPECT_EQ("", ew.what());
@@ -322,6 +331,7 @@ TEST(ExceptionWrapper, with_exception_ptr_exn_test) {
   EXPECT_EQ(nullptr, ew.get_exception());
   EXPECT_EQ(nullptr, ew.get_exception<std::exception>());
   EXPECT_EQ(nullptr, ew.get_exception<int>());
+  EXPECT_EQ(nullptr, folly::as_const(ew).to_exception_ptr());
   EXPECT_EQ(nullptr, ew.to_exception_ptr());
   EXPECT_EQ("", ew.class_name());
   EXPECT_EQ("", ew.what());
@@ -339,6 +349,7 @@ TEST(ExceptionWrapper, with_exception_ptr_any_test) {
   EXPECT_NE(nullptr, ew.get_exception<int>());
   EXPECT_EQ(12, *ew.get_exception<int>());
   EXPECT_TRUE(ew.has_exception_ptr());
+  EXPECT_EQ(ep, folly::as_const(ew).to_exception_ptr());
   EXPECT_EQ(ep, ew.to_exception_ptr());
   EXPECT_TRUE(ew.has_exception_ptr());
   EXPECT_EQ(demangle(typeid(int)), ew.class_name());
@@ -353,6 +364,7 @@ TEST(ExceptionWrapper, with_exception_ptr_any_test) {
   EXPECT_EQ(nullptr, ew.get_exception());
   EXPECT_EQ(nullptr, ew.get_exception<std::exception>());
   EXPECT_EQ(nullptr, ew.get_exception<int>());
+  EXPECT_EQ(nullptr, folly::as_const(ew).to_exception_ptr());
   EXPECT_EQ(nullptr, ew.to_exception_ptr());
   EXPECT_FALSE(ew.has_exception_ptr());
   EXPECT_EQ("", ew.class_name());
@@ -372,6 +384,7 @@ TEST(ExceptionWrapper, with_non_std_exception_test) {
   EXPECT_TRUE(ew.has_exception_ptr());
   EXPECT_EQ(demangle(typeid(int)), ew.class_name());
   EXPECT_EQ(demangle(typeid(int)), ew.what());
+  EXPECT_NE(nullptr, folly::as_const(ew).to_exception_ptr());
   EXPECT_NE(nullptr, ew.to_exception_ptr());
   EXPECT_TRUE(ew.has_exception_ptr());
   EXPECT_EQ(demangle(typeid(int)), ew.class_name());
@@ -386,6 +399,7 @@ TEST(ExceptionWrapper, with_non_std_exception_test) {
   EXPECT_EQ(nullptr, ew.get_exception());
   EXPECT_EQ(nullptr, ew.get_exception<std::exception>());
   EXPECT_EQ(nullptr, ew.get_exception<int>());
+  EXPECT_EQ(nullptr, folly::as_const(ew).to_exception_ptr());
   EXPECT_EQ(nullptr, ew.to_exception_ptr());
   EXPECT_FALSE(ew.has_exception_ptr());
   EXPECT_EQ("", ew.class_name());
@@ -403,6 +417,7 @@ TEST(ExceptionWrapper, with_exception_ptr_any_nil_test) {
   EXPECT_EQ(nullptr, ew.get_exception<std::exception>());
   EXPECT_NE(nullptr, ew.get_exception<int>());
   EXPECT_EQ(12, *ew.get_exception<int>());
+  EXPECT_EQ(ep, folly::as_const(ew).to_exception_ptr());
   EXPECT_EQ(ep, ew.to_exception_ptr());
   EXPECT_EQ("<unknown exception>", ew.class_name()); // because concrete type is
   // erased
@@ -417,6 +432,7 @@ TEST(ExceptionWrapper, with_exception_ptr_any_nil_test) {
   EXPECT_EQ(nullptr, ew.get_exception());
   EXPECT_EQ(nullptr, ew.get_exception<std::exception>());
   EXPECT_EQ(nullptr, ew.get_exception<int>());
+  EXPECT_EQ(nullptr, folly::as_const(ew).to_exception_ptr());
   EXPECT_EQ(nullptr, ew.to_exception_ptr());
   EXPECT_EQ("", ew.class_name());
   EXPECT_EQ("", ew.what());