diff --git a/src/account.cpp b/src/account.cpp
index ee28a9b383b9afac88838ce12380e6648717c688..8eac6fece976eeed4a4c95e5d7e42848a8b406f4 100644
--- a/src/account.cpp
+++ b/src/account.cpp
@@ -155,10 +155,10 @@ Account::enableUpnp(bool state)
{
std::lock_guard<std::mutex> lk {upnp_mtx};
- if (state and !upnp_)
- upnp_.reset(new upnp::Controller());
- else if (!state and upnp_)
- upnp_.reset();
+ if (state and !upnpCtrl_)
+ upnpCtrl_.reset(new upnp::Controller());
+ else if (!state and upnpCtrl_)
+ upnpCtrl_.reset();
}
void
@@ -529,8 +529,8 @@ IpAddr
Account::getUPnPIpAddress() const
{
std::lock_guard<std::mutex> lk(upnp_mtx);
- if (upnp_)
- return upnp_->getExternalIP();
+ if (upnpCtrl_)
+ return upnpCtrl_->getExternalIP();
return {};
}
@@ -542,8 +542,8 @@ bool
Account::getUPnPActive() const
{
std::lock_guard<std::mutex> lk(upnp_mtx);
- if (upnp_)
- return upnp_->hasValidIGD();
+ if (upnpCtrl_)
+ return upnpCtrl_->hasValidIGD();
return false;
}
diff --git a/src/account.h b/src/account.h
index 0fca8e383cc023c4615e38741b2bd104c5260dfd..f11a5007612f49198e3d6279cdbb888ab6387a30 100644
--- a/src/account.h
+++ b/src/account.h
@@ -324,7 +324,7 @@ public:
void addDefaultModerator(const std::string& peerURI);
void removeDefaultModerator(const std::string& peerURI);
- bool isLocalModeratorsEnabled() const { return localModeratorsEnabled_; }
+ bool isLocalModeratorsEnabled() const { return localModeratorsEnabled_; }
void enableLocalModerators(bool isModEnabled) { localModeratorsEnabled_ = isModEnabled; }
public: // virtual methods that has to be implemented by concrete classes
@@ -343,9 +343,9 @@ private:
std::mutex callIDSetMtx_;
std::set<std::string> callIDSet_;
- void enableUpnp(bool state);
-
protected:
+ virtual void enableUpnp(bool state);
+
static void parseString(const std::map<std::string, std::string>& details,
const char* key,
std::string& s);
@@ -515,7 +515,7 @@ protected:
*/
bool upnpEnabled_;
mutable std::mutex upnp_mtx {};
- std::shared_ptr<jami::upnp::Controller> upnp_;
+ std::shared_ptr<jami::upnp::Controller> upnpCtrl_;
std::set<std::string> defaultModerators_ {};
bool localModeratorsEnabled_;
diff --git a/src/ice_transport.cpp b/src/ice_transport.cpp
index 914b756806d07da7d8fb6f5e6d9782fca7a16dd3..146eaceae8bc11f8cb001d973f9a98e65573bcc0 100644
--- a/src/ice_transport.cpp
+++ b/src/ice_transport.cpp
@@ -120,10 +120,16 @@ public:
static const char* getCandidateType(const pj_ice_sess_cand* cand);
bool isTcpEnabled() const { return config_.protocol == PJ_ICE_TP_TCP; }
bool addStunConfig(int af);
- void setupDefaultCandidates();
+ void addDefaultCandidates();
void requestUpnpMappings();
bool hasUpnp() const;
- void setupServerReflexiveCandidates();
+ // Take a list of address pairs (local/public) and add them as
+ // reflexive candidates using STUN config.
+ void addServerReflexiveCandidates(const std::vector<std::pair<IpAddr, IpAddr>>& addrList);
+ // Generate server reflexive candidates using the published (DHT/Account) address
+ const std::vector<std::pair<IpAddr, IpAddr>> setupGenericReflexiveCandidates();
+ // Generate server reflexive candidates using UPNP mappings.
+ const std::vector<std::pair<IpAddr, IpAddr>> setupUpnpReflexiveCandidates();
void setDefaultRemoteAddress(unsigned comp_id, const IpAddr& addr);
const IpAddr& getDefaultRemoteAddress(unsigned comp_id) const;
bool handleEvents(unsigned max_msec);
@@ -337,12 +343,14 @@ IceTransport::Impl::Impl(const char* name,
peerChannels_.resize(component_count_ + 1);
- setupDefaultCandidates();
+ addDefaultCandidates();
- if (upnp_)
- requestUpnpMappings();
+ addServerReflexiveCandidates(setupGenericReflexiveCandidates());
- setupServerReflexiveCandidates();
+ if (upnp_) {
+ requestUpnpMappings();
+ addServerReflexiveCandidates(setupUpnpReflexiveCandidates());
+ }
pool_.reset(
pj_pool_create(iceTransportFactory.getPoolFactory(), "IceTransport.pool", 512, 512, NULL));
@@ -544,9 +552,9 @@ IceTransport::Impl::handleEvents(unsigned max_msec)
void
IceTransport::Impl::onComplete(pj_ice_strans* ice_st, pj_ice_strans_op op, pj_status_t status)
{
- const char* opname = op == PJ_ICE_STRANS_OP_INIT
- ? "initialization"
- : op == PJ_ICE_STRANS_OP_NEGOTIATION ? "negotiation" : "unknown_op";
+ const char* opname = op == PJ_ICE_STRANS_OP_INIT ? "initialization"
+ : op == PJ_ICE_STRANS_OP_NEGOTIATION ? "negotiation"
+ : "unknown_op";
const bool done = status == PJ_SUCCESS;
if (done) {
@@ -742,7 +750,7 @@ IceTransport::Impl::addStunConfig(int af)
stun.af = af;
stun.conn_type = config_.stun.conn_type;
- JAMI_DBG("[ice:%p)] added host stun config for %s transport",
+ JAMI_DBG("[ice:%p] added host stun config for %s transport",
this,
config_.protocol == PJ_ICE_TP_TCP ? "TCP" : "UDP");
@@ -750,7 +758,7 @@ IceTransport::Impl::addStunConfig(int af)
}
void
-IceTransport::Impl::setupDefaultCandidates()
+IceTransport::Impl::addDefaultCandidates()
{
JAMI_DBG("[ice:%p]: Setup default candidates", this);
@@ -790,11 +798,11 @@ IceTransport::Impl::requestUpnpMappings()
std::lock_guard<std::mutex> lock(upnpMappingsMutex_);
auto ret = upnpMappings_.emplace(mapPtr->getMapKey(), *mapPtr);
if (ret.second) {
- JAMI_DBG("[ice:%p]: UPNP mapping %s successfully allocated",
+ JAMI_DBG("[ice:%p] UPNP mapping %s successfully allocated",
this,
mapPtr->toString(true).c_str());
} else {
- JAMI_WARN("[ice:%p]: UPNP mapping %s already in the list!",
+ JAMI_WARN("[ice:%p] UPNP mapping %s already in the list!",
this,
mapPtr->toString().c_str());
}
@@ -812,70 +820,14 @@ IceTransport::Impl::hasUpnp() const
}
void
-IceTransport::Impl::setupServerReflexiveCandidates()
+IceTransport::Impl::addServerReflexiveCandidates(
+ const std::vector<std::pair<IpAddr, IpAddr>>& addrList)
{
- JAMI_DBG("[ice:%p]: Setup server reflexive candidates", this);
-
- std::vector<std::pair<IpAddr, IpAddr>> addrList;
- auto hasUpnpCand = hasUpnp();
- auto isTcp = isTcpEnabled();
-
- // Server reflexive candidates are built using UPNP mappings if enabled and
- // available. Otherwise, the published address will be used instead. This
- // address is given with the other ICE options.
- // For TCP transport, the connection type is set to passive for UPNP
- // candidates and set to active otherwise.
- if (hasUpnpCand) {
- std::lock_guard<std::mutex> lock(upnpMappingsMutex_);
- unsigned compId = 1;
- for (auto const& [_, map] : upnpMappings_) {
- assert(map.getMapKey());
- IpAddr localAddr {map.getInternalAddress()};
- localAddr.setPort(map.getInternalPort());
- IpAddr publicAddr {map.getExternalAddress()};
- publicAddr.setPort(map.getExternalPort());
- addrList.emplace_back(localAddr, publicAddr);
-
- JAMI_DBG("[ice:%p]: Using upnp mapping %s (%s) -> %s (%s) for comp %u",
- this,
- localAddr.toString(true).c_str(),
- localAddr.getFamily() == pj_AF_INET()
- ? "IPv4"
- : localAddr.getFamily() == pj_AF_INET6() ? "IPv6" : "unknown",
- publicAddr.toString(true).c_str(),
- publicAddr.getFamily() == pj_AF_INET()
- ? "IPv4"
- : localAddr.getFamily() == pj_AF_INET6() ? "IPv6" : "unknown",
- compId);
- compId++;
- }
- } else {
- auto localAddr = accountLocalAddr_;
- auto publicAddr = accountPublicAddr_;
- if (localAddr and publicAddr) {
- for (unsigned compIdx = 0; compIdx < component_count_; compIdx++) {
- uint16_t port = 0;
- if (isTcp and not hasUpnpCand)
- port = 9;
- else
- port = upnp::Controller::generateRandomPort(isTcp ? PortType::TCP
- : PortType::UDP);
-
- localAddr.setPort(port);
- publicAddr.setPort(port);
- addrList.emplace_back(localAddr, publicAddr);
-
- JAMI_DBG("[ice:%p]: Using Account address [%s : %s] for comp %u",
- this,
- localAddr.toString(true).c_str(),
- publicAddr.toString(true).c_str(),
- compIdx + 1);
- }
- }
- }
-
if (addrList.size() != component_count_) {
- JAMI_WARN("[ice:%p]: No DHT or UPNP server reflexive candidates added", this);
+ JAMI_WARN("[ice:%p]: Provided addr list size %lu does not match component count %u",
+ this,
+ addrList.size(),
+ component_count_);
return;
}
@@ -889,14 +841,15 @@ IceTransport::Impl::setupServerReflexiveCandidates()
for (unsigned compIdx = 0; compIdx < component_count_; compIdx++) {
auto localAddr = addrList[compIdx].first;
auto publicAddr = addrList[compIdx].second;
+
pj_sockaddr_cp(&stun.cfg.user_mapping[compIdx].local_addr, localAddr.pjPtr());
pj_sockaddr_cp(&stun.cfg.user_mapping[compIdx].mapped_addr, publicAddr.pjPtr());
- if (isTcp) {
- if (hasUpnpCand) {
- stun.cfg.user_mapping[compIdx].tp_type = PJ_CAND_TCP_PASSIVE;
- } else {
+ if (isTcpEnabled()) {
+ if (publicAddr.getPort() == 9) {
stun.cfg.user_mapping[compIdx].tp_type = PJ_CAND_TCP_ACTIVE;
+ } else {
+ stun.cfg.user_mapping[compIdx].tp_type = PJ_CAND_TCP_PASSIVE;
}
} else {
stun.cfg.user_mapping[compIdx].tp_type = PJ_CAND_UDP;
@@ -907,6 +860,81 @@ IceTransport::Impl::setupServerReflexiveCandidates()
assert(stun.cfg.user_mapping_cnt < PJ_ICE_MAX_COMP);
}
+const std::vector<std::pair<IpAddr, IpAddr>>
+IceTransport::Impl::setupGenericReflexiveCandidates()
+{
+ std::vector<std::pair<IpAddr, IpAddr>> addrList;
+ auto isTcp = isTcpEnabled();
+
+ // First, set default server reflexive candidates using current
+ // account public address.
+ // Then, check for UPNP mappings and and them if available.
+ // For TCP transport, the connection type is set to passive for UPNP
+ // candidates and set to active otherwise.
+
+ if (accountLocalAddr_ and accountPublicAddr_) {
+ for (unsigned compIdx = 0; compIdx < component_count_; compIdx++) {
+ // For TCP, the type is set to active, because most likely the incoming
+ // connection will be blocked by the NAT.
+ // For UDP use random port number.
+ uint16_t port = isTcp ? 9
+ : upnp::Controller::generateRandomPort(isTcp ? PortType::TCP
+ : PortType::UDP);
+
+ accountLocalAddr_.setPort(port);
+ accountPublicAddr_.setPort(port);
+ addrList.emplace_back(accountLocalAddr_, accountPublicAddr_);
+
+ JAMI_DBG("[ice:%p]: Add generic local reflexive candidates [%s : %s] for comp %u",
+ this,
+ accountLocalAddr_.toString(true).c_str(),
+ accountPublicAddr_.toString(true).c_str(),
+ compIdx + 1);
+ }
+ }
+
+ return addrList;
+}
+
+const std::vector<std::pair<IpAddr, IpAddr>>
+IceTransport::Impl::setupUpnpReflexiveCandidates()
+{
+ // Add UPNP server reflexive candidates if available.
+ if (not hasUpnp())
+ return {};
+
+ std::lock_guard<std::mutex> lock(upnpMappingsMutex_);
+
+ if (static_cast<unsigned>(upnpMappings_.size()) < component_count_) {
+ JAMI_WARN("[ice:%p]: Not enough mappings %lu. Expected %u",
+ this,
+ upnpMappings_.size(),
+ component_count_);
+ return {};
+ }
+
+ std::vector<std::pair<IpAddr, IpAddr>> addrList;
+
+ unsigned compId = 1;
+ for (auto const& [_, map] : upnpMappings_) {
+ assert(map.getMapKey());
+ IpAddr localAddr {map.getInternalAddress()};
+ localAddr.setPort(map.getInternalPort());
+ IpAddr publicAddr {map.getExternalAddress()};
+ publicAddr.setPort(map.getExternalPort());
+ addrList.emplace_back(localAddr, publicAddr);
+
+ JAMI_DBG("[ice:%p]: Add UPNP local reflexive candidates [%s : %s] for comp %u",
+ this,
+ localAddr.toString(true).c_str(),
+ publicAddr.toString(true).c_str(),
+ compId);
+ compId++;
+ }
+
+ return addrList;
+}
+
void
IceTransport::Impl::setDefaultRemoteAddress(unsigned comp_id, const IpAddr& addr)
{
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index a1357123d370a0b08ee30db43e4076e6a53c055f..a111671c3f1ccc1a956ce59c065bd2c643f6e4d2 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -680,16 +680,9 @@ JamiAccount::onConnectedOutgoingCall(const std::shared_ptr<SIPCall>& call,
const auto localAddress = ip_utils::getInterfaceAddr(shared->getLocalInterface(),
target.getFamily());
- IpAddr addrSdp;
- if (shared->getUPnPActive()) {
- // use UPnP addr, or published addr if its set
- addrSdp = shared->getPublishedSameasLocal() ? shared->getUPnPIpAddress()
- : shared->getPublishedIpAddress();
- } else {
- addrSdp = shared->isStunEnabled() or (not shared->getPublishedSameasLocal())
- ? shared->getPublishedIpAddress()
- : localAddress;
- }
+ IpAddr addrSdp = shared->getPublishedSameasLocal()
+ ? localAddress
+ : shared->getPublishedIpAddress(target.getFamily());
// fallback on local address
if (not addrSdp)
@@ -1804,12 +1797,12 @@ JamiAccount::registerAsyncOps()
loadCachedProxyServer([onLoad](const std::string&) { onLoad(); });
- if (upnp_) {
+ if (upnpCtrl_) {
JAMI_DBG("UPnP: Attempting to map ports for Jami account");
// Release current mapping if any.
if (dhtUpnpMapping_.isValid()) {
- upnp_->releaseMapping(dhtUpnpMapping_);
+ upnpCtrl_->releaseMapping(dhtUpnpMapping_);
}
dhtUpnpMapping_ = upnp::Mapping {0, 0, upnp::PortType::UDP};
@@ -1875,7 +1868,7 @@ JamiAccount::registerAsyncOps()
});
// Request the mapping.
- auto map = upnp_->reserveMapping(dhtUpnpMapping_);
+ auto map = upnpCtrl_->reserveMapping(dhtUpnpMapping_);
// The returned mapping is invalid. Load the account now since
// we may never receive the callback.
if (not map or not map->isValid()) {
@@ -1913,7 +1906,7 @@ JamiAccount::doRegister()
setRegistrationState(RegistrationState::TRYING);
/* if UPnP is enabled, then wait for IGD to complete registration */
- if (upnp_ or proxyServerCached_.empty()) {
+ if (upnpCtrl_ or proxyServerCached_.empty()) {
registerAsyncOps();
} else {
doRegister_();
@@ -1922,6 +1915,35 @@ JamiAccount::doRegister()
cacheTurnServers(); // reset cache for TURN servers
}
+void
+JamiAccount::enableUpnp(bool enable)
+{
+ std::lock_guard<std::mutex> lk {upnp_mtx};
+
+ if (enable) {
+ auto const& addr = getPublishedIpAddress();
+
+ if (upnpCtrl_ and upnpCtrl_->getExternalIP() == addr) {
+ JAMI_DBG(
+ "[Account %s] Already have a valid controller instance with external address [%s]",
+ getAccountID().c_str(),
+ upnpCtrl_->getExternalIP().toString().c_str());
+ return;
+ }
+
+ JAMI_DBG("[Account %s] Creating a new upnp controller instance", getAccountID().c_str());
+ // Need a known IPv4 address to validate the external address
+ // returned by the IGD.
+ if (addr and addr.getFamily() == AF_INET) {
+ upnpCtrl_.reset(new upnp::Controller(addr));
+ } else {
+ upnpCtrl_.reset(new upnp::Controller());
+ }
+ } else {
+ upnpCtrl_.reset();
+ }
+}
+
std::vector<std::string>
JamiAccount::loadBootstrap() const
{
@@ -2564,8 +2586,8 @@ JamiAccount::doUnregister(std::function<void(bool)> released_cb)
dht_->join();
// Release current upnp mapping if any.
- if (upnp_ and dhtUpnpMapping_.isValid()) {
- upnp_->releaseMapping(dhtUpnpMapping_);
+ if (upnpCtrl_ and dhtUpnpMapping_.isValid()) {
+ upnpCtrl_->releaseMapping(dhtUpnpMapping_);
dhtUpnpMapping_ = upnp::Mapping {};
}
@@ -2590,7 +2612,7 @@ JamiAccount::connectivityChanged()
return;
}
// Let UPNP handle connectivity change if enabled.
- if (not upnp_)
+ if (not upnpCtrl_)
dht_->connectivityChanged();
// reset cache
setPublishedAddress({});
@@ -3352,6 +3374,11 @@ JamiAccount::storeActiveIpAddress()
JAMI_DBG("[Account %s] Store DHT public IPv4 address : %s",
getAccountID().c_str(),
publicAddr.toString().c_str());
+
+ // Check if we need to create a new UPNP controller instance.
+ if (upnpEnabled_ and (not upnpCtrl_ or not upnpCtrl_->hasValidIGD())) {
+ enableUpnp(true);
+ }
}
// IPv6
diff --git a/src/jamidht/jamiaccount.h b/src/jamidht/jamiaccount.h
index dfaadb497fbbeb9fdab83a3496bc5628ac1defe5..843e1c9dcee7dad9416531554746192de34d36be 100644
--- a/src/jamidht/jamiaccount.h
+++ b/src/jamidht/jamiaccount.h
@@ -186,6 +186,7 @@ public:
*/
void doUnregister(std::function<void(bool)> cb = {}) override;
+ void enableUpnp(bool enable) override;
/**
* @return pj_str_t "From" uri based on account information.
* From RFC3261: "The To header field first and foremost specifies the desired
@@ -648,8 +649,8 @@ private:
*/
in_port_t dhtPortUsed()
{
- return (upnp_ and dhtUpnpMapping_.isValid()) ? dhtUpnpMapping_.getExternalPort()
- : dhtDefaultPort_;
+ return (upnpCtrl_ and dhtUpnpMapping_.isValid()) ? dhtUpnpMapping_.getExternalPort()
+ : dhtDefaultPort_;
}
/* Current UPNP mapping */
diff --git a/src/sip/sipaccount.cpp b/src/sip/sipaccount.cpp
index 53c9473b8019af977a38aa6fa80b4814396b6463..b5b4beec1da782fe9d267a352d7b3fee61fcc252 100644
--- a/src/sip/sipaccount.cpp
+++ b/src/sip/sipaccount.cpp
@@ -770,7 +770,7 @@ SIPAccount::mapPortUPnP()
}
});
- auto mapRes = upnp_->reserveMapping(map);
+ auto mapRes = upnpCtrl_->reserveMapping(map);
if (mapRes and mapRes->getState() == upnp::MappingState::OPEN) {
return true;
}
@@ -789,7 +789,7 @@ SIPAccount::doRegister()
JAMI_DBG("doRegister %s", hostname_.c_str());
/* if UPnP is enabled, then wait for IGD to complete registration */
- if (upnp_) {
+ if (upnpCtrl_) {
JAMI_DBG("UPnP: waiting for IGD to register SIP account");
setRegistrationState(RegistrationState::TRYING);
if (not mapPortUPnP()) {
diff --git a/src/sip/sipaccountbase.cpp b/src/sip/sipaccountbase.cpp
index aac4c79fb179aa58439138de4327d9f721b6d009..08f08e27c1452f64b2ac09d9e8f581506e7f63a7 100644
--- a/src/sip/sipaccountbase.cpp
+++ b/src/sip/sipaccountbase.cpp
@@ -602,9 +602,17 @@ SIPAccountBase::setMessageDisplayed(const std::string& contactId,
}
IpAddr
-SIPAccountBase::getPublishedIpAddress() const
+SIPAccountBase::getPublishedIpAddress(uint16_t family) const
{
- // Prefere IPv4 if available. More likely to succeed behind NAT.
+ if (family == AF_INET)
+ return publishedIp_[0];
+ if (family == AF_INET6)
+ return publishedIp_[1];
+
+ assert(family == AF_UNSPEC);
+
+ // If family is not set, prefere IPv4 if available. It's more
+ // likely to succeed behind NAT.
if (publishedIp_[0])
return publishedIp_[0];
if (publishedIp_[1])
diff --git a/src/sip/sipaccountbase.h b/src/sip/sipaccountbase.h
index 20c3d9e73dd98b0179d61859be47a0406ce8a510..8472a65aca2d789ae2fc21e897641b5a6151e8a7 100644
--- a/src/sip/sipaccountbase.h
+++ b/src/sip/sipaccountbase.h
@@ -184,7 +184,7 @@ public:
*/
std::string getPublishedAddress() const { return publishedIpAddress_; }
- IpAddr getPublishedIpAddress() const;
+ IpAddr getPublishedIpAddress(uint16_t family = PF_UNSPEC) const;
void setPublishedAddress(const IpAddr& ip_addr);
diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp
index 7c881616240c5f70d184134ab916749ace6ee064..09ade02c30377b293c78457d613b5e207e4811c8 100644
--- a/src/upnp/upnp_context.cpp
+++ b/src/upnp/upnp_context.cpp
@@ -164,6 +164,20 @@ UPnPContext::connectivityChanged()
StopUpnp();
StartUpnp();
+
+ processMappingWithAutoUpdate();
+}
+
+void
+UPnPContext::setPublicAddress(const IpAddr& addr)
+{
+ if (not addr)
+ return;
+
+ JAMI_DBG("Setting the known public address to %s", addr.toString().c_str());
+
+ std::lock_guard<std::mutex> lock(mappingMutex_);
+ knownPublicAddress_ = std::move(addr);
}
bool
@@ -250,7 +264,8 @@ UPnPContext::releaseMapping(const Mapping& map)
auto mapPtr = getMappingWithKey(map.getMapKey());
if (not mapPtr) {
- JAMI_WARN("Trying to release an unknown mapping %s", map.toString().c_str());
+ // Might happen if the mapping failed
+ JAMI_DBG("Mapping %s does not exist or was already removed", map.toString().c_str());
return false;
}
@@ -285,7 +300,6 @@ UPnPContext::registerController(void* controller)
}
JAMI_DBG("Successfully registered controller %p", this);
-
if (not started_)
StartUpnp();
}
@@ -453,7 +467,6 @@ UPnPContext::deleteUnneededMappings(PortType type, int portCount)
// mappings than required. So, all mappings in a state other
// than "OPEN" state (typically in in-progress state) will
// be deleted as well.
- deleteMapping(map);
it = unregisterMapping(it);
} else {
it++;
@@ -773,6 +786,8 @@ UPnPContext::processMappingWithAutoUpdate()
registerMapping(newMapping);
// Release the old one.
map->setAvailable(true);
+ map->enableAutoUpdate(false);
+ map->setNotifyCallback(nullptr);
unregisterMapping(map);
}
}
@@ -789,25 +804,34 @@ UPnPContext::onIgdUpdated(const std::shared_ptr<IGD>& igd, UpnpIgdEvent event)
CHECK_VALID_THREAD();
- auto const& igdAddr = igd->getLocalIp();
+ auto const& igdLocalAddr = igd->getLocalIp();
auto protocolName = igd->getProtocolName();
// Check if IGD has a valid addresses.
- if (not igdAddr) {
+ if (not igdLocalAddr) {
JAMI_ERR("[%s] IGD has an invalid local address", protocolName);
return;
}
+
if (not igd->getPublicIp()) {
- JAMI_ERR("[%s] IGD IGD has an invalid public address", protocolName);
+ JAMI_ERR("[%s] IGD has an invalid public address", protocolName);
return;
}
- char const* IgdState = event == UpnpIgdEvent::ADDED
- ? "ADDED"
- : event == UpnpIgdEvent::REMOVED ? "REMOVED" : "INVALID";
+ if (knownPublicAddress_ and igd->getPublicIp() != knownPublicAddress_) {
+ JAMI_WARN("[%s] IGD external address [%s] does not match known public address [%s]."
+ " The mapped addresses might not be reachable",
+ protocolName,
+ igd->getPublicIp().toString().c_str(),
+ knownPublicAddress_.toString().c_str());
+ }
+
+ char const* IgdState = event == UpnpIgdEvent::ADDED ? "ADDED"
+ : event == UpnpIgdEvent::REMOVED ? "REMOVED"
+ : "INVALID";
JAMI_WARN("State of IGD %s [%s] changed to [%s]",
- igdAddr.toString(true, true).c_str(),
+ igdLocalAddr.toString(true, true).c_str(),
protocolName,
IgdState);
@@ -827,12 +851,12 @@ UPnPContext::onIgdUpdated(const std::shared_ptr<IGD>& igd, UpnpIgdEvent event)
if (ret.second) {
JAMI_DBG("IGD [%s] on address %s was added. Will process any pending requests",
protocolName,
- igdAddr.toString(true, true).c_str());
+ igdLocalAddr.toString(true, true).c_str());
} else {
// Already in the list.
JAMI_ERR("IGD [%s] on address %s already in the list",
protocolName,
- igdAddr.toString(true, true).c_str());
+ igdLocalAddr.toString(true, true).c_str());
return;
}
}
@@ -876,10 +900,10 @@ UPnPContext::onMappingAdded(const std::shared_ptr<IGD>& igd, const Mapping& mapR
if (it == mappingList.end()) {
// We may receive a response for a canceled request. Just
// ignore it.
- JAMI_WARN("Mapping request %s from IGD %s [%s] does not match local request",
- mapRes.toString().c_str(),
- igd->getLocalIp().toString().c_str(),
- mapRes.getProtocolName());
+ JAMI_DBG("Mapping request %s from IGD %s [%s] does not match local request",
+ mapRes.toString().c_str(),
+ igd->getLocalIp().toString().c_str(),
+ mapRes.getProtocolName());
return;
}
key = it->second->getMapKey();
@@ -1078,6 +1102,10 @@ UPnPContext::unregisterMapping(std::map<Mapping::key_t, Mapping::sharedPtr_t>::i
void
UPnPContext::unregisterMapping(const Mapping::sharedPtr_t& map)
{
+ if (map->getAutoUpdate()) {
+ // Dont unregister mappings with auto-update enabled.
+ return;
+ }
auto& mappingList = getMappingList(map->getType());
if (mappingList.erase(map->getMapKey()) == 1) {
diff --git a/src/upnp/upnp_context.h b/src/upnp/upnp_context.h
index 8d98d6cef4384436c9d02ca8e1a827042e527e45..566e87b61da7cf85844b28d52722a6b120d8e037 100644
--- a/src/upnp/upnp_context.h
+++ b/src/upnp/upnp_context.h
@@ -105,6 +105,9 @@ public:
// Retrieve the UPnPContext singleton
static std::shared_ptr<UPnPContext> getUPnPContext();
+ // Set the known public address
+ void setPublicAddress(const IpAddr& addr);
+
// Check if there is a valid IGD in the IGD list.
bool hasValidIGD() const;
@@ -245,6 +248,10 @@ private:
bool started_ {false};
+ // The known public address. The external addresses returned by
+ // the IGDs will be checked against this address.
+ IpAddr knownPublicAddress_ {};
+
// Set of registered controllers
std::set<void*> controllerList_;
diff --git a/src/upnp/upnp_control.cpp b/src/upnp/upnp_control.cpp
index 69829e0b8040b9383a52253b81664197215dc67f..72048aa23dffec5803dcf9d36426acef58f6c51e 100644
--- a/src/upnp/upnp_control.cpp
+++ b/src/upnp/upnp_control.cpp
@@ -38,6 +38,16 @@ Controller::Controller()
JAMI_DBG("Controller@%p: Created UPnP Controller session", this);
}
+Controller::Controller(const IpAddr& addr)
+ : Controller()
+{
+ assert(upnpContext_);
+ assert(addr);
+ assert(addr.getFamily() == AF_INET);
+
+ upnpContext_->setPublicAddress(addr);
+}
+
Controller::~Controller()
{
JAMI_DBG("Controller@%p: Destroying UPnP Controller session", this);
diff --git a/src/upnp/upnp_control.h b/src/upnp/upnp_control.h
index 920feb4ac4725079af0434196c283a3bc4489830..87714f12e8cbf353642d71994dee30811fda191c 100644
--- a/src/upnp/upnp_control.h
+++ b/src/upnp/upnp_control.h
@@ -47,6 +47,7 @@ class Controller
{
public:
Controller();
+ Controller(const IpAddr& addr);
~Controller();
// Checks if a valid IGD is available.