From 9591971b31713cea01040d9dc417f92584dd4d55 Mon Sep 17 00:00:00 2001 From: Andreas Traczyk Date: Thu, 22 Nov 2018 15:29:45 -0500 Subject: [PATCH] fix deadlock when registering SIP accounts This patch fixes a double lock on the same thread during doRegister whilst protecting configuration data and also implements the same mechanism for ringaccount. Change-Id: I0d5dfd7e6285f85042152092d4ff0d50cd89e5db --- src/ringdht/ringaccount.cpp | 18 ++++++++++------ src/sip/sipaccount.cpp | 43 ++++++++++++++++++------------------- src/sip/sipaccount.h | 13 +++++++++++ 3 files changed, 45 insertions(+), 29 deletions(-) diff --git a/src/ringdht/ringaccount.cpp b/src/ringdht/ringaccount.cpp index f76454388..348cd2c54 100644 --- a/src/ringdht/ringaccount.cpp +++ b/src/ringdht/ringaccount.cpp @@ -1875,6 +1875,7 @@ RingAccount::mapPortUPnP() void RingAccount::doRegister() { + std::unique_lock lock(configurationMutex_); if (not isUsable()) { RING_WARN("Account must be enabled and active to register, ignoring"); return; @@ -1893,17 +1894,20 @@ RingAccount::doRegister() /* if UPnP is enabled, then wait for IGD to complete registration */ if (upnp_) { - auto shared = shared_from_this(); RING_DBG("UPnP: waiting for IGD to register RING account"); + lock.unlock(); setRegistrationState(RegistrationState::TRYING); - std::thread{ [shared] { - auto this_ = std::static_pointer_cast(shared).get(); - if ( not this_->mapPortUPnP()) - RING_WARN("UPnP: Could not successfully map DHT port with UPnP, continuing with account registration anyways."); - this_->doRegister_(); + std::thread{ [w=weak()] { + if (auto acc = w.lock()) { + if (not acc->mapPortUPnP()) + RING_WARN("UPnP: Could not successfully map DHT port with UPnP, continuing with account registration anyways."); + acc->doRegister_(); + } }}.detach(); - } else + } else { + lock.unlock(); doRegister_(); + } } diff --git a/src/sip/sipaccount.cpp b/src/sip/sipaccount.cpp index 0bc395865..f405a8206 100644 --- a/src/sip/sipaccount.cpp +++ b/src/sip/sipaccount.cpp @@ -697,10 +697,9 @@ bool SIPAccount::mapPortUPnP() } } - std::weak_ptr w = std::static_pointer_cast(shared_from_this()); - upnp_->setIGDListener([w] { - if (auto shared = w.lock()) - shared->doRegister(); + upnp_->setIGDListener([w=weak()] { + if (auto acc = w.lock()) + acc->doRegister(); }); return added; @@ -708,8 +707,7 @@ bool SIPAccount::mapPortUPnP() void SIPAccount::doRegister() { - std::lock_guard lock(configurationMutex_); - + std::unique_lock lock(configurationMutex_); if (not isUsable()) { RING_WARN("Account must be enabled and active to register, ignoring"); return; @@ -720,17 +718,20 @@ void SIPAccount::doRegister() /* if UPnP is enabled, then wait for IGD to complete registration */ if (upnp_) { RING_DBG("UPnP: waiting for IGD to register SIP account"); + lock.unlock(); setRegistrationState(RegistrationState::TRYING); - auto shared = shared_from_this(); - std::thread{ [shared] { - sip_utils::register_thread(); - auto this_ = std::static_pointer_cast(shared).get(); - if ( not this_->mapPortUPnP()) - RING_WARN("UPnP: Could not successfully map SIP port with UPnP, continuing with account registration anyways."); - this_->doRegister1_(); + std::thread{ [w=weak()] { + if (auto acc = w.lock()) { + sip_utils::register_thread(); + if (not acc->mapPortUPnP()) + RING_WARN("UPnP: Could not successfully map SIP port with UPnP, continuing with account registration anyways."); + acc->doRegister1_(); + } }}.detach(); - } else + } else { + lock.unlock(); doRegister1_(); + } } void SIPAccount::doRegister1_() @@ -743,12 +744,11 @@ void SIPAccount::doRegister1_() } } - std::weak_ptr weak_acc = std::static_pointer_cast(shared_from_this()); link_->resolveSrvName( hostname_, tlsEnable_ ? PJSIP_TRANSPORT_TLS : PJSIP_TRANSPORT_UDP, - [weak_acc](std::vector host_ips) { - if (auto acc = weak_acc.lock()) { + [w = weak()](std::vector host_ips) { + if (auto acc = w.lock()) { std::lock_guard lock(acc->configurationMutex_); if (host_ips.empty()) { RING_ERR("Can't resolve hostname for registration."); @@ -884,10 +884,9 @@ SIPAccount::connectivityChanged() return; } - auto shared = std::static_pointer_cast(shared_from_this()); - doUnregister([shared](bool /* transport_free */) { - if (shared->isUsable()) - shared->doRegister(); + doUnregister([acc=shared()](bool /* transport_free */) { + if (acc->isUsable()) + acc->doRegister(); }); } @@ -2101,7 +2100,7 @@ SIPAccount::sendTextMessage(const std::string& to, const std::mapacc = std::static_pointer_cast(shared_from_this()); + t->acc = shared(); t->id = id; status = pjsip_endpt_send_request(link_->getEndpoint(), tdata, -1, t, [](void *token, pjsip_event *e) { diff --git a/src/sip/sipaccount.h b/src/sip/sipaccount.h index 7085eb18e..efeab5946 100644 --- a/src/sip/sipaccount.h +++ b/src/sip/sipaccount.h @@ -70,6 +70,19 @@ class SIPAccount : public SIPAccountBase { public: constexpr static const char * const ACCOUNT_TYPE = "SIP"; + std::shared_ptr shared() { + return std::static_pointer_cast(shared_from_this()); + } + std::shared_ptr shared() const { + return std::static_pointer_cast(shared_from_this()); + } + std::weak_ptr weak() { + return std::static_pointer_cast(shared_from_this()); + } + std::weak_ptr weak() const { + return std::static_pointer_cast(shared_from_this()); + } + /** * Constructor * @param accountID The account identifier -- 2.22.0