Commit 473115d5 authored by Éloi Bail's avatar Éloi Bail Committed by Guillaume Roguez

ice: fix upnp candidate mapping

Refs #68205

Change-Id: I32270124d4b2d6e407465150a4d53f819e118557
Signed-off-by: Guillaume Roguez's avatarGuillaume Roguez <guillaume.roguez@savoirfairelinux.com>
parent ff3083e7
......@@ -436,7 +436,7 @@ IceTransport::getLocalCandidatesAddr(unsigned comp_id) const
pj_ice_sess_cand cand[PJ_ARRAY_SIZE(cand_)];
unsigned cand_cnt = PJ_ARRAY_SIZE(cand);
if (pj_ice_strans_enum_cands(icest_.get(), comp_id+1, &cand_cnt, cand) != PJ_SUCCESS) {
if (pj_ice_strans_enum_cands(icest_.get(), comp_id, &cand_cnt, cand) != PJ_SUCCESS) {
RING_ERR("pj_ice_strans_enum_cands() failed");
return cand_addrs;
}
......@@ -447,24 +447,25 @@ IceTransport::getLocalCandidatesAddr(unsigned comp_id) const
}
void
IceTransport::addCandidate(int comp_id, const IpAddr& addr)
IceTransport::addCandidate(int comp_id, const IpAddr& localAddr,
const IpAddr& publicAddr)
{
pj_ice_sess_cand cand;
cand.type = PJ_ICE_CAND_TYPE_HOST;
cand.status = PJ_SUCCESS;
cand.comp_id = comp_id + 1; /* starts at 1, not 0 */
cand.type = PJ_ICE_CAND_TYPE_SRFLX;
cand.status = PJ_EPENDING; /* not used */
cand.comp_id = comp_id;
cand.transport_id = 1; /* 1 = STUN */
cand.local_pref = 65535; /* host */
/* cand.foundation = ? */
/* cand.prio = calculated by ice session */
/* make base and addr the same since we're not going through a server */
pj_sockaddr_cp(&cand.base_addr, addr.pjPtr());
pj_sockaddr_cp(&cand.addr, addr.pjPtr());
pj_bzero(&cand.rel_addr, sizeof(cand.rel_addr)); /* not usring rel_addr */
pj_sockaddr_cp(&cand.base_addr, localAddr.pjPtr());
pj_sockaddr_cp(&cand.addr, publicAddr.pjPtr());
pj_sockaddr_cp(&cand.rel_addr, &cand.base_addr);
pj_ice_calc_foundation(pool_.get(), &cand.foundation, cand.type, &cand.base_addr);
pj_ice_sess_add_cand(pj_ice_strans_get_ice_sess(icest_.get()),
auto ret = pj_ice_sess_add_cand(pj_ice_strans_get_ice_sess(icest_.get()),
cand.comp_id,
cand.transport_id,
cand.type,
......@@ -475,6 +476,17 @@ IceTransport::addCandidate(int comp_id, const IpAddr& addr)
&cand.rel_addr,
pj_sockaddr_get_len(&cand.addr),
NULL);
if (ret != PJ_SUCCESS) {
RING_ERR("fail to add candidate for comp_id=%d : %s : %s", comp_id
, localAddr.toString().c_str()
, publicAddr.toString().c_str());
sip_utils::sip_strerror(ret);
} else {
RING_DBG("success to add candidate for comp_id=%d : %s : %s", comp_id
, localAddr.toString().c_str()
, publicAddr.toString().c_str());
}
}
void
......@@ -486,23 +498,28 @@ IceTransport::selectUPnPIceCandidates()
* create a port mapping either with that port, or with an available port
* add candidate with that port and public IP
*/
IpAddr publicIP = upnp_->getExternalIP();
if (publicIP) {
for(unsigned comp_id = 0; comp_id < component_count_; comp_id++) {
RING_DBG("UPnP: Opening port(s) for Ice comp %d and adding candidate with public IP.", comp_id);
std::vector<IpAddr> candidates = getLocalCandidatesAddr(comp_id);
for(IpAddr addr : candidates) {
if (auto publicIP = upnp_->getExternalIP()) {
/* comp_id start at 1 */
for (unsigned comp_id = 1; comp_id <= component_count_; ++comp_id) {
RING_DBG("UPnP: Opening port(s) for ICE comp %d and adding candidate with public IP",
comp_id);
auto candidates = getLocalCandidatesAddr(comp_id);
for (IpAddr addr : candidates) {
auto localIP = upnp_->getLocalIP();
localIP.setPort(addr.getPort());
if (addr != localIP)
continue;
uint16_t port = addr.getPort();
uint16_t port_used;
if (upnp_->addAnyMapping(port, upnp::PortType::UDP, true, &port_used)) {
publicIP.setPort(port_used);
addCandidate(comp_id, publicIP);
addCandidate(comp_id, addr, publicIP);
} else
RING_WARN("UPnP: Could not create a port mapping for the ICE candidae.");
RING_WARN("UPnP: Could not create a port mapping for the ICE candide");
}
}
} else {
RING_WARN("UPnP: Could not determine public IP for ICE candidates.");
RING_WARN("UPnP: Could not determine public IP for ICE candidates");
}
}
}
......
......@@ -239,7 +239,8 @@ class IceTransport {
/**
* Adds candidate to ICE session
*/
void addCandidate(int comp_id, const IpAddr& addr);
void addCandidate(int comp_id, const IpAddr& localAddr,
const IpAddr& publicAddr);
/**
* Creates UPnP port mappings and adds ICE candidates based on those mappings
......
......@@ -220,7 +220,7 @@ UPnPContext::hasValidIGD(std::chrono::seconds timeout)
* assumes you already have a lock on validIGDMutex_
*/
IGD*
UPnPContext::chooseIGD_unlocked()
UPnPContext::chooseIGD_unlocked() const
{
if (validIGDs_.empty())
return nullptr;
......@@ -239,13 +239,7 @@ UPnPContext::addMapping(IGD* igd,
{
*upnp_error = -1;
IpAddr local_ip = ip_utils::getLocalAddr(pj_AF_INET());
if (!local_ip) {
RING_DBG("UPnP: cannot determine local IP");
return {};
}
Mapping mapping{local_ip, port_external, port_internal};
Mapping mapping{port_external, port_internal};
/* check if this mapping already exists
* if the mapping is the same, then we just need to increment the number of users globally
......@@ -440,19 +434,33 @@ UPnPContext::removeMapping(const Mapping& mapping)
}
IpAddr
UPnPContext::getExternalIP()
UPnPContext::getLocalIP() const
{
/* get a lock on the igd list because we don't want the igd to be modified
* or removed from the list while using it */
std::lock_guard<std::mutex> lock(validIGDMutex_);
/* if its a valid igd, we must have already gotten the local ip */
if (auto igd = chooseIGD_unlocked())
return igd->localIp;
RING_WARN("UPnP: no valid IGD available");
return {};
}
IpAddr
UPnPContext::getExternalIP() const
{
/* get a lock on the igd list because we don't want the igd to be modified
* or removed from the list while using it */
std::lock_guard<std::mutex> lock(validIGDMutex_);
IGD* igd = chooseIGD_unlocked();
if (not igd) {
RING_WARN("UPnP: no valid IGD available");
return {};
}
/* if its a valid igd, we must have already gotten the external ip */
return igd->publicIp;
if (auto igd = chooseIGD_unlocked())
return igd->publicIp;
RING_WARN("UPnP: no valid IGD available");
return {};
}
/**
......@@ -598,7 +606,10 @@ UPnPContext::parseIGD(IXML_Document* doc, const Upnp_Discovery* d_event)
new_igd->publicIp = getExternalIP(new_igd.get());
if (new_igd->publicIp) {
RING_DBG("UPnP: got external IP: %s", new_igd->publicIp.toString().c_str());
found_connected_IGD = true;
new_igd->localIp = ip_utils::getLocalAddr(pj_AF_INET());
if (new_igd->localIp)
found_connected_IGD = true;
}
}
}
......@@ -895,15 +906,13 @@ UPnPContext::getExternalIP(const IGD* igd)
void
UPnPContext::removeMappingsByLocalIPAndDescription(const IGD* igd, const std::string& description)
{
/* need to get the local addr */
IpAddr local_ip = ip_utils::getLocalAddr(pj_AF_INET());
if (!local_ip) {
if (!igd->localIp) {
RING_DBG("UPnP: cannot determine local IP in function removeMappingsByLocalIPAndDescription()");
return;
}
RING_DBG("UPnP: removing all port mappings with description: \"%s\" and local ip: %s",
description.c_str(), local_ip.toString().c_str());
description.c_str(), igd->localIp.toString().c_str());
int entry_idx = 0;
bool done = false;
......@@ -936,7 +945,7 @@ UPnPContext::removeMappingsByLocalIPAndDescription(const IGD* igd, const std::st
std::string client_ip = get_first_doc_item(response.get(), "NewInternalClient");
/* check if same IP and description */
if (IpAddr(client_ip) == local_ip and desc_actual.compare(description) == 0) {
if (IpAddr(client_ip) == igd->localIp and desc_actual.compare(description) == 0) {
/* get the rest of the needed parameters */
std::string port_internal = get_first_doc_item(response.get(), "NewInternalPort");
std::string port_external = get_first_doc_item(response.get(), "NewExternalPort");
......@@ -1021,7 +1030,7 @@ UPnPContext::addPortMapping(const IGD* igd, const Mapping& mapping, int* error_c
UpnpAddToAction(&action_ptr, action_name.c_str(), igd->getServiceType().c_str(),
"NewInternalPort", mapping.getPortInternalStr().c_str());
UpnpAddToAction(&action_ptr, action_name.c_str(), igd->getServiceType().c_str(),
"NewInternalClient", mapping.getLocalIp().toString().c_str());
"NewInternalClient", igd->localIp.toString().c_str());
UpnpAddToAction(&action_ptr, action_name.c_str(), igd->getServiceType().c_str(),
"NewEnabled", "1");
UpnpAddToAction(&action_ptr, action_name.c_str(), igd->getServiceType().c_str(),
......
......@@ -100,7 +100,13 @@ public:
/**
* tries to get the external ip of the router
*/
IpAddr getExternalIP();
IpAddr getExternalIP() const;
/**
* get our local ip
*/
IpAddr getLocalIP() const;
/**
* callback function for the UPnP client (control point)
......@@ -170,7 +176,7 @@ private:
* chooses the IGD to use (currently selects the first one in the map)
* assumes you already have a lock on igd_mutex_
*/
IGD* chooseIGD_unlocked();
IGD* chooseIGD_unlocked() const;
/* sends out async search for IGD */
void searchForIGD();
......
......@@ -127,7 +127,17 @@ Controller::removeMappings()
}
IpAddr
Controller::getExternalIP()
Controller::getLocalIP() const
{
#if HAVE_LIBUPNP
if (upnpContext_)
return upnpContext_->getLocalIP();
#endif
return {}; // empty address
}
IpAddr
Controller::getExternalIP() const
{
#if HAVE_LIBUPNP
if (upnpContext_)
......
......@@ -98,7 +98,12 @@ public:
/**
* tries to get the external ip of the IGD (router)
*/
IpAddr getExternalIP();
IpAddr getExternalIP() const;
/**
* tries to get the local ip of the IGD (router)
*/
IpAddr getLocalIP() const;
private:
......
......@@ -34,8 +34,7 @@ namespace ring { namespace upnp {
/* move constructor and operator */
Mapping::Mapping(Mapping&& other)
: local_ip_(std::move(other.local_ip_))
, port_external_(other.port_external_)
: port_external_(other.port_external_)
, port_internal_(other.port_internal_)
, type_(other.type_)
, description_(std::move(other.description_))
......@@ -47,7 +46,6 @@ Mapping::Mapping(Mapping&& other)
Mapping& Mapping::operator=(Mapping&& other)
{
if (this != &other) {
local_ip_ = std::move(other.local_ip_);
port_external_ = other.port_external_;
other.port_external_ = 0;
port_internal_ = other.port_internal_;
......@@ -64,8 +62,7 @@ bool operator== (Mapping &cMap1, Mapping &cMap2)
* mapping; we don't compare the IGD because for now we assume that we always
* use the same one and that all mappings are active
*/
return (cMap1.local_ip_ == cMap2.local_ip_ &&
cMap1.port_external_ == cMap2.port_external_ &&
return (cMap1.port_external_ == cMap2.port_external_ &&
cMap1.port_internal_ == cMap2.port_internal_ &&
cMap1.type_ == cMap2.type_);
}
......@@ -75,4 +72,4 @@ bool operator!= (Mapping &cMap1, Mapping &cMap2)
return !(cMap1 == cMap2);
}
}} // namespace ring::upnp
\ No newline at end of file
}} // namespace ring::upnp
......@@ -52,13 +52,11 @@ public:
constexpr static uint16_t UPNP_PORT_MAX = 65535;
Mapping(
IpAddr local_ip = IpAddr(),
uint16_t port_external = 0,
uint16_t port_internal = 0,
PortType type = PortType::UDP,
std::string description = UPNP_DEFAULT_MAPPING_DESCRIPTION)
: local_ip_(local_ip)
, port_external_(port_external)
: port_external_(port_external)
, port_internal_(port_internal)
, type_(type)
, description_(description)
......@@ -73,7 +71,6 @@ public:
friend bool operator== (Mapping &cRedir1, Mapping &cRedir2);
friend bool operator!= (Mapping &cRedir1, Mapping &cRedir2);
const IpAddr& getLocalIp() const { return local_ip_; };
uint16_t getPortExternal() const { return port_external_; };
std::string getPortExternalStr() const { return std::to_string(port_external_); };
uint16_t getPortInternal() const { return port_internal_; };
......@@ -83,7 +80,7 @@ public:
std::string getDescription() const { return description_; };
std::string toString() const {
return local_ip_.toString() + ", " + getPortExternalStr() + ":" + getPortInternalStr() + ", " + getTypeStr();
return getPortExternalStr() + ":" + getPortInternalStr() + ", " + getTypeStr();
};
bool isValid() const {
......@@ -98,7 +95,6 @@ private:
NON_COPYABLE(Mapping);
protected:
IpAddr local_ip_; /* the destination of the mapping */
uint16_t port_external_;
uint16_t port_internal_;
PortType type_; /* UPD or TCP */
......@@ -118,8 +114,7 @@ public:
* this is only relevant when multiple accounts are using the same SIP port */
unsigned users;
GlobalMapping(const Mapping& mapping, unsigned users = 1)
: Mapping(mapping.getLocalIp()
, mapping.getPortExternal()
: Mapping(mapping.getPortExternal()
, mapping.getPortInternal()
, mapping.getType()
, mapping.getDescription())
......@@ -135,6 +130,9 @@ class PortMapGlobal : public std::map<uint16_t, GlobalMapping> {};
class IGD {
public:
/* device address seen by IGD */
IpAddr localIp;
/* external IP of IGD; can change */
IpAddr publicIp;
......@@ -143,7 +141,7 @@ public:
PortMapGlobal tcpMappings;
/* constructors */
IGD() {};
IGD() {}
IGD(std::string UDN,
std::string baseURL,
std::string friendlyName,
......@@ -158,7 +156,7 @@ public:
, serviceId_(serviceId)
, controlURL_(controlURL)
, eventSubURL_(eventSubURL)
{};
{}
/* move constructor and operator */
IGD(IGD&&) = default;
......@@ -192,4 +190,4 @@ private:
}} // namespace ring::upnp
#endif /* UPNP_IGD_H_ */
\ No newline at end of file
#endif /* UPNP_IGD_H_ */
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