diff options
author | Loic Dachary <loic@dachary.org> | 2013-08-27 16:09:17 +0200 |
---|---|---|
committer | Loic Dachary <loic@dachary.org> | 2013-08-27 16:09:17 +0200 |
commit | ea2fc85e091683ced062594ad25fa569e5c1bbd7 (patch) | |
tree | aca4fe2df2652f51f05bee4db6f86bdaa61cc4dd | |
parent | af5281e0f672554a322fef826d2229f563ae8577 (diff) | |
download | ceph-ea2fc85e091683ced062594ad25fa569e5c1bbd7.tar.gz |
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>
-rw-r--r-- | src/common/sharedptr_registry.hpp | 23 | ||||
-rw-r--r-- | src/test/common/test_sharedptr_registry.cc | 18 |
2 files changed, 32 insertions, 9 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 233412a3701..b1713a9bd9f 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 { |