From: Loic Dachary <loic@dachary.org>
Date: Tue, 27 Aug 2013 14:09:17 +0000 (+0200)
Subject: SharedPtrRegistry: get_next must not delete while holding the lock
X-Git-Tag: v0.69~41^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F545%2Fhead;p=ceph.git

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 6579bd4ba712..90043001ee7f 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 233412a3701b..b1713a9bd9ff 100644
--- 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 {