From 1a6cc728c457fb32260c4ddf5c9ac45f4fcd9fb0 Mon Sep 17 00:00:00 2001
From: Mohamed Chibani <mohamed.chibani@savoirfairelinux.com>
Date: Sat, 13 Mar 2021 10:56:39 -0500
Subject: [PATCH] upnp: code improvements and bug fixes
This patch adds many improvements and bug fixes:
1- For PUPNP lib, use the synchronous methods for add and remove
requests. The async versions have many issues mainly at shutdown.
If the callbacks do not arrive on time, the memory allocated by
the lib when performing the request will leak.
2- Rework the init and IGD search of PUPNP.
3- Move the termination step (for libupnp in particular) from
the destructor to a newly added method (termiinate()). This will
prevent a race at shutdown between libupnp threads (callbacks) and
the daemon shutdown process.
4- Do not try to initialize libupnp if the host address is invalid.
This will prevent putting the lib in an unstable state.
Gitlab: #472
Change-Id: I14e5ddcd2b735da374a1f09bcef40f6a86a38133
---
src/manager.cpp | 7 +-
src/upnp/protocol/natpmp/nat_pmp.cpp | 42 +-
src/upnp/protocol/natpmp/nat_pmp.h | 4 +
src/upnp/protocol/pupnp/pupnp.cpp | 740 ++++++++++++++++-----------
src/upnp/protocol/pupnp/pupnp.h | 59 ++-
src/upnp/protocol/upnp_protocol.h | 5 +-
src/upnp/upnp_context.cpp | 61 ++-
src/upnp/upnp_context.h | 5 +-
8 files changed, 571 insertions(+), 352 deletions(-)
diff --git a/src/manager.cpp b/src/manager.cpp
index 7ce39ed1da..3c13538805 100644
--- a/src/manager.cpp
+++ b/src/manager.cpp
@@ -91,6 +91,8 @@ using random_device = dht::crypto::random_device;
#include "data_transfer.h"
#include "dring/media_const.h"
+#include "upnp/upnp_context.h"
+
#include <libavutil/ffversion.h>
#include <opendht/thread_pool.h>
@@ -845,6 +847,9 @@ Manager::finish() noexcept
return;
try {
+ // Terminate UPNP context
+ jami::upnp::UPnPContext::getUPnPContext()->shutdown();
+
// Forbid call creation
callFactory.forbid();
@@ -894,6 +899,7 @@ Manager::finish() noexcept
}
if (pimpl_->ioContextRunner_.joinable())
pimpl_->ioContextRunner_.join();
+
} catch (const VoipLinkException& err) {
JAMI_ERR("%s", err.what());
}
@@ -2828,7 +2834,6 @@ Manager::removeAccounts()
removeAccount(acc);
}
-
std::vector<std::string_view>
Manager::loadAccountOrder() const
{
diff --git a/src/upnp/protocol/natpmp/nat_pmp.cpp b/src/upnp/protocol/natpmp/nat_pmp.cpp
index eab6049fe3..c7d20f8905 100644
--- a/src/upnp/protocol/natpmp/nat_pmp.cpp
+++ b/src/upnp/protocol/natpmp/nat_pmp.cpp
@@ -27,6 +27,7 @@ namespace upnp {
NatPmp::NatPmp()
{
+ JAMI_DBG("NAT-PMP: Instance [%p] created", this);
getNatpmpScheduler()->run([this] {
threadId_ = getCurrentThread();
igd_ = std::make_shared<PMPIGD>();
@@ -35,7 +36,7 @@ NatPmp::NatPmp()
NatPmp::~NatPmp()
{
- clearIgds();
+ JAMI_DBG("NAT-PMP: Instance [%p] destroyed", this);
}
void
@@ -46,8 +47,6 @@ NatPmp::initNatPmp()
return;
}
- CHECK_VALID_THREAD();
-
initialized_ = false;
hostAddress_ = ip_utils::getLocalAddr(pj_AF_INET());
@@ -116,10 +115,27 @@ NatPmp::initNatPmp()
void
NatPmp::setObserver(UpnpMappingObserver* obs)
{
+ if (not isValidThread()) {
+ getNatpmpScheduler()->run([this, obs] { setObserver(obs); });
+ return;
+ }
+
JAMI_DBG("NAT-PMP: Setting observer to %p", obs);
+
observer_ = obs;
}
+void
+NatPmp::terminate()
+{
+ if (not isValidThread()) {
+ getNatpmpScheduler()->run([this] { terminate(); });
+ return;
+ }
+
+ observer_ = nullptr;
+}
+
void
NatPmp::clearIgds()
{
@@ -128,8 +144,6 @@ NatPmp::clearIgds()
return;
}
- CHECK_VALID_THREAD();
-
initialized_ = false;
if (searchForIgdTimer_)
searchForIgdTimer_->cancel();
@@ -148,8 +162,6 @@ NatPmp::searchForIgd()
return;
}
- CHECK_VALID_THREAD();
-
if (not initialized_) {
initNatPmp();
}
@@ -180,6 +192,11 @@ NatPmp::getIgdList(std::list<std::shared_ptr<IGD>>& igdList) const
bool
NatPmp::isReady() const
{
+ if (observer_ == nullptr) {
+ JAMI_ERR("NAT-PMP: the observer is not set!");
+ return false;
+ }
+
// Must at least have a valid local address.
if (not getHostAddress() or getHostAddress().isLoopback())
return false;
@@ -616,6 +633,8 @@ NatPmp::processIgdUpdate(UpnpIgdEvent event)
removeAllMappings();
}
+ if (observer_ == nullptr)
+ return;
// Process the response on the context thread.
runOnUpnpContextThread([obs = observer_, igd = igd_, event] { obs->onIgdUpdated(igd, event); });
}
@@ -623,6 +642,9 @@ NatPmp::processIgdUpdate(UpnpIgdEvent event)
void
NatPmp::processMappingAdded(const Mapping& map)
{
+ if (observer_ == nullptr)
+ return;
+
// Process the response on the context thread.
runOnUpnpContextThread([obs = observer_, igd = igd_, map] { obs->onMappingAdded(igd, map); });
}
@@ -630,6 +652,9 @@ NatPmp::processMappingAdded(const Mapping& map)
void
NatPmp::processMappingRenewed(const Mapping& map)
{
+ if (observer_ == nullptr)
+ return;
+
// Process the response on the context thread.
runOnUpnpContextThread([obs = observer_, igd = igd_, map] { obs->onMappingRenewed(igd, map); });
}
@@ -637,6 +662,9 @@ NatPmp::processMappingRenewed(const Mapping& map)
void
NatPmp::processMappingRemoved(const Mapping& map)
{
+ if (observer_ == nullptr)
+ return;
+
// Process the response on the context thread.
runOnUpnpContextThread([obs = observer_, igd = igd_, map] { obs->onMappingRemoved(igd, map); });
}
diff --git a/src/upnp/protocol/natpmp/nat_pmp.h b/src/upnp/protocol/natpmp/nat_pmp.h
index ddf0a7ea8b..fe2bf22956 100644
--- a/src/upnp/protocol/natpmp/nat_pmp.h
+++ b/src/upnp/protocol/natpmp/nat_pmp.h
@@ -97,6 +97,10 @@ public:
// Removes a mapping.
void requestMappingRemove(const Mapping& igdMapping) override;
+ // Terminate. Nothing to do here, the clean-up is done when
+ // the IGD is cleared.
+ void terminate() override;
+
private:
void initNatPmp();
void getIgdPublicAddress();
diff --git a/src/upnp/protocol/pupnp/pupnp.cpp b/src/upnp/protocol/pupnp/pupnp.cpp
index 0b81a8c099..20aef2bede 100644
--- a/src/upnp/protocol/pupnp/pupnp.cpp
+++ b/src/upnp/protocol/pupnp/pupnp.cpp
@@ -98,20 +98,90 @@ errorOnResponse(IXML_Document* doc)
PUPnP::PUPnP()
{
+ JAMI_DBG("PUPnP: Instance [%p] created", this);
threadId_ = getCurrentThread();
+}
- if (not initUpnpLib())
- return;
- if (not registerClient())
+PUPnP::~PUPnP()
+{
+ JAMI_DBG("PUPnP: Instance [%p] destroyed", this);
+}
+
+void
+PUPnP::initUpnpLib()
+{
+ assert(not initialized_);
+
+ int upnp_err = UpnpInit2(nullptr, 0);
+
+ if (upnp_err != UPNP_E_SUCCESS) {
+ JAMI_ERR("PUPnP: Can't initialize libupnp: %s", UpnpGetErrorMessage(upnp_err));
+ UpnpFinish();
+ initialized_ = false;
return;
+ }
+
+ // Disable embedded WebServer if any.
+ if (UpnpIsWebserverEnabled() == 1) {
+ JAMI_WARN("PUPnP: Web-server is enabled. Disabling");
+ UpnpEnableWebserver(0);
+ if (UpnpIsWebserverEnabled() == 1) {
+ JAMI_ERR("PUPnP: Could not disable Web-server!");
+ } else {
+ JAMI_DBG("PUPnP: Web-server successfully disabled");
+ }
+ }
+
+ char* ip_address = UpnpGetServerIpAddress();
+ char* ip_address6 = nullptr;
+ unsigned short port = UpnpGetServerPort();
+ unsigned short port6 = 0;
+#if UPNP_ENABLE_IPV6
+ ip_address6 = UpnpGetServerIp6Address();
+ port6 = UpnpGetServerPort6();
+#endif
+ if (ip_address6 and port6)
+ JAMI_DBG("PUPnP: Initialized on %s:%u | %s:%u", ip_address, port, ip_address6, port6);
+ else
+ JAMI_DBG("PUPnP: Initialized on %s:%u", ip_address, port);
+
+ // Relax the parser to allow malformed XML text.
+ ixmlRelaxParser(1);
+
+ initialized_ = true;
+}
+
+void
+PUPnP::registerClient()
+{
+ assert(not clientRegistered_);
+
+ CHECK_VALID_THREAD();
+
+ // Register Upnp control point.
+ std::unique_lock<std::mutex> lk(ctrlptMutex_);
+ int upnp_err = UpnpRegisterClient(ctrlPtCallback, this, &ctrlptHandle_);
+ if (upnp_err != UPNP_E_SUCCESS) {
+ JAMI_ERR("PUPnP: Can't register client: %s", UpnpGetErrorMessage(upnp_err));
+ } else {
+ JAMI_DBG("PUPnP: Successfully registered client");
+ clientRegistered_ = true;
+ }
+}
+
+void
+PUPnP::startPUPnP()
+{
+ JAMI_DBG("PUPnP: Starting PUPNP internal thread");
pupnpThread_ = std::thread([this] {
+ JAMI_DBG("PUPnP: Internal thread started");
+ pupnpRun_ = true;
while (pupnpRun_) {
{
std::unique_lock<std::mutex> lk(igdListMutex_);
pupnpCv_.wait(lk, [this] {
- return not clientRegistered_ or not pupnpRun_ or searchForIgd_
- or not dwnldlXmlList_.empty();
+ return not pupnpRun_ or searchForIgd_ or not dwnldlXmlList_.empty();
});
}
@@ -158,90 +228,60 @@ PUPnP::PUPnP()
}
}
}
+
+ JAMI_DBG("PUPnP: Internal thread stopped");
});
}
-PUPnP::~PUPnP()
+void
+PUPnP::setObserver(UpnpMappingObserver* obs)
{
- // Clear all the lists.
+ CHECK_VALID_THREAD();
+
+ JAMI_DBG("PUPnP: Setting observer to %p", obs);
+
+ observer_ = obs;
+}
+
+void
+PUPnP::terminate()
+{
+ CHECK_VALID_THREAD();
+
+ JAMI_DBG("PUPnP: Terminate instance %p", this);
+
+ clientRegistered_ = false;
+ observer_ = nullptr;
+
{
std::lock_guard<std::mutex> lock(validIgdListMutex_);
std::lock_guard<std::mutex> lk(ctrlptMutex_);
- validIgdList_.clear();
- clientRegistered_ = false;
+
UpnpUnRegisterClient(ctrlptHandle_);
}
- {
- std::lock_guard<std::mutex> lk2(igdListMutex_);
- discoveredIgdList_.clear();
- dwnldlXmlList_.clear();
- cancelXmlList_.clear();
- pupnpRun_ = false;
+ if (UpnpFinish() != UPNP_E_SUCCESS) {
+ JAMI_ERR("PUPnP: Failed to properly close lib-upnp");
}
- UpnpFinish();
-
+ pupnpRun_ = false;
// Notify thread to terminate.
pupnpCv_.notify_all();
if (pupnpThread_.joinable())
pupnpThread_.join();
- JAMI_DBG("PUPnP: Instance [%p] destroyed", this);
-}
-
-bool
-PUPnP::initUpnpLib()
-{
- int upnp_err = UpnpInit2(nullptr, 0);
-
- if (upnp_err != UPNP_E_SUCCESS) {
- JAMI_ERR("PUPnP: Can't initialize libupnp: %s", UpnpGetErrorMessage(upnp_err));
- UpnpFinish();
- pupnpRun_ = false;
- return false;
+ // Clear all the lists.
+ {
+ std::lock_guard<std::mutex> lock(validIgdListMutex_);
+ validIgdList_.clear();
}
- char* ip_address = UpnpGetServerIpAddress();
- char* ip_address6 = nullptr;
- unsigned short port = UpnpGetServerPort();
- unsigned short port6 = 0;
-#if UPNP_ENABLE_IPV6
- ip_address6 = UpnpGetServerIp6Address();
- port6 = UpnpGetServerPort6();
-#endif
- if (ip_address6 and port6)
- JAMI_DBG("PUPnP: Initialized on %s:%u | %s:%u", ip_address, port, ip_address6, port6);
- else
- JAMI_DBG("PUPnP: Initialized on %s:%u", ip_address, port);
- // Relax the parser to allow malformed XML text.
- ixmlRelaxParser(1);
-
- return true;
-}
-
-bool
-PUPnP::registerClient()
-{
- // Register Upnp control point.
- std::unique_lock<std::mutex> lk(ctrlptMutex_);
- int upnp_err = UpnpRegisterClient(ctrlPtCallback, this, &ctrlptHandle_);
- if (upnp_err != UPNP_E_SUCCESS) {
- JAMI_ERR("PUPnP: Can't register client: %s", UpnpGetErrorMessage(upnp_err));
- pupnpRun_ = false;
- return false;
- } else {
- JAMI_DBG("PUPnP: Successfully registered client");
- clientRegistered_ = true;
+ {
+ std::lock_guard<std::mutex> lk2(igdListMutex_);
+ discoveredIgdList_.clear();
+ dwnldlXmlList_.clear();
+ cancelXmlList_.clear();
}
-
- return true;
-}
-
-void
-PUPnP::setObserver(UpnpMappingObserver* obs)
-{
- observer_ = obs;
}
void
@@ -295,25 +335,58 @@ PUPnP::searchForIgd()
// Update local address before searching.
hostAddress_ = ip_utils::getLocalAddr(pj_AF_INET());
- if (not isReady()) {
- if (igdSearchCounter_++ < PUPNP_MAX_RESTART_SEARCH_RETRIES) {
- JAMI_DBG("PUPnP: Start search for IGD: attempt %u", igdSearchCounter_);
+ if (isReady()) {
+ JAMI_WARN("PUPnP: Already have a valid IGD. Skipping the search request");
+ return;
+ }
- searchForIgd_ = true;
- pupnpCv_.notify_one();
+ if (igdSearchCounter_++ >= PUPNP_MAX_RESTART_SEARCH_RETRIES) {
+ JAMI_WARN("PUPnP: Setup failed after %u trials. PUPnP will be disabled!",
+ PUPNP_MAX_RESTART_SEARCH_RETRIES);
+ return;
+ }
- // Cancel the current timer (if any) and re-schedule.
- if (searchForIgdTimer_)
- searchForIgdTimer_->cancel();
+ JAMI_DBG("PUPnP: Start search for IGD: attempt %u", igdSearchCounter_);
- searchForIgdTimer_ = getUpnContextScheduler()
- ->scheduleIn([this] { searchForIgd(); },
- PUPNP_TIMEOUT_BEFORE_IGD_SEARCH_RETRY);
- } else {
- JAMI_WARN("PUPnP: Setup failed after %u trials. PUPnP will be disabled!",
- PUPNP_MAX_RESTART_SEARCH_RETRIES);
+ if (not pupnpRun_) {
+ startPUPnP();
+ }
+
+ // Do not init if the host is not valid. Otherwise, the init will fail
+ // anyway and may put libupnp in an unstable state (mainly deadlocks)
+ // even if the UpnpFinish() method is called.
+ if (not hostAddress_ or hostAddress_.isLoopback()) {
+ JAMI_WARN("PUPnP: Host address is invalid. Skipping the IGD search");
+ } else {
+ // Init and register if needed
+ if (not initialized_) {
+ initUpnpLib();
+ }
+ if (initialized_ and not clientRegistered_) {
+ registerClient();
}
}
+
+ // Start search
+ if (clientRegistered_ and pupnpRun_) {
+ assert(initialized_);
+ JAMI_DBG("PUPnP: Start search for IGD");
+ searchForIgd_ = true;
+ pupnpCv_.notify_one();
+ } else {
+ JAMI_WARN("PUPnP: PUPNP not fully setup. Skipping the IGD search");
+ }
+
+ // Cancel the current timer (if any) and re-schedule.
+ if (searchForIgdTimer_)
+ searchForIgdTimer_->cancel();
+
+ searchForIgdTimer_ = getUpnContextScheduler()->scheduleIn(
+ [w = weak()] {
+ if (auto upnpThis = w.lock())
+ upnpThis->searchForIgd();
+ },
+ PUPNP_TIMEOUT_BEFORE_IGD_SEARCH_RETRY);
}
void
@@ -369,7 +442,8 @@ PUPnP::incrementErrorsCounter(const std::shared_ptr<IGD>& igd)
// Disable this IGD.
igd->setValid(false);
// Notify the listener.
- observer_->onIgdUpdated(igd, UpnpIgdEvent::INVALID_STATE);
+ if (observer_)
+ observer_->onIgdUpdated(igd, UpnpIgdEvent::INVALID_STATE);
}
}
@@ -484,18 +558,24 @@ PUPnP::validateIgd(const IGDInfo& info)
deleteMappingsByDescription(igd_candidate, Mapping::UPNP_MAPPING_DESCRIPTION_PREFIX);
// Report to the listener.
- observer_->onIgdUpdated(igd_candidate, UpnpIgdEvent::ADDED);
+ if (observer_)
+ observer_->onIgdUpdated(igd_candidate, UpnpIgdEvent::ADDED);
// Subscribe to IGD events.
int upnp_err = UpnpSubscribeAsync(ctrlptHandle_,
eventSub.c_str(),
- SUBSCRIBE_TIMEOUT,
+ UPNP_INFINITE,
subEventCallback,
this);
- if (upnp_err != UPNP_E_SUCCESS)
- JAMI_WARN("PUPnP: Error when trying to request subscription for %s -> %s",
+ if (upnp_err != UPNP_E_SUCCESS) {
+ JAMI_WARN("PUPnP: Failed to send subscribe request to %s: error %i - %s",
igd_candidate->getUID().c_str(),
+ upnp_err,
UpnpGetErrorMessage(upnp_err));
+ } else {
+ JAMI_DBG("PUPnP: Successfully sent subscribe request to %s",
+ igd_candidate->getUID().c_str());
+ }
return true;
}
@@ -503,13 +583,16 @@ PUPnP::validateIgd(const IGDInfo& info)
void
PUPnP::requestMappingAdd(const std::shared_ptr<IGD>& igd, const Mapping& mapping)
{
- if (auto pupnp_igd = std::dynamic_pointer_cast<const UPnPIGD>(igd)) {
- actionAddPortMappingAsync(*pupnp_igd, mapping);
+ if (auto pupnp_igd = std::dynamic_pointer_cast<UPnPIGD>(igd)) {
+ dht::ThreadPool::io().run([w = weak(), pupnp_igd, mapping] {
+ if (auto upnpThis = w.lock())
+ upnpThis->actionAddPortMapping(pupnp_igd, mapping);
+ });
}
}
bool
-PUPnP::actionAddPortMappingAsync(const UPnPIGD& igd, const Mapping& mapping)
+PUPnP::actionAddPortMappingAsync(const std::shared_ptr<UPnPIGD>& igd, const Mapping& mapping)
{
if (not clientRegistered_) {
return false;
@@ -519,48 +602,49 @@ PUPnP::actionAddPortMappingAsync(const UPnPIGD& igd, const Mapping& mapping)
// Set action sequence.
UpnpAddToAction(&action_container_ptr,
ACTION_ADD_PORT_MAPPING,
- igd.getServiceType().c_str(),
+ igd->getServiceType().c_str(),
"NewRemoteHost",
"");
UpnpAddToAction(&action_container_ptr,
ACTION_ADD_PORT_MAPPING,
- igd.getServiceType().c_str(),
+ igd->getServiceType().c_str(),
"NewExternalPort",
mapping.getExternalPortStr().c_str());
UpnpAddToAction(&action_container_ptr,
ACTION_ADD_PORT_MAPPING,
- igd.getServiceType().c_str(),
+ igd->getServiceType().c_str(),
"NewProtocol",
mapping.getTypeStr());
UpnpAddToAction(&action_container_ptr,
ACTION_ADD_PORT_MAPPING,
- igd.getServiceType().c_str(),
+ igd->getServiceType().c_str(),
"NewInternalPort",
mapping.getInternalPortStr().c_str());
UpnpAddToAction(&action_container_ptr,
ACTION_ADD_PORT_MAPPING,
- igd.getServiceType().c_str(),
+ igd->getServiceType().c_str(),
"NewInternalClient",
getHostAddress().toString().c_str());
UpnpAddToAction(&action_container_ptr,
ACTION_ADD_PORT_MAPPING,
- igd.getServiceType().c_str(),
+ igd->getServiceType().c_str(),
"NewEnabled",
"1");
UpnpAddToAction(&action_container_ptr,
ACTION_ADD_PORT_MAPPING,
- igd.getServiceType().c_str(),
+ igd->getServiceType().c_str(),
"NewPortMappingDescription",
mapping.toString().c_str());
UpnpAddToAction(&action_container_ptr,
ACTION_ADD_PORT_MAPPING,
- igd.getServiceType().c_str(),
+ igd->getServiceType().c_str(),
"NewLeaseDuration",
"0");
action.reset(action_container_ptr);
+
int upnp_err = UpnpSendActionAsync(ctrlptHandle_,
- igd.getControlURL().c_str(),
- igd.getServiceType().c_str(),
+ igd->getControlURL().c_str(),
+ igd->getServiceType().c_str(),
nullptr,
action.get(),
ctrlPtCallback,
@@ -568,12 +652,14 @@ PUPnP::actionAddPortMappingAsync(const UPnPIGD& igd, const Mapping& mapping)
if (upnp_err != UPNP_E_SUCCESS) {
JAMI_WARN("PUPnP: Failed to send async action %s from: %s, %d: %s",
ACTION_ADD_PORT_MAPPING,
- igd.getServiceType().c_str(),
+ igd->getServiceType().c_str(),
upnp_err,
UpnpGetErrorMessage(upnp_err));
return false;
}
+
JAMI_DBG("PUPnP: Sent request to open port %s", mapping.toString().c_str());
+
return true;
}
@@ -585,8 +671,6 @@ PUPnP::processAddMapAction(const std::string& ctrlURL,
{
Mapping mapToAdd(portType, ePort, iPort);
- JAMI_DBG("PUPnP: Opened port %s", mapToAdd.toString().c_str());
-
{
std::lock_guard<std::mutex> lock(validIgdListMutex_);
for (auto const& it : validIgdList_) {
@@ -601,9 +685,11 @@ PUPnP::processAddMapAction(const std::string& ctrlURL,
}
if (mapToAdd.getIgd()) {
- observer_->onMappingAdded(mapToAdd.getIgd(), std::move(mapToAdd));
+ JAMI_DBG("PUPnP: Opened port %s", mapToAdd.toString().c_str());
+ if (observer_)
+ observer_->onMappingAdded(mapToAdd.getIgd(), std::move(mapToAdd));
} else {
- JAMI_WARN("PUPnP: Did not find matching ctrl URL [%s] for (map %s)",
+ JAMI_WARN("PUPnP: Did not find matching ctrl URL [%s] for %s",
ctrlURL.c_str(),
mapToAdd.toString().c_str());
}
@@ -616,6 +702,7 @@ PUPnP::processRemoveMapAction(const std::string& ctrlURL,
PortType portType)
{
Mapping mapToRemove(portType, ePort, iPort);
+
{
std::lock_guard<std::mutex> lock(validIgdListMutex_);
for (auto const& it : validIgdList_) {
@@ -629,26 +716,34 @@ PUPnP::processRemoveMapAction(const std::string& ctrlURL,
}
if (mapToRemove.getIgd()) {
- observer_->onMappingRemoved(mapToRemove.getIgd(), std::move(mapToRemove));
+ JAMI_DBG("PUPnP: Closed port %s", mapToRemove.toString().c_str());
+ if (observer_)
+ observer_->onMappingRemoved(mapToRemove.getIgd(), std::move(mapToRemove));
} else {
- JAMI_WARN("PUPnP: Did not find matching ctrl URL [%s] for (map %s)",
+ JAMI_WARN("PUPnP: Did not find matching ctrl URL [%s] for %s",
ctrlURL.c_str(),
mapToRemove.toString().c_str());
}
}
void
-PUPnP::requestMappingRemove(const Mapping& igdMapping)
+PUPnP::requestMappingRemove(const Mapping& mapping)
{
+ auto igd = std::dynamic_pointer_cast<UPnPIGD>(mapping.getIgd());
+
+ if (not igd)
+ return;
+
std::lock_guard<std::mutex> lock(validIgdListMutex_);
- // Iterate over all IGDs in internal list and try to remove selected mapping.
for (auto const& it : validIgdList_) {
- if (auto igd = std::dynamic_pointer_cast<UPnPIGD>(it)) {
- std::lock_guard<std::mutex> lk2(ctrlptMutex_);
- actionDeletePortMappingAsync(*igd,
- igdMapping.getExternalPortStr(),
- igdMapping.getTypeStr());
- return;
+ if (std::dynamic_pointer_cast<UPnPIGD>(it) == igd) {
+ // Send remove request using the matching IGD
+ dht::ThreadPool::io().run([w = weak(), igd, mapping] {
+ if (auto upnpThis = w.lock()) {
+ upnpThis->actionDeletePortMapping(igd, mapping);
+ }
+ });
+ break;
}
}
}
@@ -695,13 +790,24 @@ PUPnP::eventTypeToString(Upnp_EventType eventType)
int
PUPnP::ctrlPtCallback(Upnp_EventType event_type, const void* event, void* user_data)
{
- if (auto pupnp = static_cast<PUPnP*>(user_data)) {
- std::lock_guard<std::mutex> lk(pupnp->ctrlptMutex_);
- return pupnp->handleCtrlPtUPnPEvents(event_type, event);
+ auto pupnp = static_cast<PUPnP*>(user_data);
+
+ if (pupnp == nullptr) {
+ JAMI_WARN("PUPnP: Control point callback without PUPnP");
+ return UPNP_E_SUCCESS;
}
- JAMI_WARN("PUPnP: Control point callback without PUPnP");
- return 0;
+ auto upnpThis = pupnp->weak().lock();
+
+ if (not upnpThis)
+ return UPNP_E_SUCCESS;
+
+ // Ignore if already unregistered.
+ if (not upnpThis->clientRegistered_)
+ return UPNP_E_SUCCESS;
+
+ // Process the callback.
+ return upnpThis->handleCtrlPtUPnPEvents(event_type, event);
}
PUPnP::CtrlAction
@@ -760,27 +866,27 @@ PUPnP::processDiscoverySearchResult(const std::string& cpDeviceId,
return;
}
- dwnldlXmlList_.emplace_back(
- dht::ThreadPool::io().get<pIGDInfo>([this, location = std::move(igdLocationUrl)] {
- IXML_Document* doc_container_ptr = nullptr;
- XMLDocument doc_desc_ptr(nullptr, ixmlDocument_free);
- int upnp_err = UpnpDownloadXmlDoc(location.c_str(), &doc_container_ptr);
- if (doc_container_ptr)
- doc_desc_ptr.reset(doc_container_ptr);
- pupnpCv_.notify_all();
- if (upnp_err != UPNP_E_SUCCESS or not doc_desc_ptr) {
- JAMI_WARN("PUPnP: Error downloading device XML document from %s -> %s",
- location.c_str(),
- UpnpGetErrorMessage(upnp_err));
- return std::make_unique<IGDInfo>(
- IGDInfo {std::move(location), XMLDocument(nullptr, ixmlDocument_free)});
- } else {
- JAMI_DBG("PUPnP: Succeeded to download device XML document from %s",
- location.c_str());
- return std::make_unique<IGDInfo>(
- IGDInfo {std::move(location), std::move(doc_desc_ptr)});
- }
- }));
+ dwnldlXmlList_.emplace_back(dht::ThreadPool::io().get<pIGDInfo>([this,
+ location = std::move(
+ igdLocationUrl)] {
+ IXML_Document* doc_container_ptr = nullptr;
+ XMLDocument doc_desc(nullptr, ixmlDocument_free);
+ int upnp_err = UpnpDownloadXmlDoc(location.c_str(), &doc_container_ptr);
+ doc_desc.reset(doc_container_ptr);
+
+ pupnpCv_.notify_all();
+
+ if (upnp_err != UPNP_E_SUCCESS or not doc_desc) {
+ JAMI_WARN("PUPnP: Error downloading device XML document from %s -> %s",
+ location.c_str(),
+ UpnpGetErrorMessage(upnp_err));
+ return std::make_unique<IGDInfo>(
+ IGDInfo {std::move(location), XMLDocument(nullptr, ixmlDocument_free)});
+ } else {
+ JAMI_DBG("PUPnP: Succeeded to download device XML document from %s", location.c_str());
+ return std::make_unique<IGDInfo>(IGDInfo {std::move(location), std::move(doc_desc)});
+ }
+ }));
}
void
@@ -813,26 +919,26 @@ PUPnP::processDiscoveryAdvertisementByebye(const std::string& cpDeviceId)
}
// Notify the listener.
- if (igd) {
+ if (observer_ and igd) {
observer_->onIgdUpdated(igd, UpnpIgdEvent::REMOVED);
}
}
void
-PUPnP::processDiscoverySubscriptionExpired(const std::string& eventSubUrl)
+PUPnP::processDiscoverySubscriptionExpired(Upnp_EventType event_type, const std::string& eventSubUrl)
{
std::lock_guard<std::mutex> lk(validIgdListMutex_);
for (auto& it : validIgdList_) {
if (auto igd = std::dynamic_pointer_cast<UPnPIGD>(it)) {
if (igd->getEventSubURL() == eventSubUrl) {
- JAMI_DBG("PUPnP: Received [%s] event for IGD [%s] %s. Will be renewed.",
- PUPnP::eventTypeToString(UPNP_EVENT_SUBSCRIPTION_EXPIRED),
+ JAMI_DBG("PUPnP: Received [%s] event for IGD [%s] %s. Request a new subscribe.",
+ PUPnP::eventTypeToString(event_type),
igd->getUID().c_str(),
igd->getLocalIp().toString().c_str());
std::lock_guard<std::mutex> lk1(ctrlptMutex_);
UpnpSubscribeAsync(ctrlptHandle_,
eventSubUrl.c_str(),
- SUBSCRIBE_TIMEOUT,
+ UPNP_INFINITE,
subEventCallback,
this);
break;
@@ -844,10 +950,6 @@ PUPnP::processDiscoverySubscriptionExpired(const std::string& eventSubUrl)
int
PUPnP::handleCtrlPtUPnPEvents(Upnp_EventType event_type, const void* event)
{
- // Ignore if not registered
- if (not PUPnP::clientRegistered_)
- return UPNP_E_SUCCESS;
-
switch (event_type) {
// "ALIVE" events are processed as "SEARCH RESULT". It might be usefull
// if "SEARCH RESULT" was missed.
@@ -888,7 +990,6 @@ PUPnP::handleCtrlPtUPnPEvents(Upnp_EventType event_type, const void* event)
upnpThis->processDiscoveryAdvertisementByebye(deviceId);
}
});
-
break;
}
case UPNP_DISCOVERY_SEARCH_TIMEOUT: {
@@ -902,36 +1003,63 @@ PUPnP::handleCtrlPtUPnPEvents(Upnp_EventType event_type, const void* event)
// Nothing to do.
break;
}
- case UPNP_EVENT_AUTORENEWAL_FAILED: // Fall through. Treat failed autorenewal like an expired
- // subscription.
+ // Treat failed autorenewal like an expired subscription.
+ case UPNP_EVENT_AUTORENEWAL_FAILED:
case UPNP_EVENT_SUBSCRIPTION_EXPIRED: // This event will occur only if autorenewal is disabled.
{
+ JAMI_WARN("PUPnP: Received Subscription Event %s", eventTypeToString(event_type));
const UpnpEventSubscribe* es_event = (const UpnpEventSubscribe*) event;
+ if (es_event == nullptr) {
+ JAMI_WARN("PUPnP: Received Subscription Event with null pointer");
+ break;
+ }
std::string publisherUrl(UpnpEventSubscribe_get_PublisherUrl_cstr(es_event));
// Process the response on the main thread.
- runOnUpnpContextThread([w = weak(), publisherUrl = std::move(publisherUrl)] {
+ runOnUpnpContextThread([w = weak(), event_type, publisherUrl = std::move(publisherUrl)] {
if (auto upnpThis = w.lock()) {
- upnpThis->processDiscoverySubscriptionExpired(publisherUrl);
+ upnpThis->processDiscoverySubscriptionExpired(event_type, publisherUrl);
}
});
break;
}
- case UPNP_EVENT_SUBSCRIBE_COMPLETE: {
- break;
- }
+ case UPNP_EVENT_SUBSCRIBE_COMPLETE:
case UPNP_EVENT_UNSUBSCRIBE_COMPLETE: {
+ UpnpEventSubscribe* es_event = (UpnpEventSubscribe*) event;
+ if (es_event == nullptr) {
+ JAMI_WARN("PUPnP: Received Subscription Event with null pointer");
+ } else {
+ UpnpEventSubscribe_delete(es_event);
+ }
break;
}
case UPNP_CONTROL_ACTION_COMPLETE: {
const UpnpActionComplete* a_event = (const UpnpActionComplete*) event;
-
- if (UpnpActionComplete_get_ErrCode(a_event) == UPNP_E_SUCCESS) {
+ if (a_event == nullptr) {
+ JAMI_WARN("PUPnP: Received Action Complete Event with null pointer");
+ break;
+ }
+ auto res = UpnpActionComplete_get_ErrCode(a_event);
+ if (res != UPNP_E_SUCCESS and res != UPNP_E_TIMEDOUT) {
+ auto err = UpnpActionComplete_get_ErrCode(a_event);
+ JAMI_WARN("PUPnP: Received Action Complete error %i %s", err, UpnpGetErrorMessage(err));
+ } else {
auto actionRequest = UpnpActionComplete_get_ActionRequest(a_event);
// Abort if there is no action to process.
- if (actionRequest == nullptr)
+ if (actionRequest == nullptr) {
+ JAMI_WARN("PUPnP: Can't get the Action Request data from the event");
break;
+ }
+
+ auto actionResult = UpnpActionComplete_get_ActionResult(a_event);
+ if (actionResult != nullptr) {
+ ixmlDocument_free(actionResult);
+ } else {
+ JAMI_WARN("PUPnP: Action Result document not found");
+ }
+
auto upnpString = UpnpActionComplete_get_CtrlUrl(a_event);
+
std::string ctrlUrl {UpnpString_get_String(upnpString),
UpnpString_get_Length(upnpString)};
@@ -945,6 +1073,8 @@ PUPnP::handleCtrlPtUPnPEvents(Upnp_EventType event_type, const void* event)
std::string iPortStr(getFirstDocItem(actionRequest, "NewInternalPort"));
std::string portTypeStr(getFirstDocItem(actionRequest, "NewProtocol"));
+ ixmlDocument_free(actionRequest);
+
uint16_t ePort = ePortStr.empty() ? 0 : std::stoi(ePortStr);
uint16_t iPort = iPortStr.empty() ? 0 : std::stoi(iPortStr);
PortType portType = portTypeStr == "UDP" ? upnp::PortType::UDP
@@ -957,18 +1087,19 @@ PUPnP::handleCtrlPtUPnPEvents(Upnp_EventType event_type, const void* event)
ePort,
iPort,
portType] {
- if (auto upnpThis = w.lock()) {
- switch (ctrlAction) {
- case CtrlAction::ADD_PORT_MAPPING:
- upnpThis->processAddMapAction(ctrlUrl, ePort, iPort, portType);
- break;
- case CtrlAction::DELETE_PORT_MAPPING:
- upnpThis->processRemoveMapAction(ctrlUrl, ePort, iPort, portType);
- break;
- default:
- // All other control actions are ignored.
- break;
- }
+ auto upnpThis = w.lock();
+ if (not upnpThis)
+ return;
+ switch (ctrlAction) {
+ case CtrlAction::ADD_PORT_MAPPING:
+ upnpThis->processAddMapAction(ctrlUrl, ePort, iPort, portType);
+ break;
+ case CtrlAction::DELETE_PORT_MAPPING:
+ upnpThis->processRemoveMapAction(ctrlUrl, ePort, iPort, portType);
+ break;
+ default:
+ // All other control actions are ignored.
+ break;
}
});
}
@@ -994,16 +1125,15 @@ PUPnP::subEventCallback(Upnp_EventType event_type, const void* event, void* user
}
int
-PUPnP::handleSubscriptionUPnPEvent(Upnp_EventType /*unused*/, const void* event)
+PUPnP::handleSubscriptionUPnPEvent(Upnp_EventType event_type, const void* event)
{
- if (event == nullptr) {
+ UpnpEventSubscribe* es_event = static_cast<UpnpEventSubscribe*>(const_cast<void*>(event));
+
+ if (es_event == nullptr) {
JAMI_ERR("PUPnP: Unexpected null pointer!");
return UPNP_E_INVALID_ARGUMENT;
}
-
- const UpnpEventSubscribe* es_event = (const UpnpEventSubscribe*) event;
std::string publisherUrl(UpnpEventSubscribe_get_PublisherUrl_cstr(es_event));
-
int upnp_err = UpnpEventSubscribe_get_ErrCode(es_event);
if (upnp_err != UPNP_E_SUCCESS) {
JAMI_WARN("PUPnP: Subscription error %s from %s",
@@ -1144,9 +1274,9 @@ PUPnP::actionIsIgdConnected(const UPnPIGD& igd)
return false;
// Action and response pointers.
- XMLDocument action(nullptr, ixmlDocument_free); // Action pointer.
- XMLDocument response(nullptr, ixmlDocument_free); // Response pointer.
+ XMLDocument action(nullptr, ixmlDocument_free); // Action pointer.
IXML_Document* action_container_ptr = nullptr;
+ XMLDocument response(nullptr, ixmlDocument_free); // Response pointer.
IXML_Document* response_container_ptr = nullptr;
// Set action name.
@@ -1156,11 +1286,12 @@ PUPnP::actionIsIgdConnected(const UPnPIGD& igd)
igd.getServiceType().c_str(),
0,
nullptr);
- if (not action_container_ptr) {
+ action.reset(action_container_ptr);
+
+ if (not action) {
JAMI_WARN("PUPnP: Failed to make GetStatusInfo action");
return false;
}
- action.reset(action_container_ptr);
int upnp_err = UpnpSendAction(ctrlptHandle_,
igd.getControlURL().c_str(),
@@ -1168,11 +1299,12 @@ PUPnP::actionIsIgdConnected(const UPnPIGD& igd)
nullptr,
action.get(),
&response_container_ptr);
- if (upnp_err != UPNP_E_SUCCESS) {
+ response.reset(response_container_ptr);
+
+ if (not response or upnp_err != UPNP_E_SUCCESS) {
JAMI_WARN("PUPnP: Failed to send GetStatusInfo action -> %s", UpnpGetErrorMessage(upnp_err));
return false;
}
- response.reset(response_container_ptr);
if (errorOnResponse(response.get())) {
JAMI_WARN("PUPnP: Failed to get GetStatusInfo from %s -> %d: %s",
@@ -1207,11 +1339,12 @@ PUPnP::actionGetExternalIP(const UPnPIGD& igd)
IXML_Document* action_container_ptr = nullptr;
action_container_ptr = UpnpMakeAction(action_name, igd.getServiceType().c_str(), 0, nullptr);
- if (not action_container_ptr) {
+ action.reset(action_container_ptr);
+
+ if (not action) {
JAMI_WARN("PUPnP: Failed to make GetExternalIPAddress action");
return {};
}
- action.reset(action_container_ptr);
IXML_Document* response_container_ptr = nullptr;
int upnp_err = UpnpSendAction(ctrlptHandle_,
@@ -1220,12 +1353,13 @@ PUPnP::actionGetExternalIP(const UPnPIGD& igd)
nullptr,
action.get(),
&response_container_ptr);
- if (upnp_err != UPNP_E_SUCCESS) {
+ response.reset(response_container_ptr);
+
+ if (not response or upnp_err != UPNP_E_SUCCESS) {
JAMI_WARN("PUPnP: Failed to send GetExternalIPAddress action -> %s",
UpnpGetErrorMessage(upnp_err));
return {};
}
- response.reset(response_container_ptr);
if (errorOnResponse(response.get())) {
JAMI_WARN("PUPnP: Failed to get GetExternalIPAddress from %s -> %d: %s",
@@ -1255,9 +1389,10 @@ PUPnP::getMappingsListByDescr(const std::shared_ptr<IGD>& igd, const std::string
for (int entry_idx = 0;; entry_idx++) {
std::unique_ptr<IXML_Document, decltype(ixmlDocument_free)&>
action(nullptr, ixmlDocument_free); // Action pointer.
+ IXML_Document* action_container_ptr = nullptr;
+
std::unique_ptr<IXML_Document, decltype(ixmlDocument_free)&>
response(nullptr, ixmlDocument_free); // Response pointer.
- IXML_Document* action_container_ptr = nullptr;
IXML_Document* response_container_ptr = nullptr;
UpnpAddToAction(&action_container_ptr,
@@ -1267,6 +1402,11 @@ PUPnP::getMappingsListByDescr(const std::shared_ptr<IGD>& igd, const std::string
std::to_string(entry_idx).c_str());
action.reset(action_container_ptr);
+ if (not action) {
+ JAMI_WARN("PUPnP: Failed to add NewPortMappingIndex action");
+ break;
+ }
+
int upnp_err = UpnpSendAction(ctrlptHandle_,
upnpIgd->getControlURL().c_str(),
upnpIgd->getServiceType().c_str(),
@@ -1274,6 +1414,7 @@ PUPnP::getMappingsListByDescr(const std::shared_ptr<IGD>& igd, const std::string
action.get(),
&response_container_ptr);
response.reset(response_container_ptr);
+
if (not response) {
// No existing mapping. Abort silently.
break;
@@ -1359,161 +1500,107 @@ PUPnP::deleteMappingsByDescription(const std::shared_ptr<IGD>& igd, const std::s
}
bool
-PUPnP::actionDeletePortMapping(const UPnPIGD& igd,
- const std::string& port_external,
- const std::string& protocol)
+PUPnP::actionAddPortMapping(const std::shared_ptr<UPnPIGD>& igd, const Mapping& mapping)
{
if (not clientRegistered_)
return false;
// Action and response pointers.
- std::unique_ptr<IXML_Document, decltype(ixmlDocument_free)&>
- action(nullptr, ixmlDocument_free); // Action pointer.
- std::unique_ptr<IXML_Document, decltype(ixmlDocument_free)&>
- response(nullptr, ixmlDocument_free); // Response pointer.
- IXML_Document* action_container_ptr = nullptr;
-
- // Set action name.
- std::string action_name {"DeletePortMapping"};
-
- // Set action sequence.
- UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
- "NewRemoteHost",
- "");
- UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
- "NewExternalPort",
- port_external.c_str());
- UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
- "NewProtocol",
- protocol.c_str());
-
- action.reset(action_container_ptr);
- int upnp_err = UpnpSendActionAsync(ctrlptHandle_,
- igd.getControlURL().c_str(),
- igd.getServiceType().c_str(),
- nullptr,
- action.get(),
- ctrlPtCallback,
- this);
-
- if (upnp_err != UPNP_E_SUCCESS) {
- JAMI_WARN("PUPnP: Failed to send %s request from: %s, %d: %s",
- action_name.c_str(),
- igd.getServiceType().c_str(),
- upnp_err,
- UpnpGetErrorMessage(upnp_err));
- return false;
- }
-
- JAMI_DBG("PUPnP: Successfully sent %s request to %s",
- action_name.c_str(),
- igd.getServiceType().c_str());
-
- return true;
-}
-
-bool
-PUPnP::actionAddPortMapping(const UPnPIGD& igd,
- const Mapping& mapping,
- UPnPProtocol::UpnpError& error_code)
-{
- if (not clientRegistered_)
- return false;
-
- error_code = UPnPProtocol::UpnpError::ERROR_OK;
-
- // Action and response pointers.
- XMLDocument action(nullptr, ixmlDocument_free); // Action pointer.
- XMLDocument response(nullptr, ixmlDocument_free); // Response pointer.
+ XMLDocument action(nullptr, ixmlDocument_free);
IXML_Document* action_container_ptr = nullptr;
+ XMLDocument response(nullptr, ixmlDocument_free);
IXML_Document* response_container_ptr = nullptr;
- // Set action name.
- std::string action_name {"AddPortMapping"};
-
// Set action sequence.
UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
+ ACTION_ADD_PORT_MAPPING,
+ igd->getServiceType().c_str(),
"NewRemoteHost",
"");
UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
+ ACTION_ADD_PORT_MAPPING,
+ igd->getServiceType().c_str(),
"NewExternalPort",
mapping.getExternalPortStr().c_str());
UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
+ ACTION_ADD_PORT_MAPPING,
+ igd->getServiceType().c_str(),
"NewProtocol",
mapping.getTypeStr());
UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
+ ACTION_ADD_PORT_MAPPING,
+ igd->getServiceType().c_str(),
"NewInternalPort",
mapping.getInternalPortStr().c_str());
UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
+ ACTION_ADD_PORT_MAPPING,
+ igd->getServiceType().c_str(),
"NewInternalClient",
getHostAddress().toString().c_str());
UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
+ ACTION_ADD_PORT_MAPPING,
+ igd->getServiceType().c_str(),
"NewEnabled",
"1");
UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
+ ACTION_ADD_PORT_MAPPING,
+ igd->getServiceType().c_str(),
"NewPortMappingDescription",
mapping.toString().c_str());
UpnpAddToAction(&action_container_ptr,
- action_name.c_str(),
- igd.getServiceType().c_str(),
+ ACTION_ADD_PORT_MAPPING,
+ igd->getServiceType().c_str(),
"NewLeaseDuration",
"0");
action.reset(action_container_ptr);
int upnp_err = UpnpSendAction(ctrlptHandle_,
- igd.getControlURL().c_str(),
- igd.getServiceType().c_str(),
+ igd->getControlURL().c_str(),
+ igd->getServiceType().c_str(),
nullptr,
action.get(),
&response_container_ptr);
+ response.reset(response_container_ptr);
+
if (upnp_err != UPNP_E_SUCCESS) {
JAMI_WARN("PUPnP: Failed to send action %s from: %s, %d: %s",
- action_name.c_str(),
- igd.getServiceType().c_str(),
+ ACTION_ADD_PORT_MAPPING,
+ igd->getServiceType().c_str(),
upnp_err,
UpnpGetErrorMessage(upnp_err));
- error_code = UPnPProtocol::UpnpError::INVALID_ERR;
return false;
}
- if (not response_container_ptr) {
- JAMI_WARN("PUPnP: Failed to get response from %s", action_name.c_str());
+ if (not response) {
+ JAMI_WARN("PUPnP: Failed to get response from %s", ACTION_ADD_PORT_MAPPING);
return false;
}
- response.reset(response_container_ptr);
// Check if there is an error code.
std::string errorCode = getFirstDocItem(response.get(), "errorCode");
if (not errorCode.empty()) {
std::string errorDescription = getFirstDocItem(response.get(), "errorDescription");
JAMI_WARN("PUPnP: %s returned with error: %s: %s",
- action_name.c_str(),
+ ACTION_ADD_PORT_MAPPING,
errorCode.c_str(),
errorDescription.c_str());
- error_code = UPnPProtocol::UpnpError::INVALID_ERR;
return false;
}
+
+ JAMI_DBG("PUPnP: Sent request to open port %s", mapping.toString().c_str());
+
+ // Process the response on the main thread.
+ runOnUpnpContextThread([w = weak(), mapping, igd] {
+ auto upnpThis = w.lock();
+ if (not upnpThis)
+ return;
+ upnpThis->processAddMapAction(igd->getControlURL(),
+ mapping.getExternalPort(),
+ mapping.getInternalPort(),
+ mapping.getType());
+ });
+
return true;
}
@@ -1563,5 +1650,86 @@ PUPnP::actionDeletePortMappingAsync(const UPnPIGD& igd,
return true;
}
+bool
+PUPnP::actionDeletePortMapping(const std::shared_ptr<UPnPIGD>& igd, const Mapping& mapping)
+{
+ if (not clientRegistered_) {
+ return false;
+ }
+
+ // Action and response pointers.
+ XMLDocument action(nullptr, ixmlDocument_free);
+ IXML_Document* action_container_ptr = nullptr;
+ XMLDocument response(nullptr, ixmlDocument_free);
+ IXML_Document* response_container_ptr = nullptr;
+
+ // Set action sequence.
+ UpnpAddToAction(&action_container_ptr,
+ ACTION_DELETE_PORT_MAPPING,
+ igd->getServiceType().c_str(),
+ "NewRemoteHost",
+ "");
+ UpnpAddToAction(&action_container_ptr,
+ ACTION_DELETE_PORT_MAPPING,
+ igd->getServiceType().c_str(),
+ "NewExternalPort",
+ mapping.getExternalPortStr().c_str());
+ UpnpAddToAction(&action_container_ptr,
+ ACTION_DELETE_PORT_MAPPING,
+ igd->getServiceType().c_str(),
+ "NewProtocol",
+ mapping.getTypeStr());
+
+ action.reset(action_container_ptr);
+
+ int upnp_err = UpnpSendAction(ctrlptHandle_,
+ igd->getControlURL().c_str(),
+ igd->getServiceType().c_str(),
+ nullptr,
+ action.get(),
+ &response_container_ptr);
+ response.reset(response_container_ptr);
+
+ if (upnp_err != UPNP_E_SUCCESS) {
+ JAMI_WARN("PUPnP: Failed to send action %s from: %s, %d: %s",
+ ACTION_DELETE_PORT_MAPPING,
+ igd->getServiceType().c_str(),
+ upnp_err,
+ UpnpGetErrorMessage(upnp_err));
+ return false;
+ }
+
+ if (not response) {
+ JAMI_WARN("PUPnP: Failed to get response from %s", ACTION_DELETE_PORT_MAPPING);
+ return false;
+ }
+
+ // Check if there is an error code.
+ std::string errorCode = getFirstDocItem(response.get(), "errorCode");
+ if (not errorCode.empty()) {
+ std::string errorDescription = getFirstDocItem(response.get(), "errorDescription");
+ JAMI_WARN("PUPnP: %s returned with error: %s: %s",
+ ACTION_DELETE_PORT_MAPPING,
+ errorCode.c_str(),
+ errorDescription.c_str());
+ return false;
+ }
+
+ JAMI_DBG("PUPnP: Sent request to close port %s", mapping.toString().c_str());
+
+ // Process the response on the main thread.
+ runOnUpnpContextThread([w = weak(), mapping, igd] {
+ auto upnpThis = w.lock();
+ if (not upnpThis)
+ return;
+ upnpThis->processRemoveMapAction(igd->getControlURL(),
+ mapping.getExternalPort(),
+ mapping.getInternalPort(),
+ mapping.getType());
+ });
+
+ return true;
+}
+
} // namespace upnp
} // namespace jami
diff --git a/src/upnp/protocol/pupnp/pupnp.h b/src/upnp/protocol/pupnp/pupnp.h
index 3290bab6ae..e490f5ffd6 100644
--- a/src/upnp/protocol/pupnp/pupnp.h
+++ b/src/upnp/protocol/pupnp/pupnp.h
@@ -69,7 +69,6 @@ constexpr static int CONFLICT_IN_MAPPING = 718;
// Timeout values (in seconds).
constexpr static unsigned int SEARCH_TIMEOUT {60};
-constexpr static unsigned int SUBSCRIBE_TIMEOUT {300};
// Max number of IGD search attempts before failure.
constexpr static unsigned int PUPNP_MAX_RESTART_SEARCH_RETRIES {5};
// Time-out between two successive IGD search.
@@ -98,15 +97,9 @@ public:
PUPnP();
~PUPnP();
- bool initUpnpLib();
- bool registerClient();
-
// Set the observer
void setObserver(UpnpMappingObserver* obs) override;
- // Start search for UPNP devices
- void searchForDevices();
-
// Returns the protocol type.
NatProtocolType getProtocol() const override { return NatProtocolType::PUPNP; }
@@ -142,9 +135,25 @@ public:
// Removes a mapping.
void requestMappingRemove(const Mapping& igdMapping) override;
+ void terminate() override;
+
private:
+ NON_COPYABLE(PUPnP);
+
ScheduledExecutor* getUpnContextScheduler() { return UpnpThreadUtil::getScheduler(); }
+ // Init lib-upnp
+ void initUpnpLib();
+
+ // Register the client
+ void registerClient();
+
+ // Start the internal thread.
+ void startPUPnP();
+
+ // Start search for UPNP devices
+ void searchForDevices();
+
// Return true if it has at least one valid IGD.
bool hasValidIgd() const;
@@ -168,12 +177,12 @@ private:
uint16_t iPort,
PortType portType);
- // Returns control point action callback based on xml node.
- CtrlAction getAction(const char* xmlNode);
-
// Validate IGD from the xml document received from the router.
bool validateIgd(const IGDInfo&);
+ // Returns control point action callback based on xml node.
+ static CtrlAction getAction(const char* xmlNode);
+
// Control point callback.
static int ctrlPtCallback(Upnp_EventType event_type, const void* event, void* user_data);
#if UPNP_VERSION < 10800
@@ -187,7 +196,8 @@ private:
const std::string& igdUrl,
const IpAddr& dstAddr);
void processDiscoveryAdvertisementByebye(const std::string& deviceId);
- void processDiscoverySubscriptionExpired(const std::string& eventSubUrl);
+ void processDiscoverySubscriptionExpired(Upnp_EventType event_type,
+ const std::string& eventSubUrl);
// Callback event handler function for the UPnP client (control point).
int handleCtrlPtUPnPEvents(Upnp_EventType event_type, const void* event);
@@ -202,7 +212,7 @@ private:
#endif
// Callback subscription event function for handling subscription request.
- int handleSubscriptionUPnPEvent(Upnp_EventType /*unused*/, const void* event);
+ int handleSubscriptionUPnPEvent(Upnp_EventType event_type, const void* event);
// Parses the IGD candidate.
std::unique_ptr<UPnPIGD> parseIgd(IXML_Document* doc, std::string locationUrl);
@@ -212,30 +222,24 @@ private:
bool actionIsIgdConnected(const UPnPIGD& igd);
IpAddr actionGetExternalIP(const UPnPIGD& igd);
- bool actionDeletePortMapping(const UPnPIGD& igd,
- const std::string& port_external,
- const std::string& protocol);
- bool actionAddPortMapping(const UPnPIGD& igd,
- const Mapping& mapping,
- UPnPProtocol::UpnpError& upnp_error);
- bool actionAddPortMappingAsync(const UPnPIGD& igd, const Mapping& mapping);
+ bool actionAddPortMapping(const std::shared_ptr<UPnPIGD>& igd, const Mapping& mapping);
+ bool actionAddPortMappingAsync(const std::shared_ptr<UPnPIGD>& igd, const Mapping& mapping);
bool actionDeletePortMappingAsync(const UPnPIGD& igd,
const std::string& port_external,
const std::string& protocol);
+ bool actionDeletePortMapping(const std::shared_ptr<UPnPIGD>& igd, const Mapping& mapping);
// Event type to string
static const char* eventTypeToString(Upnp_EventType eventType);
-private:
- // Indicates of the client is registered. Since at most only one instance
- // of this class is created, using a static is unambiguous.
- std::atomic_bool clientRegistered_ {false};
-
std::weak_ptr<PUPnP> weak() { return std::static_pointer_cast<PUPnP>(shared_from_this()); }
- NON_COPYABLE(PUPnP);
+ // Initialization status.
+ std::atomic_bool initialized_ {false};
+ // Client registration status.
+ std::atomic_bool clientRegistered_ {false};
std::condition_variable pupnpCv_ {}; // Condition variable for thread-safe signaling.
- std::atomic_bool pupnpRun_ {true}; // Variable to allow the thread to run.
+ std::atomic_bool pupnpRun_ {false}; // Variable to allow the thread to run.
std::thread pupnpThread_ {}; // PUPnP thread for non-blocking client registration.
std::shared_ptr<Task> searchForIgdTimer_ {};
unsigned int igdSearchCounter_ {0};
@@ -248,8 +252,7 @@ private:
dwnldlXmlList_; // List of futures for blocking xml download function calls.
std::list<std::future<pIGDInfo>> cancelXmlList_; // List of abandoned documents
- mutable std::mutex igdListMutex_; // Mutex used to protect IGD instances.
-
+ mutable std::mutex igdListMutex_; // Mutex used to protect IGD instances.
std::mutex ctrlptMutex_; // Mutex for client handle protection.
UpnpClient_Handle ctrlptHandle_ {-1}; // Control point handle.
diff --git a/src/upnp/protocol/upnp_protocol.h b/src/upnp/protocol/upnp_protocol.h
index eed6cf00f6..ce229b2f13 100644
--- a/src/upnp/protocol/upnp_protocol.h
+++ b/src/upnp/protocol/upnp_protocol.h
@@ -124,10 +124,13 @@ public:
// Get the host (local) address.
const IpAddr& getHostAddress() const { return hostAddress_; }
+ // Terminate
+ virtual void terminate() = 0;
+
protected:
// The host (local) address. Must be fully set before making any request.
IpAddr hostAddress_ {};
- UpnpMappingObserver* observer_;
+ UpnpMappingObserver* observer_ {nullptr};
};
} // namespace upnp
diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp
index e52f86bc10..f4031a92db 100644
--- a/src/upnp/upnp_context.cpp
+++ b/src/upnp/upnp_context.cpp
@@ -35,14 +35,6 @@ constexpr static uint16_t UPNP_TCP_PORT_MAX {UPNP_TCP_PORT_MIN + 5000};
constexpr static uint16_t UPNP_UDP_PORT_MIN {20000};
constexpr static uint16_t UPNP_UDP_PORT_MAX {UPNP_UDP_PORT_MIN + 5000};
-std::shared_ptr<UPnPContext>
-UPnPContext::getUPnPContext()
-{
- // This is the unique shared instance (singleton) of UPnPContext class.
- static auto context = std::make_shared<UPnPContext>();
- return context;
-}
-
UPnPContext::UPnPContext()
{
JAMI_DBG("Creating UPnPContext instance [%p]", this);
@@ -57,15 +49,39 @@ UPnPContext::UPnPContext()
}
}
-UPnPContext::~UPnPContext()
+std::shared_ptr<UPnPContext>
+UPnPContext::getUPnPContext()
{
+ // This is the unique shared instance (singleton) of UPnPContext class.
+ static auto context = std::make_shared<UPnPContext>();
+ return context;
+}
+
+void
+UPnPContext::shutdown()
+{
+ if (not isValidThread()) {
+ runOnUpnpContextThread([this] { shutdown(); });
+ return;
+ }
+
+ JAMI_DBG("Shutdown UPnPContext instance [%p]", this);
+
stopUpnp(true);
+ for (auto const& [_, proto] : protocolList_)
+ proto->terminate();
+
std::lock_guard<std::mutex> lock(mappingMutex_);
mappingList_->clear();
- mappingListUpdateTimer_->cancel();
+ if (mappingListUpdateTimer_)
+ mappingListUpdateTimer_->cancel();
controllerList_.clear();
+ protocolList_.clear();
+}
+UPnPContext::~UPnPContext()
+{
JAMI_DBG("UPnPContext instance [%p] destroyed", this);
}
@@ -113,7 +129,7 @@ UPnPContext::stopUpnp(bool forceRelease)
return;
}
- JAMI_DBG("Stoping UPNP context");
+ JAMI_DBG("Stopping UPNP context");
// Clear all current mappings if any.
@@ -186,8 +202,6 @@ UPnPContext::connectivityChanged()
return;
}
- CHECK_VALID_THREAD();
-
if (controllerList_.empty())
return;
@@ -297,8 +311,6 @@ UPnPContext::releaseMapping(const Mapping& map)
return;
}
- CHECK_VALID_THREAD();
-
auto mapPtr = getMappingWithKey(map.getMapKey());
if (not mapPtr) {
@@ -327,11 +339,11 @@ UPnPContext::registerController(void* controller)
auto ret = controllerList_.emplace(controller);
if (not ret.second) {
- JAMI_WARN("Controller %p is already registered", this);
+ JAMI_WARN("Controller %p is already registered", controller);
return;
}
- JAMI_DBG("Successfully registered controller %p", this);
+ JAMI_DBG("Successfully registered controller %p", controller);
if (not started_)
startUpnp();
}
@@ -345,13 +357,14 @@ UPnPContext::unregisterController(void* controller)
}
if (controllerList_.erase(controller) == 1) {
- JAMI_DBG("Successfully unregistered controller %p", this);
+ JAMI_DBG("Successfully unregistered controller %p", controller);
} else {
- JAMI_ERR("Trying to unregister an unknown controller %p", this);
+ JAMI_DBG("Controller %p was already removed", controller);
}
- if (controllerList_.empty())
+ if (controllerList_.empty()) {
stopUpnp();
+ }
}
uint16_t
@@ -385,8 +398,6 @@ UPnPContext::requestMapping(const Mapping::sharedPtr_t& map)
return;
}
- CHECK_VALID_THREAD();
-
auto const& igd = getPreferredIgd();
// We must have at least a valid IGD pointer if we get here.
// Not this method is called only if there were a valid IGD, however,
@@ -836,8 +847,6 @@ UPnPContext::onIgdUpdated(const std::shared_ptr<IGD>& igd, UpnpIgdEvent event)
return;
}
- CHECK_VALID_THREAD();
-
char const* IgdState = event == UpnpIgdEvent::ADDED
? "ADDED"
: event == UpnpIgdEvent::REMOVED ? "REMOVED" : "INVALID";
@@ -1023,8 +1032,6 @@ UPnPContext::deleteAllMappings(PortType type)
return;
}
- CHECK_VALID_THREAD();
-
std::lock_guard<std::mutex> lock(mappingMutex_);
auto& mappingList = getMappingList(type);
@@ -1044,8 +1051,6 @@ UPnPContext::onMappingRemoved(const std::shared_ptr<IGD>& igd, const Mapping& ma
return;
}
- CHECK_VALID_THREAD();
-
auto map = getMappingWithKey(mapRes.getMapKey());
// Notify the listener.
if (map and map->getNotifyCallback())
diff --git a/src/upnp/upnp_context.h b/src/upnp/upnp_context.h
index d6879ff8c9..6ce476a03e 100644
--- a/src/upnp/upnp_context.h
+++ b/src/upnp/upnp_context.h
@@ -90,9 +90,12 @@ public:
UPnPContext();
~UPnPContext();
- // Retrieve the UPnPContext singleton
+ // Retrieve the UPnPContext singleton.
static std::shared_ptr<UPnPContext> getUPnPContext();
+ // Terminate the instance.
+ void shutdown();
+
// Set the known public address
void setPublicAddress(const IpAddr& addr);
--
GitLab