Commit 9591971b authored by Andreas Traczyk's avatar Andreas Traczyk Committed by Adrien Béraud

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
parent 6b5f2866
...@@ -1875,6 +1875,7 @@ RingAccount::mapPortUPnP() ...@@ -1875,6 +1875,7 @@ RingAccount::mapPortUPnP()
void void
RingAccount::doRegister() RingAccount::doRegister()
{ {
std::unique_lock<std::mutex> lock(configurationMutex_);
if (not isUsable()) { if (not isUsable()) {
RING_WARN("Account must be enabled and active to register, ignoring"); RING_WARN("Account must be enabled and active to register, ignoring");
return; return;
...@@ -1893,17 +1894,20 @@ RingAccount::doRegister() ...@@ -1893,17 +1894,20 @@ RingAccount::doRegister()
/* if UPnP is enabled, then wait for IGD to complete registration */ /* if UPnP is enabled, then wait for IGD to complete registration */
if (upnp_) { if (upnp_) {
auto shared = shared_from_this();
RING_DBG("UPnP: waiting for IGD to register RING account"); RING_DBG("UPnP: waiting for IGD to register RING account");
lock.unlock();
setRegistrationState(RegistrationState::TRYING); setRegistrationState(RegistrationState::TRYING);
std::thread{ [shared] { std::thread{ [w=weak()] {
auto this_ = std::static_pointer_cast<RingAccount>(shared).get(); if (auto acc = w.lock()) {
if ( not this_->mapPortUPnP()) if (not acc->mapPortUPnP())
RING_WARN("UPnP: Could not successfully map DHT port with UPnP, continuing with account registration anyways."); RING_WARN("UPnP: Could not successfully map DHT port with UPnP, continuing with account registration anyways.");
this_->doRegister_(); acc->doRegister_();
}
}}.detach(); }}.detach();
} else } else {
lock.unlock();
doRegister_(); doRegister_();
}
} }
......
...@@ -697,10 +697,9 @@ bool SIPAccount::mapPortUPnP() ...@@ -697,10 +697,9 @@ bool SIPAccount::mapPortUPnP()
} }
} }
std::weak_ptr<SIPAccount> w = std::static_pointer_cast<SIPAccount>(shared_from_this()); upnp_->setIGDListener([w=weak()] {
upnp_->setIGDListener([w] { if (auto acc = w.lock())
if (auto shared = w.lock()) acc->doRegister();
shared->doRegister();
}); });
return added; return added;
...@@ -708,8 +707,7 @@ bool SIPAccount::mapPortUPnP() ...@@ -708,8 +707,7 @@ bool SIPAccount::mapPortUPnP()
void SIPAccount::doRegister() void SIPAccount::doRegister()
{ {
std::lock_guard<std::mutex> lock(configurationMutex_); std::unique_lock<std::mutex> lock(configurationMutex_);
if (not isUsable()) { if (not isUsable()) {
RING_WARN("Account must be enabled and active to register, ignoring"); RING_WARN("Account must be enabled and active to register, ignoring");
return; return;
...@@ -720,17 +718,20 @@ void SIPAccount::doRegister() ...@@ -720,17 +718,20 @@ void SIPAccount::doRegister()
/* if UPnP is enabled, then wait for IGD to complete registration */ /* if UPnP is enabled, then wait for IGD to complete registration */
if (upnp_) { if (upnp_) {
RING_DBG("UPnP: waiting for IGD to register SIP account"); RING_DBG("UPnP: waiting for IGD to register SIP account");
lock.unlock();
setRegistrationState(RegistrationState::TRYING); setRegistrationState(RegistrationState::TRYING);
auto shared = shared_from_this(); std::thread{ [w=weak()] {
std::thread{ [shared] { if (auto acc = w.lock()) {
sip_utils::register_thread(); sip_utils::register_thread();
auto this_ = std::static_pointer_cast<SIPAccount>(shared).get(); if (not acc->mapPortUPnP())
if ( not this_->mapPortUPnP()) RING_WARN("UPnP: Could not successfully map SIP port with UPnP, continuing with account registration anyways.");
RING_WARN("UPnP: Could not successfully map SIP port with UPnP, continuing with account registration anyways."); acc->doRegister1_();
this_->doRegister1_(); }
}}.detach(); }}.detach();
} else } else {
lock.unlock();
doRegister1_(); doRegister1_();
}
} }
void SIPAccount::doRegister1_() void SIPAccount::doRegister1_()
...@@ -743,12 +744,11 @@ void SIPAccount::doRegister1_() ...@@ -743,12 +744,11 @@ void SIPAccount::doRegister1_()
} }
} }
std::weak_ptr<SIPAccount> weak_acc = std::static_pointer_cast<SIPAccount>(shared_from_this());
link_->resolveSrvName( link_->resolveSrvName(
hostname_, hostname_,
tlsEnable_ ? PJSIP_TRANSPORT_TLS : PJSIP_TRANSPORT_UDP, tlsEnable_ ? PJSIP_TRANSPORT_TLS : PJSIP_TRANSPORT_UDP,
[weak_acc](std::vector<IpAddr> host_ips) { [w = weak()](std::vector<IpAddr> host_ips) {
if (auto acc = weak_acc.lock()) { if (auto acc = w.lock()) {
std::lock_guard<std::mutex> lock(acc->configurationMutex_); std::lock_guard<std::mutex> lock(acc->configurationMutex_);
if (host_ips.empty()) { if (host_ips.empty()) {
RING_ERR("Can't resolve hostname for registration."); RING_ERR("Can't resolve hostname for registration.");
...@@ -884,10 +884,9 @@ SIPAccount::connectivityChanged() ...@@ -884,10 +884,9 @@ SIPAccount::connectivityChanged()
return; return;
} }
auto shared = std::static_pointer_cast<SIPAccount>(shared_from_this()); doUnregister([acc=shared()](bool /* transport_free */) {
doUnregister([shared](bool /* transport_free */) { if (acc->isUsable())
if (shared->isUsable()) acc->doRegister();
shared->doRegister();
}); });
} }
...@@ -2101,7 +2100,7 @@ SIPAccount::sendTextMessage(const std::string& to, const std::map<std::string, s ...@@ -2101,7 +2100,7 @@ SIPAccount::sendTextMessage(const std::string& to, const std::map<std::string, s
uint64_t id; uint64_t id;
}; };
ctx* t = new ctx; ctx* t = new ctx;
t->acc = std::static_pointer_cast<SIPAccount>(shared_from_this()); t->acc = shared();
t->id = id; t->id = id;
status = pjsip_endpt_send_request(link_->getEndpoint(), tdata, -1, t, [](void *token, pjsip_event *e) { status = pjsip_endpt_send_request(link_->getEndpoint(), tdata, -1, t, [](void *token, pjsip_event *e) {
......
...@@ -70,6 +70,19 @@ class SIPAccount : public SIPAccountBase { ...@@ -70,6 +70,19 @@ class SIPAccount : public SIPAccountBase {
public: public:
constexpr static const char * const ACCOUNT_TYPE = "SIP"; constexpr static const char * const ACCOUNT_TYPE = "SIP";
std::shared_ptr<SIPAccount> shared() {
return std::static_pointer_cast<SIPAccount>(shared_from_this());
}
std::shared_ptr<SIPAccount const> shared() const {
return std::static_pointer_cast<SIPAccount const>(shared_from_this());
}
std::weak_ptr<SIPAccount> weak() {
return std::static_pointer_cast<SIPAccount>(shared_from_this());
}
std::weak_ptr<SIPAccount const> weak() const {
return std::static_pointer_cast<SIPAccount const>(shared_from_this());
}
/** /**
* Constructor * Constructor
* @param accountID The account identifier * @param accountID The account identifier
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment