From: Maged Michael <magedmichael@fb.com>
Date: Thu, 6 Apr 2017 10:04:20 +0000 (-0700)
Subject: IndexedMemPool: Fix race condition on size_ that can cause the destructor to access... 
X-Git-Tag: v2017.04.10.00~12
X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=bba519bfb1b4c4d24fd210696e31fcf5e25d13e8

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
---

diff --git a/folly/IndexedMemPool.h b/folly/IndexedMemPool.h
index 06cd44a3..d9e84935 100644
--- a/folly/IndexedMemPool.h
+++ b/folly/IndexedMemPool.h
@@ -150,7 +150,11 @@ struct IndexedMemPool : boost::noncopyable {
   /// Destroys all of the contained elements
   ~IndexedMemPool() {
     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();
       }
     }
diff --git a/folly/test/IndexedMemPoolTest.cpp b/folly/test/IndexedMemPoolTest.cpp
index 1e807d43..daa17e02 100644
--- a/folly/test/IndexedMemPoolTest.cpp
+++ b/folly/test/IndexedMemPoolTest.cpp
@@ -242,3 +242,52 @@ TEST(IndexedMemPool, no_data_races) {
     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());
+}