From eebf86014e66463e9fc4e0887280e06a26720ae8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Tue, 29 Jun 2021 15:37:54 -0400
Subject: [PATCH] sipcall: lock transport while initializing ICE
When starting an outgoing call, if the ICE initialization is too fast:
+ SIPCall::initIceMediaTransport will init the ICE transport
+ onInitDone will be called before tmpMediaTransport_ initialized
+ JamiAccount::SIPStartCall will be called and add ICE candidates
BUT because transportMtx_ is not locked, tmpMediaTransport_ will be null
and no ICE transport will be added.
This is why createUTransport should be done while locked. Anyway the method is
async so should not block.
GitLab: #574
Change-Id: Id88dc92f6d77c18de3ea4614e2b5fd79d3429ff0
---
src/jamidht/jamiaccount.cpp | 18 ++++++++++--------
src/sip/sipcall.cpp | 24 +++++++++++++++++-------
2 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index 345235fe67..3d63c45119 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -755,15 +755,17 @@ JamiAccount::onConnectedOutgoingCall(const std::shared_ptr<SIPCall>& call,
// So, we need to run the call creation in the main thread
// Also, we do not directly call SIPStartCall before receiving onInitDone, because
// there is an inside waitForInitialization that can block the thread.
- runOnMainThread([w = std::move(w), target = std::move(target), call = std::move(call)] {
- auto shared = w.lock();
- if (!shared)
- return;
+ // Note: avoid runMainThread as SIPStartCall use transportMutex
+ dht::ThreadPool::io().run(
+ [w = std::move(w), target = std::move(target), call = std::move(call)] {
+ auto shared = w.lock();
+ if (!shared)
+ return;
- if (not shared->SIPStartCall(*call, target)) {
- JAMI_ERR("Could not send outgoing INVITE request for new call");
- }
- });
+ if (not shared->SIPStartCall(*call, target)) {
+ JAMI_ERR("Could not send outgoing INVITE request for new call");
+ }
+ });
};
call->setIPToIP(true);
call->setPeerNumber(to_id);
diff --git a/src/sip/sipcall.cpp b/src/sip/sipcall.cpp
index c2ab034280..1012db1a88 100644
--- a/src/sip/sipcall.cpp
+++ b/src/sip/sipcall.cpp
@@ -82,7 +82,8 @@ static constexpr std::chrono::milliseconds MS_BETWEEN_2_KEYFRAME_REQUEST {1000};
static constexpr int ICE_COMP_ID_RTP {1};
static constexpr int ICE_COMP_COUNT_PER_STREAM {2};
static constexpr auto MULTISTREAM_REQUIRED_VERSION_STR = "10.0.2"sv;
-static const std::vector<unsigned> MULTISTREAM_REQUIRED_VERSION = split_string_to_unsigned(MULTISTREAM_REQUIRED_VERSION_STR, '.');
+static const std::vector<unsigned> MULTISTREAM_REQUIRED_VERSION
+ = split_string_to_unsigned(MULTISTREAM_REQUIRED_VERSION_STR, '.');
SIPCall::SIPCall(const std::shared_ptr<SIPAccountBase>& account,
const std::string& callId,
@@ -1535,14 +1536,18 @@ SIPCall::setPeerUaVersion(std::string_view ua)
}
if (peerUserAgent_.empty()) {
- JAMI_DBG("[call:%s] Set peer's User-Agent to [%.*s]", getCallId().c_str(), (int)ua.size(), ua.data());
+ JAMI_DBG("[call:%s] Set peer's User-Agent to [%.*s]",
+ getCallId().c_str(),
+ (int) ua.size(),
+ ua.data());
} else if (not peerUserAgent_.empty()) {
// Unlikely, but should be handled since we dont have control over the peer.
// Even if it's unexpected, we still try to parse the UA version.
JAMI_WARN("[call:%s] Peer's User-Agent unexpectedly changed from [%s] to [%.*s]",
getCallId().c_str(),
peerUserAgent_.c_str(),
- (int)ua.size(), ua.data());
+ (int) ua.size(),
+ ua.data());
}
peerUserAgent_ = ua;
@@ -1585,10 +1590,15 @@ SIPCall::setPeerUaVersion(std::string_view ua)
}
// Check if peer's version is at least 10.0.2 to enable multi-stream.
- peerSupportMultiStream_ = Account::meetMinimumRequiredVersion(peerVersion, MULTISTREAM_REQUIRED_VERSION);
+ peerSupportMultiStream_ = Account::meetMinimumRequiredVersion(peerVersion,
+ MULTISTREAM_REQUIRED_VERSION);
if (not peerSupportMultiStream_) {
- JAMI_DBG("Peer's version [%.*s] does not support multi-stream. Min required version: [%.*s]",
- (int)version.size(), version.data(), (int)MULTISTREAM_REQUIRED_VERSION_STR.size(), MULTISTREAM_REQUIRED_VERSION_STR.data());
+ JAMI_DBG(
+ "Peer's version [%.*s] does not support multi-stream. Min required version: [%.*s]",
+ (int) version.size(),
+ version.data(),
+ (int) MULTISTREAM_REQUIRED_VERSION_STR.size(),
+ MULTISTREAM_REQUIRED_VERSION_STR.data());
}
}
@@ -2873,8 +2883,8 @@ SIPCall::initIceMediaTransport(bool master, std::optional<IceTransportOptions> o
// Each RTP stream requires a pair of ICE components (RTP + RTCP).
iceOptions.compCountPerStream = ICE_COMP_COUNT_PER_STREAM;
auto& iceTransportFactory = Manager::instance().getIceTransportFactory();
- auto transport = iceTransportFactory.createUTransport(getCallId().c_str(), iceOptions);
std::lock_guard<std::mutex> lk(transportMtx_);
+ auto transport = iceTransportFactory.createUTransport(getCallId().c_str(), iceOptions);
// Destroy old ice on a separate io pool
if (tmpMediaTransport_)
dht::ThreadPool::io().run([ice = std::make_shared<decltype(tmpMediaTransport_)>(
--
GitLab