From d1ddabb086a2a605b6fa4f95d7b3f72b7865d48b Mon Sep 17 00:00:00 2001
From: Nicholas Ormrod <njormrod@fb.com>
Date: Tue, 25 Jul 2017 10:50:36 -0700
Subject: [PATCH] Fix double-free in DynamicConverter

Summary:
If an exception is thrown during the construction of value in a container, then the Transformer iterator will not successfully reconstruct a cache_ object after explicitly calling its destructor (due to the exception being thrown), which results in a double-free when cache_ is destroyed as part of Transformer's destructor.

Conveniently, there exists a piece of code designed to solve just this problem: folly::Optional. Replace cache_ and valid_ with folly::Optional.

This also fixes the unnecessary requirement that container value types have default constructors, since cache_ is no longer default constructed inside of Transformer.

Reviewed By: markisaa

Differential Revision: D5472342

fbshipit-source-id: eade1f7ce260b9b3406d92af8255b5ffa4e4a51c
---
 folly/DynamicConverter.h            | 39 +++++++++++++++--------------
 folly/test/DynamicConverterTest.cpp | 37 +++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 19 deletions(-)

diff --git a/folly/DynamicConverter.h b/folly/DynamicConverter.h
index e73c19c2..f29737a8 100644
--- a/folly/DynamicConverter.h
+++ b/folly/DynamicConverter.h
@@ -25,6 +25,7 @@
 #include <boost/mpl/has_xxx.hpp>
 
 #include <folly/Likely.h>
+#include <folly/Optional.h>
 #include <folly/Traits.h>
 #include <folly/dynamic.h>
 
@@ -102,28 +103,32 @@ namespace dynamicconverter_detail {
 template<typename T>
 struct Dereferencer {
   static inline void derefToCache(
-      T* /* mem */, const dynamic::const_item_iterator& /* it */) {
+      Optional<T>* /* mem */,
+      const dynamic::const_item_iterator& /* it */) {
     throw TypeError("array", dynamic::Type::OBJECT);
   }
 
-  static inline void derefToCache(T* mem, const dynamic::const_iterator& it) {
-    new (mem) T(convertTo<T>(*it));
+  static inline void derefToCache(
+      Optional<T>* mem,
+      const dynamic::const_iterator& it) {
+    mem->emplace(convertTo<T>(*it));
   }
 };
 
 template<typename F, typename S>
 struct Dereferencer<std::pair<F, S>> {
-  static inline void
-  derefToCache(std::pair<F, S>* mem, const dynamic::const_item_iterator& it) {
-    new (mem) std::pair<F, S>(
-        convertTo<F>(it->first), convertTo<S>(it->second)
-    );
+  static inline void derefToCache(
+      Optional<std::pair<F, S>>* mem,
+      const dynamic::const_item_iterator& it) {
+    mem->emplace(convertTo<F>(it->first), convertTo<S>(it->second));
   }
 
   // Intentional duplication of the code in Dereferencer
   template <typename T>
-  static inline void derefToCache(T* mem, const dynamic::const_iterator& it) {
-    new (mem) T(convertTo<T>(*it));
+  static inline void derefToCache(
+      Optional<T>* mem,
+      const dynamic::const_iterator& it) {
+    mem->emplace(convertTo<T>(*it));
   }
 };
 
@@ -135,26 +140,22 @@ class Transformer
 
   typedef typename T::value_type ttype;
 
-  mutable ttype cache_;
-  mutable bool valid_;
+  mutable Optional<ttype> cache_;
 
   void increment() {
     ++this->base_reference();
-    valid_ = false;
+    cache_ = none;
   }
 
   ttype& dereference() const {
-    if (LIKELY(!valid_)) {
-      cache_.~ttype();
+    if (!cache_) {
       Dereferencer<ttype>::derefToCache(&cache_, this->base_reference());
-      valid_ = true;
     }
-    return cache_;
+    return cache_.value();
   }
 
  public:
-  explicit Transformer(const It& it)
-      : Transformer::iterator_adaptor_(it), valid_(false) {}
+  explicit Transformer(const It& it) : Transformer::iterator_adaptor_(it) {}
 };
 
 // conversion factory
diff --git a/folly/test/DynamicConverterTest.cpp b/folly/test/DynamicConverterTest.cpp
index af5cbac8..91c7b6ef 100644
--- a/folly/test/DynamicConverterTest.cpp
+++ b/folly/test/DynamicConverterTest.cpp
@@ -426,3 +426,40 @@ TEST(DynamicConverter, asan_exception_case_uset) {
           dynamic::array(1, dynamic::array(), 3))),
       TypeError);
 }
+
+static int constructB = 0;
+static int destroyB = 0;
+static int ticker = 0;
+struct B {
+  struct BException : std::exception {};
+
+  /* implicit */ B(int x) : x_(x) {
+    if (ticker-- == 0) {
+      throw BException();
+    }
+    constructB++;
+  }
+  B(const B& o) : x_(o.x_) {
+    constructB++;
+  }
+  ~B() {
+    destroyB++;
+  }
+  int x_;
+};
+namespace folly {
+template <>
+struct DynamicConverter<B> {
+  static B convert(const dynamic& d) {
+    return B(convertTo<int>(d));
+  }
+};
+}
+
+TEST(DynamicConverter, double_destroy) {
+  dynamic d = dynamic::array(1, 3, 5, 7, 9, 11, 13, 15, 17);
+  ticker = 3;
+
+  EXPECT_THROW(convertTo<std::vector<B>>(d), B::BException);
+  EXPECT_EQ(constructB, destroyB);
+}
-- 
2.34.1