From a7694ef8b36f14d24c4e70be04353f4e99bb32ea Mon Sep 17 00:00:00 2001 From: Dunedan Date: Mon, 24 Feb 2025 08:23:07 +0100 Subject: [PATCH] Fix adding port forwardings using UPnP The UPnP implementation included a combination of two subtle bugs, which resulted in failure to create port forwardings every time after the first one. When using UPnP, the internet gateway to create the port forwardings at needs to discovered. As that takes a while, the its root descriptor URL was supposed to be cached after successful discovery in the user config in "network.upnprootdescurl". However, instead of caching the root descriptor URL, the control URL got cached. That caused following requests to the root descriptor URL to fail, as they ended up at the control URL instead. As such requests might also fail when the network topology changed, the code was supposed to fall back to discovering the internet gateway again when the cached one didn't work. However, due to the inner workings of miniupnpc the request using the cached root descriptor URL didn't result in an error, so the new discovery was never triggered. As the wrong value was persisted in the user config there was also no way to get out of this situation again. This commit fixes both of these bugs. As far as I can tell these bugs existed since the introduction of the caching of the root descriptor URL in 0ba25e9968, which means creating port forwardings using UPnP has been broken since Alpha 15. (cherry picked from commit 18d7746c84633e66bf8821fd0ee29f2c78c3e47f) Signed-off-by: Itms --- source/network/NetServer.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/network/NetServer.cpp b/source/network/NetServer.cpp index f3753c2481..b3706530a6 100644 --- a/source/network/NetServer.cpp +++ b/source/network/NetServer.cpp @@ -242,7 +242,7 @@ void CNetServerWorker::SetupUPnP() int ret = 0; // Try a cached URL first - if (!rootDescURL.empty() && UPNP_GetIGDFromUrl(rootDescURL.c_str(), &urls, &data, internalIPAddress, sizeof(internalIPAddress))) + if (!rootDescURL.empty() && UPNP_GetIGDFromUrl(rootDescURL.c_str(), &urls, &data, internalIPAddress, sizeof(internalIPAddress)) && strlen(data.first.controlurl) != 0) { LOGMESSAGE("Net server: using cached IGD = %s", urls.controlURL); ret = 1; @@ -339,9 +339,9 @@ void CNetServerWorker::SetupUPnP() externalIPAddress, psPort, protocall, intClient, intPort, duration); // Cache root descriptor URL to try to avoid discovery next time. - g_ConfigDB.SetValueString(CFG_USER, "network.upnprootdescurl", urls.controlURL); - g_ConfigDB.WriteValueToFile(CFG_USER, "network.upnprootdescurl", urls.controlURL); - LOGMESSAGE("Net server: cached UPnP root descriptor URL as %s", urls.controlURL); + g_ConfigDB.SetValueString(CFG_USER, "network.upnprootdescurl", urls.rootdescURL); + g_ConfigDB.WriteValueToFile(CFG_USER, "network.upnprootdescurl", urls.rootdescURL); + LOGMESSAGE("Net server: cached UPnP root descriptor URL as %s", urls.rootdescURL); freeUPnP(); }