SharedPtrRegistry: get_next must not delete while holding the lock

    bool get_next(const K &key, pair<K, VPtr> *next)

may indirectly delete the object pointed by next->second when
doing :

    *next = make_pair(i->first, next_val);

and it will deadlock (EDEADLK) when

    void operator()(V *to_remove) {
      {
	Mutex::Locker l(parent->lock);

tries to acquire the lock because it is already held. The
Mutex::Locker is isolated in a block and the *next* parameter is set
outside of the block.

A test case demonstrating the problem is added to test_sharedptr_registry.cc

http://tracker.ceph.com/issues/6117 fixes #6117

Signed-off-by: Loic Dachary <loic@dachary.org>

diff --git a/src/common/sharedptr_registry.hpp b/src/common/sharedptr_registry.hpp
index 6579bd4ba712f9055aed31ca9e8cc927768881e9..90043001ee7f80a0f26b4e1ed43bfaa279d1dcc9 100644 (file)
--- 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 233412a3701b6cb2a4239a19bd694e19f1eae6b0..b1713a9bd9ff65fbfbfc44598ed3343c39757d5a 100644 (file)
--- a/src/test/common/test_sharedptr_registry.cc
+++ b/src/test/common/test_sharedptr_registry.cc
@@ -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 {