Merge pull request #545 from dachary/wip-6117

SharedPtrRegistry: get_next must not delete while holding the lock

Reviewed-by: Samuel Just <sam.just@inktank.com>

2 files changed, 35 insertions, 12 deletions

diff --git a/src/common/sharedptr_registry.hpp b/src/common/sharedptr_registry.hpp
index 6579bd4ba71..90043001ee7 100644
--- a/src/common/sharedptr_registry.hpp
+++ b/src/common/sharedptr_registry.hpp
@@ -64,16 +64,21 @@ public:
   }
 
   bool get_next(const K &key, pair<K, VPtr> *next) {
-    VPtr next_val;
-    Mutex::Locker l(lock);
-    typename map<K, WeakVPtr>::iterator i = contents.upper_bound(key);
-    while (i != contents.end() &&
-	   !(next_val = i->second.lock()))
-      ++i;
-    if (i == contents.end())
-      return false;
+    pair<K, VPtr> r;
+    {
+      Mutex::Locker l(lock);
+      VPtr next_val;
+      typename map<K, WeakVPtr>::iterator i = contents.upper_bound(key);
+      while (i != contents.end() &&
+	     !(next_val = i->second.lock()))
+	++i;
+      if (i == contents.end())
+	return false;
+      if (next)
+	r = make_pair(i->first, next_val);
+    }
     if (next)
-      *next = make_pair(i->first, next_val);
+      *next = r;
     return true;
   }
 
diff --git a/src/test/common/test_sharedptr_registry.cc b/src/test/common/test_sharedptr_registry.cc
index aec2107c9e5..b1713a9bd9f 100644
--- a/src/test/common/test_sharedptr_registry.cc
+++ b/src/test/common/test_sharedptr_registry.cc
@@ -137,8 +137,8 @@ TEST_F(SharedPtrRegistry_all, wait_lookup_or_create) {
     EXPECT_TRUE(registry.lookup_or_create(key + 12345));
     registry.remove(key);
     ASSERT_TRUE(wait_for(registry, 0));
-    EXPECT_TRUE(t.ptr);
     t.join();
+    EXPECT_TRUE(t.ptr);
   }
   {
     unsigned int key = 2;
@@ -163,9 +163,9 @@ TEST_F(SharedPtrRegistry_all, wait_lookup_or_create) {
     }
     registry.remove(key);
     ASSERT_TRUE(wait_for(registry, 0));
+    t.join();
     EXPECT_TRUE(t.ptr);
     EXPECT_EQ(value, *t.ptr);
-    t.join();
   }
 }
 
@@ -200,8 +200,8 @@ TEST_F(SharedPtrRegistry_all, wait_lookup) {
   EXPECT_FALSE(registry.lookup(key + 12345));
   registry.remove(key);
   ASSERT_TRUE(wait_for(registry, 0));
-  EXPECT_FALSE(t.ptr);
   t.join();
+  EXPECT_FALSE(t.ptr);
 }
 
 TEST_F(SharedPtrRegistry_all, get_next) {
@@ -238,6 +238,24 @@ TEST_F(SharedPtrRegistry_all, get_next) {
 
     EXPECT_FALSE(registry.get_next(i.first, &i));
   }
+  {
+    //
+    // http://tracker.ceph.com/issues/6117
+    // reproduce the issue.
+    //
+    SharedPtrRegistryTest registry;
+    const unsigned int key1 = 111;
+    shared_ptr<int> *ptr1 = new shared_ptr<int>(registry.lookup_or_create(key1));
+    const unsigned int key2 = 222;
+    shared_ptr<int> ptr2 = registry.lookup_or_create(key2);
+    
+    pair<unsigned int, shared_ptr<int> > i;
+    EXPECT_TRUE(registry.get_next(i.first, &i));
+    EXPECT_EQ(key1, i.first);
+    delete ptr1;
+    EXPECT_TRUE(registry.get_next(i.first, &i));    
+    EXPECT_EQ(key2, i.first);
+  }
 }
 
 class SharedPtrRegistry_destructor : public ::testing::Test {