Commit 7bd9849f authored by Dan Melnic's avatar Dan Melnic Committed by Facebook Github Bot

Fix threadlocal_detail::ElementWrapper::dispose crash due to array reallocation

Summary: Fix threadlocal_detail::ElementWrapper::dispose crash due array to reallocation

Reviewed By: yfeldblum, djwatson

Differential Revision: D8868394

fbshipit-source-id: dc0c91250f2ffbcba7ac4f0bcf0d048e7785d65f
parent ac6b1e28
...@@ -184,11 +184,15 @@ class ThreadLocalPtr { ...@@ -184,11 +184,15 @@ class ThreadLocalPtr {
void reset(T* newPtr = nullptr) { void reset(T* newPtr = nullptr) {
auto guard = makeGuard([&] { delete newPtr; }); auto guard = makeGuard([&] { delete newPtr; });
threadlocal_detail::ElementWrapper& w = StaticMeta::get(&id_); threadlocal_detail::ElementWrapper* w = &StaticMeta::get(&id_);
w.dispose(TLPDestructionMode::THIS_THREAD); w->dispose(TLPDestructionMode::THIS_THREAD);
// need to get a new ptr since the
// ThreadEntry::elements array can be reallocated
w = &StaticMeta::get(&id_);
w->cleanup();
guard.dismiss(); guard.dismiss();
w.set(newPtr); w->set(newPtr);
} }
explicit operator bool() const { explicit operator bool() const {
...@@ -238,10 +242,14 @@ class ThreadLocalPtr { ...@@ -238,10 +242,14 @@ class ThreadLocalPtr {
deleter(newPtr, TLPDestructionMode::THIS_THREAD); deleter(newPtr, TLPDestructionMode::THIS_THREAD);
} }
}); });
threadlocal_detail::ElementWrapper& w = StaticMeta::get(&id_); threadlocal_detail::ElementWrapper* w = &StaticMeta::get(&id_);
w.dispose(TLPDestructionMode::THIS_THREAD); w->dispose(TLPDestructionMode::THIS_THREAD);
// need to get a new ptr since the
// ThreadEntry::elements array can be reallocated
w = &StaticMeta::get(&id_);
w->cleanup();
guard.dismiss(); guard.dismiss();
w.set(newPtr, deleter); w->set(newPtr, deleter);
} }
// Holds a global lock for iteration through all thread local child objects. // Holds a global lock for iteration through all thread local child objects.
...@@ -456,4 +464,19 @@ class ThreadLocalPtr { ...@@ -456,4 +464,19 @@ class ThreadLocalPtr {
mutable typename StaticMeta::EntryID id_; mutable typename StaticMeta::EntryID id_;
}; };
namespace threadlocal_detail {
template <typename>
struct static_meta_of;
template <typename T, typename Tag, typename AccessMode>
struct static_meta_of<ThreadLocalPtr<T, Tag, AccessMode>> {
using type = StaticMeta<Tag, AccessMode>;
};
template <typename T, typename Tag, typename AccessMode>
struct static_meta_of<ThreadLocal<T, Tag, AccessMode>> {
using type = StaticMeta<Tag, AccessMode>;
};
} // namespace threadlocal_detail
} // namespace folly } // namespace folly
...@@ -139,6 +139,7 @@ void StaticMetaBase::onThreadExit(void* ptr) { ...@@ -139,6 +139,7 @@ void StaticMetaBase::onThreadExit(void* ptr) {
shouldRun = false; shouldRun = false;
FOR_EACH_RANGE (i, 0, threadEntry->elementsCapacity) { FOR_EACH_RANGE (i, 0, threadEntry->elementsCapacity) {
if (threadEntry->elements[i].dispose(TLPDestructionMode::THIS_THREAD)) { if (threadEntry->elements[i].dispose(TLPDestructionMode::THIS_THREAD)) {
threadEntry->elements[i].cleanup();
shouldRun = true; shouldRun = true;
} }
} }
...@@ -170,6 +171,7 @@ void StaticMetaBase::onThreadExit(void* ptr) { ...@@ -170,6 +171,7 @@ void StaticMetaBase::onThreadExit(void* ptr) {
shouldRunInner = false; shouldRunInner = false;
FOR_EACH_RANGE (i, 0, tmp->elementsCapacity) { FOR_EACH_RANGE (i, 0, tmp->elementsCapacity) {
if (tmp->elements[i].dispose(TLPDestructionMode::THIS_THREAD)) { if (tmp->elements[i].dispose(TLPDestructionMode::THIS_THREAD)) {
tmp->elements[i].cleanup();
shouldRunInner = true; shouldRunInner = true;
shouldRunOuter = true; shouldRunOuter = true;
} }
...@@ -202,6 +204,12 @@ void StaticMetaBase::onThreadExit(void* ptr) { ...@@ -202,6 +204,12 @@ void StaticMetaBase::onThreadExit(void* ptr) {
#endif #endif
} }
uint32_t StaticMetaBase::elementsCapacity() const {
ThreadEntry* threadEntry = (*threadEntry_)();
return FOLLY_LIKELY(!!threadEntry) ? threadEntry->elementsCapacity : 0;
}
uint32_t StaticMetaBase::allocate(EntryID* ent) { uint32_t StaticMetaBase::allocate(EntryID* ent) {
uint32_t id; uint32_t id;
auto& meta = *this; auto& meta = *this;
...@@ -285,7 +293,9 @@ void StaticMetaBase::destroy(EntryID* ent) { ...@@ -285,7 +293,9 @@ void StaticMetaBase::destroy(EntryID* ent) {
} }
// Delete elements outside the locks. // Delete elements outside the locks.
for (ElementWrapper& elem : elements) { for (ElementWrapper& elem : elements) {
elem.dispose(TLPDestructionMode::ALL_THREADS); if (elem.dispose(TLPDestructionMode::ALL_THREADS)) {
elem.cleanup();
}
} }
} catch (...) { // Just in case we get a lock error or something anyway... } catch (...) { // Just in case we get a lock error or something anyway...
LOG(WARNING) << "Destructor discarding an exception that was thrown."; LOG(WARNING) << "Destructor discarding an exception that was thrown.";
......
...@@ -125,7 +125,6 @@ struct ElementWrapper { ...@@ -125,7 +125,6 @@ struct ElementWrapper {
DCHECK(deleter1 != nullptr); DCHECK(deleter1 != nullptr);
ownsDeleter ? (*deleter2)(ptr, mode) : (*deleter1)(ptr, mode); ownsDeleter ? (*deleter2)(ptr, mode) : (*deleter1)(ptr, mode);
cleanup();
return true; return true;
} }
...@@ -354,6 +353,10 @@ struct StaticMetaBase { ...@@ -354,6 +353,10 @@ struct StaticMetaBase {
static void onThreadExit(void* ptr); static void onThreadExit(void* ptr);
// returns the elementsCapacity for the
// current thread ThreadEntry struct
uint32_t elementsCapacity() const;
uint32_t allocate(EntryID* ent); uint32_t allocate(EntryID* ent);
void destroy(EntryID* ent); void destroy(EntryID* ent);
......
...@@ -60,6 +60,23 @@ struct Widget { ...@@ -60,6 +60,23 @@ struct Widget {
}; };
int Widget::totalVal_ = 0; int Widget::totalVal_ = 0;
struct MultiWidget {
int val_{0};
MultiWidget() = default;
~MultiWidget() {
// force a reallocation in the destructor by
// allocating more than elementsCapacity
using TL = ThreadLocal<size_t>;
using TLMeta = threadlocal_detail::static_meta_of<TL>::type;
auto const numElements = TLMeta::instance().elementsCapacity() + 1;
std::vector<ThreadLocal<size_t>> elems(numElements);
for (auto& t : elems) {
*t += 1;
}
}
};
TEST(ThreadLocalPtr, BasicDestructor) { TEST(ThreadLocalPtr, BasicDestructor) {
Widget::totalVal_ = 0; Widget::totalVal_ = 0;
ThreadLocalPtr<Widget> w; ThreadLocalPtr<Widget> w;
...@@ -225,6 +242,12 @@ TEST(ThreadLocal, BasicDestructor) { ...@@ -225,6 +242,12 @@ TEST(ThreadLocal, BasicDestructor) {
EXPECT_EQ(10, Widget::totalVal_); EXPECT_EQ(10, Widget::totalVal_);
} }
// this should force a realloc of the ElementWrapper array
TEST(ThreadLocal, ReallocDestructor) {
ThreadLocal<MultiWidget> w;
std::thread([&w]() { w->val_ += 10; }).join();
}
TEST(ThreadLocal, SimpleRepeatDestructor) { TEST(ThreadLocal, SimpleRepeatDestructor) {
Widget::totalVal_ = 0; Widget::totalVal_ = 0;
{ {
......
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