Commit d1ddabb0 authored by Nicholas Ormrod's avatar Nicholas Ormrod Committed by Facebook Github Bot

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
parent fbfd3029
......@@ -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
......
......@@ -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);
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment