IndexedMemPool: Fix race condition on size_ that can cause the destructor to access...
authorMaged Michael <magedmichael@fb.com>
Thu, 6 Apr 2017 10:04:20 +0000 (03:04 -0700)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Thu, 6 Apr 2017 10:21:02 +0000 (03:21 -0700)
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

folly/IndexedMemPool.h
folly/test/IndexedMemPoolTest.cpp

index 06cd44a..d9e8493 100644 (file)
@@ -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();
       }
     }
index 1e807d4..daa17e0 100644 (file)
@@ -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());
+}