Bug 489661
Summary: | Deadlock between ManagementBroker::userLock and LinkRegistry::lock | ||
---|---|---|---|
Product: | Red Hat Enterprise MRG | Reporter: | Gordon Sim <gsim> |
Component: | qpid-cpp | Assignee: | Gordon Sim <gsim> |
Status: | CLOSED ERRATA | QA Contact: | Frantisek Reznicek <freznice> |
Severity: | high | Docs Contact: | |
Priority: | urgent | ||
Version: | 1.1 | CC: | esammons |
Target Milestone: | 1.1.1 | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-04-21 16:16:38 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Gordon Sim
2009-03-11 09:32:45 UTC
Suggested fix for this specific case is to not hold the LinkRegistry lock across calls to the Link in notifyConnection() and similar calls. E.g.: Index: src/qpid/broker/LinkRegistry.cpp =================================================================== --- src/qpid/broker/LinkRegistry.cpp (revision 752399) +++ src/qpid/broker/LinkRegistry.cpp (working copy) @@ -238,67 +238,70 @@ return store; } -void LinkRegistry::notifyConnection(const std::string& key, Connection* c) +Link::shared_ptr LinkRegistry::findLink(const std::string& key) { Mutex::ScopedLock locker(lock); LinkMap::iterator l = links.find(key); - if (l != links.end()) - { - l->second->established(); - l->second->setConnection(c); - c->setUserId(str(format("%1%@%2%") % l->second->getUsername() % realm)); + if (l != links.end()) return l->second; + else return Link::shared_ptr(); +} + +void LinkRegistry::notifyConnection(const std::string& key, Connection* c) +{ + Link::shared_ptr link = findLink(key); + if (link) { + link->established(); + link->setConnection(c); + c->setUserId(str(format("%1%@%2%") % link->getUsername() % realm)); } } void LinkRegistry::notifyClosed(const std::string& key) { - Mutex::ScopedLock locker(lock); - LinkMap::iterator l = links.find(key); - if (l != links.end()) - l->second->closed(0, "Closed by peer"); + Link::shared_ptr link = findLink(key); + if (link) { + link->closed(0, "Closed by peer"); + } } void LinkRegistry::notifyConnectionForced(const std::string& key, const std::string& text) { - Mutex::ScopedLock locker(lock); - LinkMap::iterator l = links.find(key); - if (l != links.end()) - l->second->notifyConnectionForced(text); + Link::shared_ptr link = findLink(key); + if (link) { + link->notifyConnectionForced(text); + } } std::string LinkRegistry::getAuthMechanism(const std::string& key) { - Mutex::ScopedLock locker(lock); - LinkMap::iterator l = links.find(key); - if (l != links.end()) - return l->second->getAuthMechanism(); + Link::shared_ptr link = findLink(key); + if (link) + return link->getAuthMechanism(); return string("ANONYMOUS"); } std::string LinkRegistry::getAuthCredentials(const std::string& key) { - Mutex::ScopedLock locker(lock); - LinkMap::iterator l = links.find(key); - if (l == links.end()) + Link::shared_ptr link = findLink(key); + if (!link) return string(); string result; result += '\0'; - result += l->second->getUsername(); + result += link->getUsername(); result += '\0'; - result += l->second->getPassword(); + result += link->getPassword(); return result; } std::string LinkRegistry::getAuthIdentity(const std::string& key) { - Mutex::ScopedLock locker(lock); - LinkMap::iterator l = links.find(key); - if (l == links.end()) + Link::shared_ptr link = findLink(key); + if (!link) return string(); - return l->second->getUsername(); + return link->getUsername(); } Index: src/qpid/broker/LinkRegistry.h =================================================================== --- src/qpid/broker/LinkRegistry.h (revision 752399) +++ src/qpid/broker/LinkRegistry.h (working copy) @@ -70,6 +70,7 @@ void periodicMaintenance (); bool updateAddress(const std::string& oldKey, const TcpAddress& newAddress); + Link::shared_ptr findLink(const std::string& key); static std::string createKey(const TcpAddress& address); public: However it looks like there may be other cases, e.g. the Link::closed() method itself holds the Link's lock while raising a management event. The lock order there is the opposite of the lock order that might arise from a management method taking the Link objects lock. Perhaps therefore the following might be a more reliable fix in addition to the above: Index: src/qpid/management/ManagementBroker.cpp =================================================================== --- src/qpid/management/ManagementBroker.cpp (revision 752399) +++ src/qpid/management/ManagementBroker.cpp (working copy) @@ -518,6 +518,7 @@ else try { outBuffer.record(); + Mutex::ScopedUnlock u(userLock); iter->second->doMethod(methodName, inBuffer, outBuffer); } catch(exception& e) { outBuffer.restore(); Fixed by above patch, applied as r752510. Very hard to reproduce; I saw it on one random occasion when running clustered_federation_test but I suspect it could happen under any test creating a link then a bridge. I can't offer any simple reproducer I'm afraid. Bug verified on dev side by code inspection. The issue has been fixed. Validated on RHEL 4.7 / 5.3 using clustered federation_test and code review. ->VERIFIED An advisory has been issued which should help the problem described in this bug report. This report is therefore being closed with a resolution of ERRATA. For more information on therefore solution and/or where to find the updated files, please follow the link below. You may reopen this bug report if the solution does not work for you. http://rhn.redhat.com/errata/RHEA-2009-0434.html |