Commit bba519bf authored by Maged Michael's avatar Maged Michael Committed by Facebook Github Bot

IndexedMemPool: Fix race condition on size_ that can cause the destructor to...

IndexedMemPool: Fix race condition on size_ that can cause the destructor to access nonexistent slots

Summary:
Contention on allocation when there is exactly one slot remaining can cause size_ to exceed actualCapacity_.
Without taking the min of size_ and actualCapacity_ the destructor may try to destroy out-of-bound slots.

Added a test to test/IndexedMemPoolTest.cpp that failed before the fix and passes after the fix.

Reviewed By: nbronson

Differential Revision: D4837251

fbshipit-source-id: a887487727f17eaf2ba66345f40fc91d2fe3bc00
parent 674533ad
...@@ -150,7 +150,11 @@ struct IndexedMemPool : boost::noncopyable { ...@@ -150,7 +150,11 @@ struct IndexedMemPool : boost::noncopyable {
/// Destroys all of the contained elements /// Destroys all of the contained elements
~IndexedMemPool() { ~IndexedMemPool() {
if (!eagerRecycle()) { if (!eagerRecycle()) {
for (uint32_t i = size_; i > 0; --i) { // Take the minimum since it is possible that size_ > actualCapacity_.
// This can happen if there are multiple concurrent requests
// when size_ == actualCapacity_ - 1.
uint32_t last = std::min(uint32_t(size_), uint32_t(actualCapacity_));
for (uint32_t i = last; i > 0; --i) {
slots_[i].~Slot(); slots_[i].~Slot();
} }
} }
......
...@@ -242,3 +242,52 @@ TEST(IndexedMemPool, no_data_races) { ...@@ -242,3 +242,52 @@ TEST(IndexedMemPool, no_data_races) {
t.join(); t.join();
} }
} }
std::atomic<int> cnum{0};
std::atomic<int> dnum{0};
TEST(IndexedMemPool, construction_destruction) {
struct Foo {
Foo() {
cnum.fetch_add(1);
}
~Foo() {
dnum.fetch_add(1);
}
};
std::atomic<bool> start{false};
std::atomic<int> started{0};
using Pool = IndexedMemPool<Foo, 1, 1, std::atomic, false, false>;
int nthreads = 20;
int count = 1000;
{
Pool pool(2);
std::vector<std::thread> thr(nthreads);
for (auto i = 0; i < nthreads; ++i) {
thr[i] = std::thread([&]() {
started.fetch_add(1);
while (!start.load())
;
for (auto j = 0; j < count; ++j) {
uint32_t idx = pool.allocIndex();
if (idx != 0) {
pool.recycleIndex(idx);
}
}
});
}
while (started.load() < nthreads)
;
start.store(true);
for (auto& t : thr) {
t.join();
}
}
CHECK_EQ(cnum.load(), dnum.load());
}
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