diff --git a/Core/HLE/sceNet.cpp b/Core/HLE/sceNet.cpp index cfa63699cb..66bf63628c 100644 --- a/Core/HLE/sceNet.cpp +++ b/Core/HLE/sceNet.cpp @@ -626,6 +626,10 @@ void __NetInit() { #ifdef __LIBRETRO__ __UPnPInit(2000); #endif + // The UPnP service thread only exists while UPnP is enabled, and a per-game config (or a + // libretro core option) can turn it on after startup. Kick it here so discovery is already + // done by the time the game binds its first socket, rather than starting on that request. + UPnP_Notify(); __ResetInitNetLib(); __NetApctlInit(); diff --git a/Core/Util/PortManager.cpp b/Core/Util/PortManager.cpp index ba8f066a1a..7a02b02690 100644 --- a/Core/Util/PortManager.cpp +++ b/Core/Util/PortManager.cpp @@ -4,12 +4,6 @@ // it under the terms of the GNU General Public License as published by // the Free Software Foundation, version 2.0 or later versions. -// Copyright (c) 2012- PPSSPP Project. - -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, version 2.0 or later versions. - // This program is distributed in the hope that it will be useful, // but WITHOUT ANY WARRANTY; without even the implied warranty of // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the @@ -25,6 +19,12 @@ // Most of the code are based on https://github.com/RJ/libportfwd and updated to the latest miniupnp library // All credit goes to him and the official miniupnp project! http://miniupnp.free.fr/ +// Threading model: everything in PortManager runs on the UPnP service thread, which exists only +// while the UPnP setting is on. Other threads only ever push requests onto g_upnpReqs +// (UPnP_Add/UPnP_Remove) or poke the condition variable (UPnP_Notify). Talking to a router means +// blocking socket I/O with multi-second timeouts, so none of it may happen on a thread anyone +// waits for - and the thread must never end up in a state where it can't be told to stop. + #include // find_if #include #include @@ -46,254 +46,263 @@ #include "Core/Util/PortManager.h" PortManager g_PortManager; + +// Guards g_upnpServiceThread itself, so that a start racing a start (or a shutdown) can't end up +// with two service threads or a std::thread being reassigned while it's still running. +static std::mutex g_upnpThreadLock; static std::thread g_upnpServiceThread; +static bool g_upnpInitialized; + static std::mutex g_upnpLock; static std::condition_variable g_upnpCond; -static std::deque g_upnpReqs; -void PortManager::Shutdown() { - Clear(); - Restore(); +// All of the following are guarded by g_upnpLock. +static std::deque g_upnpReqs; +static unsigned int g_upnpTimeout = 2000; +static bool g_upnpExit; +// Whether a service thread is up. The thread only exists while the setting is on, so most users +// never pay for one at all - it used to be started unconditionally and parked for the session. +static bool g_upnpThreadRunning; +// Bumped whenever there's a reason for the service thread to wake up and look around. The thread +// snapshots it before doing (slow) work, so a request that arrives while it's busy can't be missed +// and leave it asleep with something queued. +static uint32_t g_upnpWakeSeq; +// Set by UPnP_Notify() - the user flipped a setting, so retry immediately instead of continuing to +// back off from earlier failures. +static bool g_upnpResetBackoff; + +// A game hammering bind() shouldn't be able to grow the queue without bound, especially while +// UPnP is unreachable and nothing is being drained. +static constexpr size_t MAX_QUEUED_REQUESTS = 64; +// After this many failed round trips we drop a request rather than let it block the queue forever. +static constexpr int MAX_REQUEST_ATTEMPTS = 3; +// Backoff bounds for rediscovering a router. Without a cap this used to redo a full SSDP discovery +// every five seconds, forever, for anyone who enabled UPnP without a router that supports it. +static constexpr double MIN_RETRY_SECONDS = 5.0; +static constexpr double MAX_RETRY_SECONDS = 300.0; +// Some routers do not support non-zero lease durations, in which case we fall back to 0 (permanent). +static const char * const DEFAULT_LEASE_DURATION = "43200"; // 12 hours, range is 0-604800 (0 = indefinite) + +#ifdef WITH_UPNP +// Real routers report an error once the index runs past the end of the mapping table. This is just +// a backstop against one that never does - walking to 65536 would mean 65536 HTTP round trips. +static constexpr int MAX_PORT_MAPPINGS = 1024; + +// Sizes miniupnpc copies into these buffers (see the strncpy calls in upnpcommands.c). It does not +// guarantee a terminating NUL, so we give every buffer one spare byte and zero it before each call. +struct PortMappingEntry { + char extPort[6 + 1]; + char intClient[16 + 1]; + char intPort[6 + 1]; + char protocol[4 + 1]; + char desc[80 + 1]; + char enabled[4 + 1]; + char rHost[64 + 1]; + char duration[16 + 1]; +}; +#endif + +static void ShowUPnPMessage(const char *key) { + auto n = GetI18NCategory(I18NCat::NETWORKING); + g_OSD.Show(OSDType::MESSAGE_INFO, n->T(key), 0.0f, "upnp_warning"); +} + +void PortManager::Shutdown(double budgetSeconds) { + if (m_InitState == UPNP_INITSTATE_DONE) { + m_deadline = time_now_d() + budgetSeconds; + Clear(); + Restore(); + m_deadline = 0.0; + } Terminate(); } void PortManager::Terminate() { DEBUG_LOG(Log::Net, "PortManager::Terminate()"); - if (urls) { #ifdef WITH_UPNP - FreeUPNPUrls(urls); + if (m_urlsValid) { + FreeUPNPUrls(&m_urls); + m_urlsValid = false; + } #endif - free(urls); - urls = NULL; - } - if (datas) { - free(datas); - datas = NULL; - } + memset(&m_urls, 0, sizeof(m_urls)); + memset(&m_datas, 0, sizeof(m_datas)); m_otherPortList.clear(); m_otherPortList.shrink_to_fit(); m_portList.clear(); m_portList.shrink_to_fit(); m_lanip.clear(); - m_defaultDesc.clear(); - m_leaseDuration.clear(); + m_leaseDuration = DEFAULT_LEASE_DURATION; m_LocalPort = UPNP_LOCAL_PORT_ANY; + m_deadline = 0.0; m_InitState = UPNP_INITSTATE_NONE; } -bool PortManager::Initialize(const unsigned int timeout) { +bool PortManager::HaveControlURL() const { + return m_urlsValid && m_urls.controlURL && m_urls.controlURL[0] != '\0'; +} + +bool PortManager::OutOfTime() const { + return m_deadline != 0.0 && time_now_d() > m_deadline; +} + +bool PortManager::Initialize(unsigned int timeout) { #ifdef WITH_UPNP - // Windows: Assuming WSAStartup already called beforehand - struct UPNPDev* devlist; - struct UPNPDev* dev; - char* descXML; - int descXMLsize = 0; - int descXMLstatus = 0; - int localport = m_LocalPort; // UPNP_LOCAL_PORT_ANY (0), or UPNP_LOCAL_PORT_SAME (1) as an alias for 1900 (for backwards compatability?) - int ipv6 = 0; // 0 = IPv4, 1 = IPv6 - unsigned char ttl = 2; // defaulting to 2 - int error = 0; - DEBUG_LOG(Log::Net, "PortManager::Initialize(%d)", timeout); - if (!g_Config.bEnableUPnP) { - ERROR_LOG(Log::Net, "PortManager::Initialize - UPnP is Disabled on Networking Settings"); - return false; - } - - if (m_InitState != UPNP_INITSTATE_NONE) { - switch (m_InitState) - { - case UPNP_INITSTATE_BUSY: { - WARN_LOG(Log::Net, "PortManager - Initialization already in progress"); - return false; - } - // Should we redetect UPnP? just in case the player switched to a different network in the middle - case UPNP_INITSTATE_DONE: { - WARN_LOG(Log::Net, "PortManager - Already Initialized"); - return true; - } - default: - break; - } - } - - m_leaseDuration = "43200"; // 12 hours - m_InitState = UPNP_INITSTATE_BUSY; - urls = (UPNPUrls*)malloc(sizeof(struct UPNPUrls)); - if (!urls) - return false; - datas = (IGDdatas*)malloc(sizeof(struct IGDdatas)); - if (!datas) { - free(urls); - return false; - } - memset(urls, 0, sizeof(struct UPNPUrls)); - memset(datas, 0, sizeof(struct IGDdatas)); - - devlist = upnpDiscover(timeout, NULL, NULL, localport, ipv6, ttl, &error); - if (devlist) - { - dev = devlist; - while (dev) - { - if (strstr(dev->st, "InternetGatewayDevice")) - break; - dev = dev->pNext; - } - if (!dev) - dev = devlist; // defaulting to first device - - INFO_LOG(Log::Net, "PortManager - UPnP device: [desc: %s] [st: %s]", dev->descURL, dev->st); - - descXML = (char*)miniwget(dev->descURL, &descXMLsize, dev->scope_id, &descXMLstatus); - if (descXML) - { - parserootdesc(descXML, descXMLsize, datas); - free(descXML); descXML = 0; - GetUPNPUrls(urls, datas, dev->descURL, dev->scope_id); - } - - // Get LAN IP address that connects to the router - char lanaddr[64] = "unset"; - - // possible "status" values: - // -1 = Internal error - // 0 = NO IGD found - // 1 = A valid connected IGD has been found - // 2 = A valid connected IGD has been found but its IP address is reserved (non routable) - // 3 = A valid IGD has been found but it reported as not connected - // 4 = an UPnP device has been found but was not recognized as an IGD -#if (MINIUPNPC_API_VERSION >= 18) - int status = UPNP_GetValidIGD(devlist, urls, datas, lanaddr, sizeof(lanaddr), nullptr, 0); -#else - int status = UPNP_GetValidIGD(devlist, urls, datas, lanaddr, sizeof(lanaddr)); -#endif - m_lanip = std::string(lanaddr); - INFO_LOG(Log::Net, "PortManager - Detected LAN IP: %s (status=%d)", m_lanip.c_str(), status); - - // Additional Info - char connectionType[64] = ""; - if (UPNP_GetConnectionTypeInfo(urls->controlURL, datas->first.servicetype, connectionType) != UPNPCOMMAND_SUCCESS) { - WARN_LOG(Log::Net, "PortManager - GetConnectionTypeInfo failed"); - } - else { - INFO_LOG(Log::Net, "PortManager - Connection Type: %s", connectionType); - } - - // Using Game ID & Player Name as default description for mapping - if (PSP_IsInited()) { - std::string gameID = g_paramSFO.GetDiscID(); - m_defaultDesc = "PPSSPP:" + gameID + ":" + g_Config.sNickName; // Some routers may automatically prefixed it with "UPnP:" - } else { - m_defaultDesc = "PPSSPP:at_menu:" + g_Config.sNickName; - } - - freeUPNPDevlist(devlist); - - //m_LocalPort = localport; // We shouldn't keep the right port for the next game reset if we wanted to redetect UPnP - m_InitState = UPNP_INITSTATE_DONE; - RefreshPortList(); + if (m_InitState == UPNP_INITSTATE_DONE) { return true; } - ERROR_LOG(Log::Net, "PortManager - upnpDiscover failed (error: %i) or No UPnP device detected", error); - if (g_Config.bEnableUPnP) { - auto n = GetI18NCategory(I18NCat::NETWORKING); - g_OSD.Show(OSDType::MESSAGE_ERROR, StringFromFormat("%s (%d)", n->T_cstr("Unable to find UPnP device"), error), 0.0f, "upnp_warning"); - } - m_InitState = UPNP_INITSTATE_NONE; -#endif // WITH_UPNP - return false; -} + // A previous failed attempt can leave allocations and half-filled structs behind, so always + // start from a clean slate. This used to leak a whole UPNPUrls/IGDdatas pair per attempt. + Terminate(); + m_InitState = UPNP_INITSTATE_BUSY; -int PortManager::GetInitState() { - return m_InitState; -} - -bool PortManager::Add(const char* protocol, unsigned short port, unsigned short intport) { -#ifdef WITH_UPNP - char port_str[16]; - char intport_str[16]; - int r; - auto n = GetI18NCategory(I18NCat::NETWORKING); - - if (intport == 0) - intport = port; - INFO_LOG(Log::Net, "PortManager::Add(%s, %d, %d)", protocol, port, intport); - if (urls == NULL || urls->controlURL == NULL || urls->controlURL[0] == '\0') - { - if (g_Config.bEnableUPnP) { - WARN_LOG(Log::Net, "PortManager::Add - the init was not done !"); - g_OSD.Show(OSDType::MESSAGE_INFO, n->T("UPnP need to be reinitialized")); - } - Terminate(); + int error = 0; + // m_LocalPort is UPNP_LOCAL_PORT_ANY (0), or UPNP_LOCAL_PORT_SAME (1) as an alias for 1900. + const int ipv6 = 0; + const unsigned char ttl = 2; + UPNPDev *devlist = upnpDiscover(timeout, nullptr, nullptr, m_LocalPort, ipv6, ttl, &error); + if (!devlist) { + ERROR_LOG(Log::Net, "PortManager - upnpDiscover failed (error: %i) or no UPnP device detected", error); + m_InitState = UPNP_INITSTATE_NONE; return false; } - snprintf(port_str, sizeof(port_str), "%d", port); - snprintf(intport_str, sizeof(intport_str), "%d", intport); - // Only add new port map if it's not previously created by PPSSPP for current IP - auto el_it = std::find_if(m_portList.begin(), m_portList.end(), - [port_str, protocol](const std::pair &el) { return el.first == port_str && el.second == protocol; }); - if (el_it == m_portList.end()) { - auto el_it = std::find_if(m_otherPortList.begin(), m_otherPortList.end(), - [port_str, protocol](const PortMap& el) { return el.extPort_str == port_str && el.protocol == protocol; }); - if (el_it != m_otherPortList.end()) { - // Try to delete the port mapping before we create it, just in case we have dangling port mapping from the daemon not being shut down correctly or the port was taken by other - r = UPNP_DeletePortMapping(urls->controlURL, datas->first.servicetype, port_str, protocol, NULL); - } - r = UPNP_AddPortMapping(urls->controlURL, datas->first.servicetype, - port_str, intport_str, m_lanip.c_str(), m_defaultDesc.c_str(), protocol, NULL, m_leaseDuration.c_str()); - if (r == 725 && m_leaseDuration != "0") { - m_leaseDuration = "0"; - r = UPNP_AddPortMapping(urls->controlURL, datas->first.servicetype, - port_str, intport_str, m_lanip.c_str(), m_defaultDesc.c_str(), protocol, NULL, m_leaseDuration.c_str()); - } - if (r != 0) - { - ERROR_LOG(Log::Net, "PortManager - AddPortMapping failed (error: %i)", r); - if (r == UPNPCOMMAND_HTTP_ERROR) { - if (g_Config.bEnableUPnP) { - g_OSD.Show(OSDType::MESSAGE_INFO, n->T("UPnP need to be reinitialized")); - } - Terminate(); // Most of the time errors occurred because the router is no longer reachable (ie. changed networks) so we should invalidate the state to prevent further lags due to timeouts - return false; - } - } - m_portList.push_front({ port_str, protocol }); - // Keep tracks of it to be restored later if it belongs to others - if (el_it != m_otherPortList.end()) el_it->taken = true; + for (const UPNPDev *dev = devlist; dev; dev = dev->pNext) { + INFO_LOG(Log::Net, "PortManager - UPnP device: [desc: %s] [st: %s]", dev->descURL, dev->st); } + + // UPNP_GetValidIGD downloads and parses the root descriptions itself and fills in both structs, + // so there's no need to miniwget/parserootdesc/GetUPNPUrls up front - doing that only cost an + // extra HTTP round trip to the router and leaked the URLs, since GetValidIGD memsets over them. + // + // possible "status" values: + // -1 = Internal error + // 0 = NO IGD found + // 1 = A valid connected IGD has been found + // 2 = A valid connected IGD has been found but its IP address is reserved (non routable) + // 3 = A valid IGD has been found but it reported as not connected + // 4 = an UPnP device has been found but was not recognized as an IGD + char lanaddr[64] = ""; +#if (MINIUPNPC_API_VERSION >= 18) + const int status = UPNP_GetValidIGD(devlist, &m_urls, &m_datas, lanaddr, sizeof(lanaddr), nullptr, 0); +#else + const int status = UPNP_GetValidIGD(devlist, &m_urls, &m_datas, lanaddr, sizeof(lanaddr)); +#endif + lanaddr[sizeof(lanaddr) - 1] = '\0'; + freeUPNPDevlist(devlist); + m_urlsValid = status >= 1; + + // Without a control URL there's nothing we can ask the router to do, and pressing on would just + // mean handing NULL to every UPNP_* call below. + if (!HaveControlURL()) { + ERROR_LOG(Log::Net, "PortManager - no usable IGD found (status=%d)", status); + Terminate(); + return false; + } + if (status != 1) { + WARN_LOG(Log::Net, "PortManager - IGD found but in an unexpected state (status=%d), trying anyway", status); + } + + m_lanip = lanaddr; + INFO_LOG(Log::Net, "PortManager - Detected LAN IP: %s (status=%d)", m_lanip.c_str(), status); + + // miniupnpc writes up to 64 bytes here and doesn't guarantee a terminator, hence the spare byte. + char connectionType[64 + 1] = ""; + if (UPNP_GetConnectionTypeInfo(m_urls.controlURL, m_datas.first.servicetype, connectionType) != UPNPCOMMAND_SUCCESS) { + WARN_LOG(Log::Net, "PortManager - GetConnectionTypeInfo failed"); + } else { + INFO_LOG(Log::Net, "PortManager - Connection Type: %s", connectionType); + } + + m_InitState = UPNP_INITSTATE_DONE; + RefreshPortList(); return true; #else return false; #endif // WITH_UPNP } -bool PortManager::Remove(const char* protocol, unsigned short port) { +bool PortManager::Add(const char *protocol, unsigned short port, unsigned short intport, const std::string &desc) { #ifdef WITH_UPNP - char port_str[16]; - auto n = GetI18NCategory(I18NCat::NETWORKING); - - INFO_LOG(Log::Net, "PortManager::Remove(%s, %d)", protocol, port); - if (urls == NULL || urls->controlURL == NULL || urls->controlURL[0] == '\0') - { - if (g_Config.bEnableUPnP) { - WARN_LOG(Log::Net, "PortManager::Remove - the init was not done !"); - g_OSD.Show(OSDType::MESSAGE_INFO, n->T("UPnP need to be reinitialized")); - } + if (intport == 0) + intport = port; + INFO_LOG(Log::Net, "PortManager::Add(%s, %d, %d)", protocol, port, intport); + if (!HaveControlURL()) { + WARN_LOG(Log::Net, "PortManager::Add - the init was not done !"); Terminate(); return false; } + + char port_str[16]; + char intport_str[16]; snprintf(port_str, sizeof(port_str), "%d", port); - int r = UPNP_DeletePortMapping(urls->controlURL, datas->first.servicetype, port_str, protocol, NULL); - if (r != 0) - { + snprintf(intport_str, sizeof(intport_str), "%d", intport); + + // Nothing to do if we already mapped this port ourselves. + if (std::find_if(m_portList.begin(), m_portList.end(), [port_str, protocol](const std::pair &el) { + return el.first == port_str && el.second == protocol; + }) != m_portList.end()) { + return true; + } + + auto other = std::find_if(m_otherPortList.begin(), m_otherPortList.end(), [port_str, protocol](const PortMap &el) { + return el.extPort_str == port_str && el.protocol == protocol; + }); + if (other != m_otherPortList.end()) { + // Someone else holds this port - drop their mapping first. Mark it as taken as soon as the + // delete goes through, not once our own mapping is in place: if the add below fails we still + // owe them a Restore(). This also covers a dangling mapping of ours from a session that + // didn't shut down cleanly. + if (UPNP_DeletePortMapping(m_urls.controlURL, m_datas.first.servicetype, port_str, protocol, nullptr) == 0) + other->taken = true; + } + + int r = UPNP_AddPortMapping(m_urls.controlURL, m_datas.first.servicetype, + port_str, intport_str, m_lanip.c_str(), desc.c_str(), protocol, nullptr, m_leaseDuration.c_str()); + if (r == 725 && m_leaseDuration != "0") { + // OnlyPermanentLeasesSupported - remember that for the rest of the session. + m_leaseDuration = "0"; + r = UPNP_AddPortMapping(m_urls.controlURL, m_datas.first.servicetype, + port_str, intport_str, m_lanip.c_str(), desc.c_str(), protocol, nullptr, m_leaseDuration.c_str()); + } + if (r != 0) { + ERROR_LOG(Log::Net, "PortManager - AddPortMapping failed (error: %i)", r); + if (r == UPNPCOMMAND_HTTP_ERROR) { + // Usually means the router is no longer reachable (changed networks, went to sleep). + // Invalidate the state so we rediscover instead of eating a timeout on every request. + ShowUPnPMessage("UPnP need to be reinitialized"); + Terminate(); + return false; + } + // Some other UPnP-level refusal. Report it as handled - retrying won't help. + return true; + } + + m_portList.push_front({ port_str, protocol }); + return true; +#else + return false; +#endif // WITH_UPNP +} + +bool PortManager::Remove(const char *protocol, unsigned short port) { +#ifdef WITH_UPNP + INFO_LOG(Log::Net, "PortManager::Remove(%s, %d)", protocol, port); + if (!HaveControlURL()) { + WARN_LOG(Log::Net, "PortManager::Remove - the init was not done !"); + Terminate(); + return false; + } + + char port_str[16]; + snprintf(port_str, sizeof(port_str), "%d", port); + int r = UPNP_DeletePortMapping(m_urls.controlURL, m_datas.first.servicetype, port_str, protocol, nullptr); + if (r != 0) { ERROR_LOG(Log::Net, "PortManager - DeletePortMapping failed (error: %i)", r); if (r == UPNPCOMMAND_HTTP_ERROR) { - if (g_Config.bEnableUPnP) { - g_OSD.Show(OSDType::MESSAGE_INFO, n->T("UPnP need to be reinitialized")); - } - Terminate(); // Most of the time errors occurred because the router is no longer reachable (ie. changed networks) so we should invalidate the state to prevent further lags due to timeouts + ShowUPnPMessage("UPnP need to be reinitialized"); + Terminate(); return false; } } @@ -308,42 +317,42 @@ bool PortManager::Remove(const char* protocol, unsigned short port) { bool PortManager::Restore() { #ifdef WITH_UPNP - int r; VERBOSE_LOG(Log::Net, "PortManager::Restore()"); - if (urls == NULL || urls->controlURL == NULL || urls->controlURL[0] == '\0') - { - if (g_Config.bEnableUPnP) WARN_LOG(Log::Net, "PortManager::Restore - the init was not done !"); + if (!HaveControlURL()) { + WARN_LOG(Log::Net, "PortManager::Restore - the init was not done !"); return false; } - for (auto it = m_otherPortList.begin(); it != m_otherPortList.end(); ++it) { - if (it->taken) { - auto port_str = it->extPort_str; - auto protocol = it->protocol; - // Remove it first if it's still being taken by PPSSPP - auto el_it = std::find_if(m_portList.begin(), m_portList.end(), - [port_str, protocol](const std::pair& el) { return el.first == port_str && el.second == protocol; }); - if (el_it != m_portList.end()) { - r = UPNP_DeletePortMapping(urls->controlURL, datas->first.servicetype, port_str.c_str(), protocol.c_str(), NULL); - if (r == 0) { - m_portList.erase(el_it); - } - else { - ERROR_LOG(Log::Net, "PortManager::Restore - DeletePortMapping failed (error: %i)", r); - if (r == UPNPCOMMAND_HTTP_ERROR) - return false; // Might be better not to exit here, but exiting a loop will avoid long timeouts in the case the router is no longer reachable - } - } - // Add the original owner back - r = UPNP_AddPortMapping(urls->controlURL, datas->first.servicetype, - it->extPort_str.c_str(), it->intPort_str.c_str(), it->lanip.c_str(), it->desc.c_str(), it->protocol.c_str(), it->remoteHost.c_str(), it->duration.c_str()); + for (PortMap &entry : m_otherPortList) { + if (!entry.taken) + continue; + if (OutOfTime()) { + WARN_LOG(Log::Net, "PortManager::Restore - out of time, leaving the rest to the router's lease timeout"); + return false; + } + // Remove it first if it's still being held by PPSSPP. + auto el_it = std::find_if(m_portList.begin(), m_portList.end(), [&entry](const std::pair &el) { + return el.first == entry.extPort_str && el.second == entry.protocol; + }); + if (el_it != m_portList.end()) { + int r = UPNP_DeletePortMapping(m_urls.controlURL, m_datas.first.servicetype, entry.extPort_str.c_str(), entry.protocol.c_str(), nullptr); if (r == 0) { - it->taken = false; - } - else { - ERROR_LOG(Log::Net, "PortManager::Restore - AddPortMapping failed (error: %i)", r); + m_portList.erase(el_it); + } else { + ERROR_LOG(Log::Net, "PortManager::Restore - DeletePortMapping failed (error: %i)", r); if (r == UPNPCOMMAND_HTTP_ERROR) - return false; // Might be better not to exit here, but exiting a loop will avoid long timeouts in the case the router is no longer reachable - } + return false; // The router is gone, the rest of the loop would just be timeouts. + } + } + // Add the original owner back. + int r = UPNP_AddPortMapping(m_urls.controlURL, m_datas.first.servicetype, + entry.extPort_str.c_str(), entry.intPort_str.c_str(), entry.lanip.c_str(), entry.desc.c_str(), + entry.protocol.c_str(), entry.remoteHost.c_str(), entry.duration.c_str()); + if (r == 0) { + entry.taken = false; + } else { + ERROR_LOG(Log::Net, "PortManager::Restore - AddPortMapping failed (error: %i)", r); + if (r == UPNPCOMMAND_HTTP_ERROR) + return false; } } return true; @@ -354,56 +363,29 @@ bool PortManager::Restore() { bool PortManager::Clear() { #ifdef WITH_UPNP - int r; - int i = 0; - char index[16]; - char intAddr[40]; - char intPort[6]; - char extPort[6]; - char protocol[4]; - char desc[80]; - char enabled[6]; - char rHost[64]; - char duration[16]; - VERBOSE_LOG(Log::Net, "PortManager::Clear()"); - if (urls == NULL || urls->controlURL == NULL || urls->controlURL[0] == '\0') - { - if (g_Config.bEnableUPnP) WARN_LOG(Log::Net, "PortManager::Clear - the init was not done !"); + if (!HaveControlURL()) { + WARN_LOG(Log::Net, "PortManager::Clear - the init was not done !"); return false; } - //unsigned int num = 0; - //UPNP_GetPortMappingNumberOfEntries(urls->controlURL, datas->first.servicetype, &num); // Not supported by many routers - do { - snprintf(index, sizeof(index), "%d", i); - rHost[0] = '\0'; enabled[0] = '\0'; - duration[0] = '\0'; desc[0] = '\0'; protocol[0] = '\0'; - extPort[0] = '\0'; intPort[0] = '\0'; intAddr[0] = '\0'; - // May gets UPNPCOMMAND_HTTP_ERROR when called while exiting PPSSPP (ie. used in destructor) - r = UPNP_GetGenericPortMappingEntry(urls->controlURL, - datas->first.servicetype, - index, - extPort, intAddr, intPort, - protocol, desc, enabled, - rHost, duration); - // Only removes port mappings created by PPSSPP for current LAN IP - if (r == 0 && intAddr == m_lanip && std::string(desc).find("PPSSPP:") != std::string::npos) { - int r2 = UPNP_DeletePortMapping(urls->controlURL, datas->first.servicetype, extPort, protocol, rHost); - if (r2 != 0) - { - ERROR_LOG(Log::Net, "PortManager::Clear - DeletePortMapping(%s, %s) failed (error: %i)", extPort, protocol, r2); - if (r2 == UPNPCOMMAND_HTTP_ERROR) - return false; - } - else { - i--; - for (auto it = m_portList.begin(); it != m_portList.end(); ) { - (it->first == extPort && it->second == protocol) ? it = m_portList.erase(it) : ++it; - } - } + // m_portList holds every mapping we know PPSSPP owns: RefreshPortList() seeds it at init time + // (so leftovers from a session that crashed are in there), and Add/Remove keep it current. That + // beats re-walking the router's whole table here, which cost an HTTP round trip per entry at + // the worst possible moment - app exit. + while (!m_portList.empty()) { + if (OutOfTime()) { + WARN_LOG(Log::Net, "PortManager::Clear - out of time, %d mapping(s) left for the router's lease timeout", (int)m_portList.size()); + return false; } - i++; - } while (r == 0 && i < 65536); + const std::pair &entry = m_portList.front(); + int r = UPNP_DeletePortMapping(m_urls.controlURL, m_datas.first.servicetype, entry.first.c_str(), entry.second.c_str(), nullptr); + if (r != 0) { + ERROR_LOG(Log::Net, "PortManager::Clear - DeletePortMapping(%s, %s) failed (error: %i)", entry.first.c_str(), entry.second.c_str(), r); + if (r == UPNPCOMMAND_HTTP_ERROR) + return false; + } + m_portList.pop_front(); + } return true; #else return false; @@ -412,123 +394,208 @@ bool PortManager::Clear() { bool PortManager::RefreshPortList() { #ifdef WITH_UPNP - int r; - int i = 0; - char index[16]; - char intAddr[40]; - char intPort[6]; - char extPort[6]; - char protocol[4]; - char desc[80]; - char enabled[6]; - char rHost[64]; - char duration[16]; - INFO_LOG(Log::Net, "PortManager::RefreshPortList()"); - if (urls == NULL || urls->controlURL == NULL || urls->controlURL[0] == '\0') - { - if (g_Config.bEnableUPnP) WARN_LOG(Log::Net, "PortManager::RefreshPortList - the init was not done !"); + if (!HaveControlURL()) { + WARN_LOG(Log::Net, "PortManager::RefreshPortList - the init was not done !"); return false; } m_portList.clear(); m_otherPortList.clear(); - //unsigned int num = 0; - //UPNP_GetPortMappingNumberOfEntries(urls->controlURL, datas->first.servicetype, &num); // Not supported by many routers + + PortMappingEntry e; + int i = 0; + int r; do { + if (OutOfTime()) + return false; + char index[16]; snprintf(index, sizeof(index), "%d", i); - rHost[0] = '\0'; enabled[0] = '\0'; - duration[0] = '\0'; desc[0] = '\0'; protocol[0] = '\0'; - extPort[0] = '\0'; intPort[0] = '\0'; intAddr[0] = '\0'; - r = UPNP_GetGenericPortMappingEntry(urls->controlURL, - datas->first.servicetype, - index, - extPort, intAddr, intPort, - protocol, desc, enabled, - rHost, duration); + memset(&e, 0, sizeof(e)); + r = UPNP_GetGenericPortMappingEntry(m_urls.controlURL, m_datas.first.servicetype, index, + e.extPort, e.intClient, e.intPort, e.protocol, e.desc, e.enabled, e.rHost, e.duration); if (r == 0) { - std::string desc_str = std::string(desc); - // Some router might prefix the description with "UPnP:" so we may need to truncate it to prevent it from getting multiple prefix when restored later - if (desc_str.find("UPnP:") == 0) - desc_str = desc_str.substr(5); - // Only include port mappings created by PPSSPP for current LAN IP - if (intAddr == m_lanip && desc_str.find("PPSSPP:") != std::string::npos) { - m_portList.push_back({ extPort, protocol }); - } - // Port mappings belong to others that might be taken by PPSSPP later - else { - m_otherPortList.push_back({ false, protocol, extPort, intPort, intAddr, rHost, desc_str, duration, enabled }); + std::string desc = e.desc; + // Some routers prefix the description with "UPnP:", which we need to strip so it doesn't + // accumulate another prefix each time we restore the mapping. + if (startsWith(desc, "UPnP:")) + desc = desc.substr(5); + if (e.intClient == m_lanip && desc.find("PPSSPP:") != std::string::npos) { + // Ours, possibly left over from an earlier session. Clear() will drop it. + m_portList.push_back({ e.extPort, e.protocol }); + } else { + // Someone else's, which we may end up taking over (and then have to put back). + m_otherPortList.push_back({ false, e.protocol, e.extPort, e.intPort, e.intClient, e.rHost, desc, e.duration, e.enabled }); } } i++; - } while (r == 0 && i < 65536); + } while (r == 0 && i < MAX_PORT_MAPPINGS); + + if (i >= MAX_PORT_MAPPINGS) + WARN_LOG(Log::Net, "PortManager::RefreshPortList - stopped after %d entries", MAX_PORT_MAPPINGS); + INFO_LOG(Log::Net, "PortManager - %d existing PPSSPP mapping(s), %d belonging to others", + (int)m_portList.size(), (int)m_otherPortList.size()); return true; #else return false; #endif // WITH_UPNP } -int upnpService(const unsigned int timeout) { +// --- Service thread --- + +static void DiscardQueuedRequests() { + std::lock_guard lock(g_upnpLock); + if (!g_upnpReqs.empty()) { + DEBUG_LOG(Log::Net, "UPnPService: discarding %d queued request(s)", (int)g_upnpReqs.size()); + g_upnpReqs.clear(); + } +} + +// Works through the queue until it's empty or the router stops answering. +// Returns false if we lost the router, in which case the unfinished request stays queued. +static bool ProcessQueuedRequests() { + while (true) { + UPnPArgs arg; + { + std::lock_guard lock(g_upnpLock); + if (g_upnpExit || g_upnpReqs.empty()) + return true; + // Take it out of the queue for the duration. Talking to the router happens without the + // lock held, and QueueRequest() supersedes pending requests for the same port - if the + // in-flight one were still in the deque it could be erased out from under us, and we'd + // then drop whatever replaced it without ever running it. + arg = std::move(g_upnpReqs.front()); + g_upnpReqs.pop_front(); + } + + bool ok; + switch (arg.cmd) { + case UPNP_CMD_ADD: + ok = g_PortManager.Add(arg.protocol.c_str(), arg.port, arg.intport, arg.desc); + break; + case UPNP_CMD_REMOVE: + ok = g_PortManager.Remove(arg.protocol.c_str(), arg.port); + break; + default: + ok = true; + break; + } + if (ok) + continue; + + // The router stopped answering, and Add()/Remove() have already reset us to disconnected. + // Put the request back for after we reconnect and stop draining - everything behind it + // would just fail the same way. + if (++arg.attempts < MAX_REQUEST_ATTEMPTS) { + std::lock_guard lock(g_upnpLock); + const bool superseded = std::any_of(g_upnpReqs.begin(), g_upnpReqs.end(), [&arg](const UPnPArgs &req) { + return req.port == arg.port && req.protocol == arg.protocol; + }); + if (!superseded && g_upnpReqs.size() < MAX_QUEUED_REQUESTS) + g_upnpReqs.push_front(std::move(arg)); + } else { + WARN_LOG(Log::Net, "UPnPService: giving up on %s port %d after %d attempts", + arg.protocol.c_str(), arg.port, MAX_REQUEST_ATTEMPTS); + } + return false; + } +} + +static int upnpService(unsigned int timeout) { SetCurrentThreadName("UPnPService"); INFO_LOG(Log::Net, "UPnPService: Begin of UPnPService Thread"); - // Service Loop + int failCount = 0; + bool wasEnabled = false; + // Absolute time before which we won't try to (re)discover a router. This has to be a deadline + // rather than a sleep length: an incoming request wakes us early, and without it every single + // request would trigger another full SSDP discovery while the router is unreachable. + double nextInitTime = 0.0; + while (true) { - UPnPArgs arg; - bool haveArg; + uint32_t seq; { - std::unique_lock lock(g_upnpLock); - // Also wake up periodically even with nothing queued (and on UPnP_Notify()), so we - // can retry a failed/disconnected UPnP init or notice the enable setting flipped, - // without needing an explicit Add/Remove request to prod us. - g_upnpCond.wait_for(lock, std::chrono::seconds(5), [] { return !g_upnpReqs.empty(); }); - haveArg = !g_upnpReqs.empty(); - if (haveArg) { - arg = g_upnpReqs.front(); - } - } - - // Exit requests must be handled regardless of whether UPnP is enabled or has - // finished initializing, otherwise shutdown could wait on this thread forever. - if (haveArg && arg.cmd == UPNP_CMD_EXIT) { std::lock_guard lock(g_upnpLock); - g_upnpReqs.pop_front(); - break; + if (g_upnpExit) + break; + seq = g_upnpWakeSeq; + if (g_upnpResetBackoff) { + // The user just flipped the setting - don't make them wait out an old backoff. + g_upnpResetBackoff = false; + failCount = 0; + nextInitTime = 0.0; + } } - // Attempts to reconnect if not connected yet or got disconnected - if (g_Config.bEnableUPnP && g_PortManager.GetInitState() == UPNP_INITSTATE_NONE) { - g_PortManager.Initialize(timeout); - } + // 0 means "sleep until someone wakes us" - which is what this thread does for the whole + // session for most users, rather than waking up periodically to find nothing to do. + double wakeAt = 0.0; - if (haveArg && g_Config.bEnableUPnP && g_PortManager.GetInitState() == UPNP_INITSTATE_DONE) { - bool ok = true; - switch (arg.cmd) { - case UPNP_CMD_ADD: - ok = g_PortManager.Add(arg.protocol.c_str(), arg.port, arg.intport); - break; - case UPNP_CMD_REMOVE: - ok = g_PortManager.Remove(arg.protocol.c_str(), arg.port); - break; - default: - break; + if (!g_Config.bEnableUPnP) { + // Callers queue requests without checking the setting (see bind() in sceNetInet), so throw + // them away here. Otherwise the queue grows without bound and, worse, the wait below never + // blocks - which is what pegged a core whenever UPnP was off but a game was using sockets. + DiscardQueuedRequests(); + if (wasEnabled) { + // Turned off at runtime - take our mappings back down right away. + INFO_LOG(Log::Net, "UPnPService: UPnP was disabled, cleaning up"); + g_PortManager.Shutdown(); + wasEnabled = false; + } + failCount = 0; + nextInitTime = 0.0; + + std::lock_guard lock(g_upnpLock); + if (!g_Config.bEnableUPnP) { + // Nothing left to do until the setting comes back on, and StartUPnPService() will + // spin up a fresh thread for that. Deciding this under the lock is what makes that + // safe: a start that observed us still running can't be left without a thread. + g_upnpThreadRunning = false; + break; + } + // Turned back on while we were cleaning up (which involves network round trips, so + // there's real time in there) - carry on rather than exiting. + continue; + } else { + wasEnabled = true; + if (g_PortManager.GetInitState() == UPNP_INITSTATE_NONE && time_now_d() >= nextInitTime) { + if (g_PortManager.Initialize(timeout)) { + failCount = 0; + nextInitTime = 0.0; + } else { + // Only complain the first time, not once per retry. + if (failCount == 0) + ShowUPnPMessage("Unable to find UPnP device"); + failCount++; + const double backoff = std::min(MIN_RETRY_SECONDS * (1 << std::min(failCount - 1, 8)), MAX_RETRY_SECONDS); + nextInitTime = time_now_d() + backoff; + } } - // It's only considered failed when disconnected (should be retried when reconnected) - if (ok) { - std::lock_guard lock(g_upnpLock); - g_upnpReqs.pop_front(); - } + if (g_PortManager.GetInitState() == UPNP_INITSTATE_DONE) { + if (!ProcessQueuedRequests()) { + // Lost the router mid-request. Whatever's left stays queued for after we reconnect. + nextInitTime = time_now_d() + MIN_RETRY_SECONDS; + } + } + if (g_PortManager.GetInitState() != UPNP_INITSTATE_DONE) + wakeAt = nextInitTime; + } + + std::unique_lock lock(g_upnpLock); + auto shouldWake = [seq] { return g_upnpExit || g_upnpWakeSeq != seq; }; + if (wakeAt == 0.0) { + g_upnpCond.wait(lock, shouldWake); + } else { + const double delay = std::max(wakeAt - time_now_d(), 0.0); + g_upnpCond.wait_for(lock, std::chrono::duration(delay), shouldWake); } } - // Cleaning up regardless of g_Config.bEnableUPnP to prevent lingering open ports on the router - if (g_PortManager.GetInitState() == UPNP_INITSTATE_DONE) { - g_PortManager.Shutdown(); - } + // Clean up regardless of g_Config.bEnableUPnP, to avoid leaving open ports on the router. + g_PortManager.Shutdown(); - // Should we ignore any leftover UPnP requests? instead of processing it on the next game start { - std::unique_lock lock(g_upnpLock); + std::lock_guard lock(g_upnpLock); g_upnpReqs.clear(); } @@ -536,39 +603,116 @@ int upnpService(const unsigned int timeout) { return 0; } -void __UPnPInit(const unsigned int timeout) { - _dbg_assert_(!g_upnpServiceThread.joinable()); +// Starts the service thread if the setting is on and it isn't up already. +static void StartUPnPService() { + std::lock_guard threadLock(g_upnpThreadLock); + { + std::lock_guard lock(g_upnpLock); + if (!g_upnpInitialized || g_upnpExit || !g_Config.bEnableUPnP) + return; + if (g_upnpThreadRunning) { + // Already up - it'll notice whatever changed on its own. + return; + } + g_upnpThreadRunning = true; + g_upnpResetBackoff = true; + } + // A previous thread may have exited when the setting was turned off. By the time it clears + // g_upnpThreadRunning it has already cleaned up after itself, so this doesn't block on a router. + if (g_upnpServiceThread.joinable()) + g_upnpServiceThread.join(); + g_upnpServiceThread = std::thread(upnpService, g_upnpTimeout); +} - g_upnpServiceThread = std::thread(upnpService, timeout); +void __UPnPInit(unsigned int timeout) { + { + std::lock_guard lock(g_upnpLock); + g_upnpExit = false; + g_upnpReqs.clear(); + g_upnpInitialized = true; + g_upnpTimeout = timeout; + } + // Only actually spawns a thread if UPnP is enabled; otherwise UPnP_Notify() starts one when + // the user turns it on. + StartUPnPService(); } void __UPnPShutdown() { - _dbg_assert_(g_upnpServiceThread.joinable()); { - std::lock_guard upnpGuard(g_upnpLock); - g_upnpReqs.push_back({ UPNP_CMD_EXIT }); - g_upnpCond.notify_one(); + std::lock_guard lock(g_upnpLock); + g_upnpInitialized = false; + g_upnpExit = true; + // Anything still queued is moot, and dropping it here means the thread won't try to talk to + // a router we may no longer be able to reach on its way out. + g_upnpReqs.clear(); + g_upnpWakeSeq++; } + g_upnpCond.notify_all(); - INFO_LOG(Log::HTTP, "Waiting for upnp thread to shut down..."); + std::lock_guard threadLock(g_upnpThreadLock); if (g_upnpServiceThread.joinable()) { + INFO_LOG(Log::Net, "Waiting for upnp thread to shut down..."); g_upnpServiceThread.join(); + INFO_LOG(Log::Net, "upnp thread shut down."); } - INFO_LOG(Log::HTTP, "upnp thread shut down."); + std::lock_guard lock(g_upnpLock); + g_upnpThreadRunning = false; } -void UPnP_Add(const char* protocol, unsigned short port, unsigned short intport) { - std::lock_guard upnpGuard(g_upnpLock); - g_upnpReqs.push_back({ UPNP_CMD_ADD, protocol, port, intport }); +static void QueueRequest(UPnPArgs args) { + // The enable setting can change after startup without the settings UI being involved - a + // per-game config, or a libretro core option. Reconcile here rather than requiring every place + // that can flip it to remember to call UPnP_Notify(): this starts a thread if the setting is on + // and there isn't one, and is a cheap no-op otherwise. The other direction takes care of itself, + // since queuing below wakes a thread that then notices the setting is off and cleans up. + StartUPnPService(); + + std::lock_guard lock(g_upnpLock); + // With UPnP off there's no service thread to drain the queue, so don't let one build up. + if (!g_upnpThreadRunning || g_upnpExit) + return; + + // Last request wins for a given port: this collapses the repeated adds that games produce when + // they rebind in a loop, and lets a remove cancel an add that hasn't been sent yet. + for (auto it = g_upnpReqs.begin(); it != g_upnpReqs.end(); ) { + (it->port == args.port && it->protocol == args.protocol) ? it = g_upnpReqs.erase(it) : ++it; + } + if (g_upnpReqs.size() >= MAX_QUEUED_REQUESTS) { + WARN_LOG(Log::Net, "UPnP request queue is full, dropping request for %s port %d", args.protocol.c_str(), args.port); + return; + } + + g_upnpReqs.push_back(std::move(args)); + g_upnpWakeSeq++; g_upnpCond.notify_one(); } -void UPnP_Remove(const char* protocol, unsigned short port) { - std::lock_guard upnpGuard(g_upnpLock); - g_upnpReqs.push_back({ UPNP_CMD_REMOVE, protocol, port, port }); - g_upnpCond.notify_one(); +// Built here rather than on the service thread, which can't safely read the game state. +// Some routers automatically prefix the description with "UPnP:". +static std::string MappingDescription() { + if (PSP_IsInited()) { + return "PPSSPP:" + g_paramSFO.GetDiscID() + ":" + g_Config.sNickName; + } + return "PPSSPP:at_menu:" + g_Config.sNickName; +} + +void UPnP_Add(const char *protocol, unsigned short port, unsigned short intport) { + QueueRequest({ UPNP_CMD_ADD, protocol, port, intport ? intport : port, MappingDescription() }); +} + +void UPnP_Remove(const char *protocol, unsigned short port) { + QueueRequest({ UPNP_CMD_REMOVE, protocol, port, port }); } void UPnP_Notify() { + { + std::lock_guard lock(g_upnpLock); + g_upnpWakeSeq++; + g_upnpResetBackoff = true; + } g_upnpCond.notify_one(); + // Turned on: start a thread. Turned off: the running one cleans up and exits by itself, which + // we deliberately don't wait for here - this is called from the UI thread, and the cleanup + // means talking to a router that may be slow to answer. + StartUPnPService(); } diff --git a/Core/Util/PortManager.h b/Core/Util/PortManager.h index 2b209bf429..5af37d1a9e 100644 --- a/Core/Util/PortManager.h +++ b/Core/Util/PortManager.h @@ -37,13 +37,6 @@ #include #include -struct UPnPArgs { - int cmd; - std::string protocol; - unsigned short port; - unsigned short intport; -}; - #define IP_PROTOCOL_TCP "TCP" #define IP_PROTOCOL_UDP "UDP" @@ -56,11 +49,20 @@ enum { enum { UPNP_CMD_ADD = 0, UPNP_CMD_REMOVE = 1, - UPNP_CMD_EXIT = 2, }; -struct UPNPUrls; -struct IGDdatas; +struct UPnPArgs { + int cmd = UPNP_CMD_ADD; + std::string protocol; + unsigned short port = 0; + unsigned short intport = 0; + // Description to register the mapping under. Built when the request is queued, on the thread + // that owns the game state, since the UPnP service thread can't safely read it later. + std::string desc; + // How many times we've failed to reach the router about this request. Bounded so a request + // can't get retried forever, blocking everything queued behind it. + int attempts = 0; +}; struct PortMap { bool taken; @@ -74,29 +76,33 @@ struct PortMap { std::string enabled; }; +// Only ever touched by the UPnP service thread (see PortManager.cpp). Don't call into it +// from anywhere else - queue a request with UPnP_Add()/UPnP_Remove() instead. class PortManager { public: - // Initialize UPnP - // timeout: milliseconds to wait for a router to respond (default = 2000 ms) - bool Initialize(const unsigned int timeout = 2000); + // Discover a router and pick up any mappings we left behind earlier. + // timeout: milliseconds to wait for a router to respond. + bool Initialize(unsigned int timeout = 2000); - // Get UPnP Initialization status - int GetInitState(); + int GetInitState() const { return m_InitState; } // Add a port & protocol (TCP, UDP or vendor-defined) to map for forwarding (intport = 0 : same as [external] port) - bool Add(const char* protocol, unsigned short port, unsigned short intport = 0); + bool Add(const char *protocol, unsigned short port, unsigned short intport, const std::string &desc); // Remove a port mapping (external port) - bool Remove(const char* protocol, unsigned short port); + bool Remove(const char *protocol, unsigned short port); - // Call on exit. Does a full shutdown. - void Shutdown(); + // Drops our mappings, restores any that we took over, and resets to the uninitialized state. + // budgetSeconds bounds how long we're willing to keep talking to the router: a router that has + // gone away answers with socket timeouts, which would otherwise stall app exit for a long time. + void Shutdown(double budgetSeconds = 3.0); private: // Retrieves port lists mapped by PPSSPP for current LAN IP & other's applications bool RefreshPortList(); - // Removes any lingering mapped ports created by PPSSPP (including from previous crashes) + // Removes the port mappings we know PPSSPP created (including leftovers from previous crashes, + // which RefreshPortList() picks up at init time). bool Clear(); // Restore ports mapped by others that were taken by PPSSPP, better used after Clear() @@ -105,30 +111,35 @@ private: // Uninitialize/Reset the state void Terminate(); - struct UPNPUrls* urls = nullptr; - struct IGDdatas* datas = nullptr; + bool HaveControlURL() const; + // True once the current operation has used up its time budget, see Shutdown(). + bool OutOfTime() const; + + UPNPUrls m_urls{}; + IGDdatas m_datas{}; + bool m_urlsValid = false; int m_InitState = UPNP_INITSTATE_NONE; int m_LocalPort = UPNP_LOCAL_PORT_ANY; + double m_deadline = 0.0; std::string m_lanip; - std::string m_defaultDesc; - std::string m_leaseDuration = "43200"; // range(0-604800) in seconds (0 = Indefinite/permanent). Some routers doesn't support non-zero value + std::string m_leaseDuration; std::deque> m_portList; std::deque m_otherPortList; }; extern PortManager g_PortManager; -void __UPnPInit(const unsigned int timeout_ms); +void __UPnPInit(unsigned int timeout_ms); void __UPnPShutdown(); // Add a port & protocol (TCP, UDP or vendor-defined) to map for forwarding (intport = 0 : same as [external] port) -void UPnP_Add(const char* protocol, unsigned short port, unsigned short intport = 0); +void UPnP_Add(const char *protocol, unsigned short port, unsigned short intport = 0); // Remove a port mapping (external port) -void UPnP_Remove(const char* protocol, unsigned short port); +void UPnP_Remove(const char *protocol, unsigned short port); -// Wakes the UPnP service thread immediately, without queuing a request - useful after -// changing the enable setting or similar, so it can (re)connect without waiting for the -// periodic retry. +// Wakes the UPnP service thread immediately, without queuing a request - call this after +// changing the enable setting so it can connect (or tear down its mappings) right away +// instead of waiting for the next retry. void UPnP_Notify(); diff --git a/UI/GameSettingsScreen.cpp b/UI/GameSettingsScreen.cpp index 7720e12c14..a771f7eaa1 100644 --- a/UI/GameSettingsScreen.cpp +++ b/UI/GameSettingsScreen.cpp @@ -1049,9 +1049,13 @@ void GameSettingsScreen::CreateNetworkingSettings(UI::ViewGroup *networkingSetti dnsServer->SetDisabledPtr(&g_Config.bInfrastructureAutoDNS); networkingSettings->Add(new ItemHeader(n->T("UPnP (port-forwarding)"))); - networkingSettings->Add(new CheckBox(&g_Config.bEnableUPnP, n->T("Enable UPnP", "Enable UPnP (need a few seconds to detect)")))->OnClick.Add([](UI::EventParams &e) { - // Wake the UPnP service thread immediately so it reacts to the new setting instead - // of waiting for the next periodic retry (or a port request that may never come). + // Only togglable outside a game - sceNet latches settings like UPnPUseOriginalPort at boot, + // and a game that's already mapped its ports wouldn't cope with them disappearing. + CheckBox *enableUPnP = networkingSettings->Add(new CheckBox(&g_Config.bEnableUPnP, n->T("Enable UPnP", "Enable UPnP (need a few seconds to detect)"))); + enableUPnP->SetEnabled(!PSP_IsInited()); + enableUPnP->OnClick.Add([](UI::EventParams &e) { + // Wake the UPnP service thread so it connects (or tears its mappings back down) right + // away, instead of waiting for a port request that may never come. UPnP_Notify(); }); auto *useOriPort = networkingSettings->Add(new CheckBox(&g_Config.bUPnPUseOriginalPort, n->T("UPnP use original port", "UPnP use original port (Enabled = PSP compatibility)"))); diff --git a/UI/NativeApp.cpp b/UI/NativeApp.cpp index 8253b8e717..2b06c0104d 100644 --- a/UI/NativeApp.cpp +++ b/UI/NativeApp.cpp @@ -515,9 +515,6 @@ void NativeInit(int argc, const char *argv[], const CommandLineOptions &cmdLineO IncrementDebugCounter(DebugCounter::APP_BOOT); - // Probably an excessive timeout. it only causes delays on shutdown, though. - __UPnPInit(2000); - ShaderTranslationInit(); g_threadManager.Init(cpu_info.num_cores, cpu_info.logical_cpu_count); @@ -704,6 +701,11 @@ void NativeInit(int argc, const char *argv[], const CommandLineOptions &cmdLineO g_Config.LoadAppendedConfig(); } + // Has to be after the config is loaded: it only starts a service thread if UPnP is enabled, + // and g_Config.Init() above doesn't read the ini, it just builds a lookup table. + // Probably an excessive timeout. It only causes delays on shutdown, though. + __UPnPInit(2000); + // This parameter should be a boot filename. Only accept it if we // don't already have one. if (!cmdLineOptions.bootFilenames.empty()) { @@ -1851,8 +1853,6 @@ void NativeShutdown() { __UPnPShutdown(); - g_PortManager.Shutdown(); - net::Shutdown(); g_Discord.Shutdown();