Skip to content
Snippets Groups Projects
Commit c10eb0bc authored by Stepan Salenikovich's avatar Stepan Salenikovich Committed by Guillaume Roguez
Browse files

upnp: don't try to use invalid device 

If upnp finds a device on the network but it cannot determine
that it is a valid upnp device nor a valid IGD device, do not
try to use it.

Also gets rid of potential memory leak if chooseIGD() crashes.

Refs #64828

Change-Id: Ibe0895ec8de043f7fcd1a1e9aa36e5b766091008
parent 48821f50
Branches
No related tags found
No related merge requests found
Show whitespace changes
Inline Side-by-side
daemon/src/upnp/upnp.cpp
+ 67
− 70
View file @ c10eb0bc
...@@ -56,38 +56,40 @@ namespace ring { namespace upnp { ...@@ -56,38 +56,40 @@ namespace ring { namespace upnp {
#if HAVE_UPNP #if HAVE_UPNP
IGD::IGD(IGDdatas d, UPNPUrls u) static void
: datas_(std::make_shared<const IGDdatas>(d)) resetURLs(UPNPUrls& urls)
{ {
UPNPUrls newURLs = UPNPUrls(u); urls.controlURL = nullptr;
copyURLs(u, &newURLs); urls.ipcondescURL = nullptr;
urls_ = std::make_shared<const UPNPUrls>(newURLs); urls.controlURL_CIF = nullptr;
}; urls.controlURL_6FC = nullptr;
#ifdef MINIUPNPC_VERSION /* if not defined, its version 1.6 */
urls.rootdescURL = nullptr;
#endif
}
IGD::~IGD() /* move constructor */
IGD::IGD(IGD&& other)
: datas_(other.datas_)
, urls_(other.urls_)
{ {
/* if last user, then free the URLs */ resetURLs(other.urls_);
if (urls_.unique() and urls_) {
UPNPUrls urls = UPNPUrls(*urls_.get());
FreeUPNPUrls(&urls);
} }
/* move operator */
IGD& IGD::operator=(IGD&& other)
{
datas_ = other.datas_;
urls_ = other.urls_;
resetURLs(other.urls_);
return *this;
} }
void IGD::~IGD()
IGD::copyURLs(const UPNPUrls &urlsSource, UPNPUrls *urlsDest)
{ {
if (urlsSource.controlURL != nullptr) /* free the URLs */
urlsDest->controlURL = strdup(urlsSource.controlURL); RING_WARN("UPnP : deleting IGD");
if (urlsSource.ipcondescURL != nullptr) FreeUPNPUrls(&urls_);
urlsDest->ipcondescURL = strdup(urlsSource.ipcondescURL);
if (urlsSource.controlURL_CIF != nullptr)
urlsDest->controlURL_CIF = strdup(urlsSource.controlURL_CIF);
if (urlsSource.controlURL_6FC != nullptr)
urlsDest->controlURL_6FC = strdup(urlsSource.controlURL_6FC);
#ifdef MINIUPNPC_VERSION /* if not defined, its version 1.6 */
if (urlsSource.rootdescURL != nullptr)
urlsDest->rootdescURL = strdup(urlsSource.rootdescURL);
#endif
} }
#endif /* HAVE_UPNP */ #endif /* HAVE_UPNP */
...@@ -100,8 +102,8 @@ bool ...@@ -100,8 +102,8 @@ bool
IGD::isEmpty() const IGD::isEmpty() const
{ {
#if HAVE_UPNP #if HAVE_UPNP
if (urls_ and urls_->controlURL != nullptr) { if (urls_.controlURL != nullptr) {
if (urls_->controlURL[0] == '\0') { if (urls_.controlURL[0] == '\0') {
return true; return true;
} else { } else {
return false; return false;
...@@ -228,8 +230,8 @@ Controller::addMapping(uint16_t port_external, uint16_t port_internal, PortType ...@@ -228,8 +230,8 @@ Controller::addMapping(uint16_t port_external, uint16_t port_internal, PortType
/* mapping doesn't exist, so try to add it */ /* mapping doesn't exist, so try to add it */
RING_DBG("UPnP : adding port mapping : %s", mapping.toString().c_str()); RING_DBG("UPnP : adding port mapping : %s", mapping.toString().c_str());
upnp_status = UPNP_AddPortMapping(mapping.igd->getURLs()->controlURL, upnp_status = UPNP_AddPortMapping(mapping.igd->getURLs().controlURL,
mapping.igd->getDatas()->first.servicetype, mapping.igd->getDatas().first.servicetype,
mapping.getExternalPort().c_str(), mapping.getExternalPort().c_str(),
mapping.getInternalPort().c_str(), mapping.getInternalPort().c_str(),
mapping.local_ip.toString().c_str(), mapping.local_ip.toString().c_str(),
...@@ -269,6 +271,11 @@ bool ...@@ -269,6 +271,11 @@ bool
Controller::addAnyMapping(uint16_t port_desired, uint16_t port_local, PortType type, bool use_same_port, bool unique, uint16_t *port_used) Controller::addAnyMapping(uint16_t port_desired, uint16_t port_local, PortType type, bool use_same_port, bool unique, uint16_t *port_used)
{ {
#if HAVE_UPNP #if HAVE_UPNP
if (defaultIGD_->isEmpty()) {
RING_WARN("UPnP : cannot perform command as the IGD has either not been chosen or is not available.");
return false;
}
auto globalMappings = type == PortType::UDP ? (PortMapGlobal*)udpGlobalMappings_.get() : (PortMapGlobal*)tcpGlobalMappings_.get(); auto globalMappings = type == PortType::UDP ? (PortMapGlobal*)udpGlobalMappings_.get() : (PortMapGlobal*)tcpGlobalMappings_.get();
if (unique) { if (unique) {
/* check that port is not already used by the client */ /* check that port is not already used by the client */
...@@ -374,8 +381,8 @@ Controller::removeMappings(PortType type) { ...@@ -374,8 +381,8 @@ Controller::removeMappings(PortType type) {
/* delete the mapping from the router if this instance was the only user */ /* delete the mapping from the router if this instance was the only user */
if (users < 2) { if (users < 2) {
RING_DBG("UPnP : deleting mapping: %s", mapping.toString().c_str()); RING_DBG("UPnP : deleting mapping: %s", mapping.toString().c_str());
int upnp_status = UPNP_DeletePortMapping(mapping.igd->getURLs()->controlURL, int upnp_status = UPNP_DeletePortMapping(mapping.igd->getURLs().controlURL,
mapping.igd->getDatas()->first.servicetype, mapping.igd->getDatas().first.servicetype,
mapping.getExternalPort().c_str(), mapping.getExternalPort().c_str(),
mapping.getType().c_str(), mapping.getType().c_str(),
NULL); NULL);
...@@ -448,8 +455,8 @@ Controller::removeMappingsByLocalIPAndDescription(const std::string& description ...@@ -448,8 +455,8 @@ Controller::removeMappingsByLocalIPAndDescription(const std::string& description
extPort[0] = '\0'; extPort[0] = '\0';
intPort[0] = '\0'; intPort[0] = '\0';
intClient[0] = '\0'; intClient[0] = '\0';
upnp_status = UPNP_GetGenericPortMappingEntry(defaultIGD_->getURLs()->controlURL, upnp_status = UPNP_GetGenericPortMappingEntry(defaultIGD_->getURLs().controlURL,
defaultIGD_->getDatas()->first.servicetype, defaultIGD_->getDatas().first.servicetype,
index, index,
extPort, intClient, intPort, extPort, intClient, intPort,
protocol, desc, enabled, protocol, desc, enabled,
...@@ -463,7 +470,7 @@ Controller::removeMappingsByLocalIPAndDescription(const std::string& description ...@@ -463,7 +470,7 @@ Controller::removeMappingsByLocalIPAndDescription(const std::string& description
RING_DBG("UPnP : found mapping with matching description and ip:\n\t%s %5s->%s:%-5s '%s'", RING_DBG("UPnP : found mapping with matching description and ip:\n\t%s %5s->%s:%-5s '%s'",
protocol, extPort, intClient, intPort, desc); protocol, extPort, intClient, intPort, desc);
int delete_err = 0; int delete_err = 0;
delete_err = UPNP_DeletePortMapping(defaultIGD_->getURLs()->controlURL, defaultIGD_->getDatas()->first.servicetype, extPort, protocol, NULL); delete_err = UPNP_DeletePortMapping(defaultIGD_->getURLs().controlURL, defaultIGD_->getDatas().first.servicetype, extPort, protocol, NULL);
if(delete_err != UPNPCOMMAND_SUCCESS) { if(delete_err != UPNPCOMMAND_SUCCESS) {
RING_DBG("UPnP : UPNP_DeletePortMapping() failed with error code %d : %s", delete_err, strupnperror(delete_err)); RING_DBG("UPnP : UPNP_DeletePortMapping() failed with error code %d : %s", delete_err, strupnperror(delete_err));
} else { } else {
...@@ -504,7 +511,7 @@ Controller::getExternalIP() ...@@ -504,7 +511,7 @@ Controller::getExternalIP()
#if HAVE_UPNP #if HAVE_UPNP
if (defaultIGD_->isEmpty()) { if (defaultIGD_->isEmpty()) {
RING_WARN("UPnP : cannot perform command as the IGD has either not been chosen or is not available."); RING_WARN("UPnP : cannot perform command as the IGD has either not been chosen or is not available.");
return IpAddr(); return {};
} }
int upnp_status; int upnp_status;
...@@ -512,19 +519,19 @@ Controller::getExternalIP() ...@@ -512,19 +519,19 @@ Controller::getExternalIP()
RING_DBG("UPnP : getting external IP"); RING_DBG("UPnP : getting external IP");
upnp_status = UPNP_GetExternalIPAddress(defaultIGD_->getURLs()->controlURL, upnp_status = UPNP_GetExternalIPAddress(defaultIGD_->getURLs().controlURL,
defaultIGD_->getDatas()->first.servicetype, defaultIGD_->getDatas().first.servicetype,
externalIPAddress); externalIPAddress);
if(upnp_status != UPNPCOMMAND_SUCCESS) { if(upnp_status != UPNPCOMMAND_SUCCESS) {
RING_DBG("UPnP : GetExternalIPAddress failed with error code %d : %s", upnp_status, strupnperror(upnp_status)); RING_DBG("UPnP : GetExternalIPAddress failed with error code %d : %s", upnp_status, strupnperror(upnp_status));
return IpAddr(); return {};
} else { } else {
RING_DBG("UPnP : got external IP = %s", externalIPAddress); RING_DBG("UPnP : got external IP = %s", externalIPAddress);
return IpAddr(std::string(externalIPAddress)); return {std::string(externalIPAddress)};
} }
#else #else
/* return empty address */ /* return empty address */
return IpAddr(); return {};
#endif #endif
} }
...@@ -535,73 +542,63 @@ static IGD ...@@ -535,73 +542,63 @@ static IGD
chooseIGD(void) chooseIGD(void)
{ {
#if HAVE_UPNP #if HAVE_UPNP
struct UPNPDev * devlist = nullptr; std::unique_ptr<UPNPDev, decltype(freeUPNPDevlist)&> devlist(nullptr, freeUPNPDevlist);
int upnp_status = 0; int upnp_status = 0;
RING_DBG("UPnP : finding default IGD"); RING_DBG("UPnP : finding default IGD");
/* look for UPnP devices on the network */ /* look for UPnP devices on the network */
devlist = upnpDiscover(2000, NULL, NULL, 0, 0, &upnp_status); devlist.reset(upnpDiscover(2000, NULL, NULL, 0, 0, &upnp_status));
if (devlist) { if (devlist) {
struct UPNPDev * device = nullptr; UPNPDev * device = nullptr;
struct UPNPUrls newIGDURLs = UPNPUrls(); std::unique_ptr<UPNPUrls, decltype(FreeUPNPUrls)&> newIGDURLs(new UPNPUrls, FreeUPNPUrls);
struct IGDdatas newIGDDatas = IGDdatas(); std::unique_ptr<IGDdatas> newIGDDatas(new IGDdatas);
char lanaddr[64]; char lanaddr[64];
newIGDURLs = UPNPUrls();
newIGDDatas = IGDdatas();
RING_DBG("UPnP devices found on the network"); RING_DBG("UPnP devices found on the network");
for(device = devlist; device; device = device->pNext) for(device = devlist.get(); device; device = device->pNext)
{ {
RING_DBG(" desc: %s\n st: %s", RING_DBG(" desc: %s\n st: %s",
device->descURL, device->st); device->descURL, device->st);
} }
upnp_status = UPNP_GetValidIGD(devlist, &newIGDURLs, &newIGDDatas, lanaddr, sizeof(lanaddr)); upnp_status = UPNP_GetValidIGD(devlist.get(), newIGDURLs.get(), newIGDDatas.get(), lanaddr, sizeof(lanaddr));
switch(upnp_status) { switch(upnp_status) {
case -1: case -1:
RING_ERR("UPnP : internal error getting valid IGD"); RING_ERR("UPnP : internal error getting valid IGD");
newIGDURLs = UPNPUrls();
newIGDDatas = IGDdatas();
break; break;
case 0: case 0:
RING_WARN("UPnP : no valid IGD found"); RING_WARN("UPnP : no valid IGD found");
FreeUPNPUrls(&newIGDURLs);
newIGDURLs = UPNPUrls();
newIGDDatas = IGDdatas();
break; break;
case 1: case 1:
RING_DBG("UPnP : found valid IGD : %s", newIGDURLs.controlURL); RING_DBG("UPnP : found valid IGD : %s", newIGDURLs->controlURL);
break; break;
case 2: case 2:
RING_DBG("UPnP : found a (not connected?) IGD, will try to use it anyway: %s", newIGDURLs.controlURL); RING_WARN("UPnP : found an IGD, but it does not seem to be connected : %s", newIGDURLs->controlURL);
break; break;
case 3: case 3:
RING_DBG("UPnP : UPnP device found, cannot determine if it is an IGD, will try to use it anyway : %s", newIGDURLs.controlURL); RING_WARN("UPnP : UPnP device found, but it does not seem to be an IGD : %s", newIGDURLs->controlURL);
break; break;
default: default:
RING_DBG("UPnP : device found, cannot determine if it is a UPnP device, will try to use it anyway : %s", newIGDURLs.controlURL); RING_WARN("UPnP : device found, but cannot determine if it is a UPnP device : %s", newIGDURLs->controlURL);
} }
if (upnp_status > 0) /* only accept IGD if it was determined to be valid */
if (upnp_status == 1) {
RING_DBG("UPnP : local IP address reported as: %s", lanaddr); RING_DBG("UPnP : local IP address reported as: %s", lanaddr);
return {*newIGDDatas.release(), *newIGDURLs.release()};
IGD igd{newIGDDatas, newIGDURLs}; } else {
return {};
freeUPNPDevlist(devlist); }
devlist = nullptr;
return igd;
} else { } else {
RING_WARN("UPnP : looking for IGD UPnP devices on the network failed with error: %d : %s", upnp_status, strupnperror(upnp_status)); RING_WARN("UPnP : looking for IGD UPnP devices on the network failed with error: %d : %s", upnp_status, strupnperror(upnp_status));
return IGD(); return {};
} }
#else #else
return IGD(); return {};
#endif #endif
} }
......
daemon/src/upnp/upnp.h
+ 9
− 9
View file @ c10eb0bc
...@@ -55,16 +55,18 @@ public: ...@@ -55,16 +55,18 @@ public:
#if HAVE_UPNP #if HAVE_UPNP
IGD(IGDdatas d = IGDdatas(), UPNPUrls u = UPNPUrls()); /* constructors */
IGD() : datas_(), urls_() {};
IGD(IGDdatas& d , UPNPUrls& u) : datas_(d), urls_(u) {};
/* move constructor and operator */ /* move constructor and operator */
IGD(IGD&& other) = default; IGD(IGD&& other);
IGD& operator=(IGD&&) = default; IGD& operator=(IGD&& other);
~IGD(); ~IGD();
std::shared_ptr<const IGDdatas> getDatas() const {return datas_;}; const IGDdatas& getDatas() const {return datas_;};
std::shared_ptr<const UPNPUrls> getURLs() const {return urls_;}; const UPNPUrls& getURLs() const {return urls_;};
#else #else
/* use default constructor and destructor */ /* use default constructor and destructor */
#endif #endif
...@@ -74,10 +76,8 @@ private: ...@@ -74,10 +76,8 @@ private:
NON_COPYABLE(IGD); NON_COPYABLE(IGD);
#if HAVE_UPNP #if HAVE_UPNP
void copyURLs(const UPNPUrls &urlsSource, UPNPUrls *urlsDest); IGDdatas datas_;
UPNPUrls urls_;
std::shared_ptr<const IGDdatas> datas_;
std::shared_ptr<const UPNPUrls> urls_;
#endif #endif
}; };
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment