From 64f3f71f8def4af920faf653f16733ce05eddeb3 Mon Sep 17 00:00:00 2001 From: Enrique Wood Date: Mon, 9 Nov 2026 21:57:40 -0300 Subject: [PATCH 1/6] feat(upnp): map ports through double NAT --- docs/configuration.md | 2 + src/upnp.cpp | 177 +++++++++++++++++++++++++++++++++++++-- src/upnp.h | 46 ++++++++++ tests/unit/test_upnp.cpp | 63 ++++++++++++++ 4 files changed, 279 insertions(+), 9 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index a6e5727c536..f57b894e33a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1553,6 +1553,8 @@ consent prompt for directory ACLs, so the Web UI shows setup steps for correctin Description Sunshine will attempt to open ports for streaming over the internet. + If Sunshine's router is behind another router (double NAT) and both support UPnP, + the ports are also opened on the outer router, forwarded to the inner router. diff --git a/src/upnp.cpp b/src/upnp.cpp index ea2bab0cdf6..e9ccfeb6a43 100644 --- a/src/upnp.cpp +++ b/src/upnp.cpp @@ -4,6 +4,8 @@ */ // standard includes #include +#include +#include #include // workaround for type_t error in miniupnpc 2.3.3, see https://github.com/miniupnp/miniupnp/commit/e263ab6f56c382e10fed31347ec68095d691a0e8 // lib includes @@ -39,6 +41,52 @@ namespace upnp { std::string description; ///< Human-readable UPnP lease description advertised to the gateway. }; + /** + * @brief A discovered IGD and the data needed to manage its port mappings. + */ + struct igd_t { + std::string host; ///< Address of the IGD, taken from its description URL. + urls_t urls; ///< Control URLs of the IGD. + IGDdatas data {}; ///< Service data of the IGD. + }; + + /** + * @brief Describe the discovered IGDs other than the one already in the list. + * @param device Device list from upnpDiscover(). + * @param igds List holding the IGD chosen by UPNP_GetValidIGD(); the other IGDs are appended to it. + */ + static void add_other_igds(const device_t &device, std::vector &igds) { + std::set> seen {igds.front().urls->rootdescURL}; + for (auto dev = device.get(); dev != nullptr; dev = dev->pNext) { + if (!seen.emplace(dev->descURL).second) { + continue; + } + + igd_t igd; + std::array lan_addr {}; + if (UPNP_GetIGDFromUrl(dev->descURL, &igd.urls.el, &igd.data, lan_addr.data(), (int) lan_addr.size()) != 1 || igd.urls->controlURL == nullptr || igd.urls->controlURL[0] == 0 || igd.data.first.servicetype[0] == 0) { + continue; + } + + igd.host = url_host(dev->descURL); + igds.emplace_back(std::move(igd)); + } + } + + /** + * @brief Get the external address an IGD reports. + * @param igd The IGD to ask. + * @return The external address, or an empty string if the IGD doesn't report one. + */ + static std::string external_address(const igd_t &igd) { + std::array addr {}; + if (UPNP_GetExternalIPAddress(igd.urls->controlURL, igd.data.first.servicetype, addr.data()) != UPNPCOMMAND_SUCCESS) { + return {}; + } + + return addr.data(); + } + static std::string_view status_string(igd_status_e status) { using enum igd_status_e; @@ -88,6 +136,78 @@ namespace upnp { } } + /** + * @brief Check whether a value is a private (LAN) IPv4 address. + * @param address The value to check. + * @return `true` if `address` is an IPv4 address in a private, shared or link-local range. + */ + static bool is_private_ipv4(std::string_view address) { + boost::system::error_code ec; + const auto addr = boost::asio::ip::make_address_v4(std::string {address}, ec); + return !ec && net::from_address(addr.to_string()) == net::LAN; + } + + int shared_prefix_length(std::string_view a, std::string_view b) { + boost::system::error_code ec_a; + boost::system::error_code ec_b; + const auto addr_a = boost::asio::ip::make_address_v4(std::string {a}, ec_a); + const auto addr_b = boost::asio::ip::make_address_v4(std::string {b}, ec_b); + if (ec_a || ec_b) { + return -1; + } + + return std::countl_zero(addr_a.to_uint() ^ addr_b.to_uint()); + } + + std::string url_host(std::string_view url) { + if (const auto scheme = url.find("://"sv); scheme != std::string_view::npos) { + url.remove_prefix(scheme + 3); + } + + if (url.starts_with('[')) { + return std::string {url.substr(1, url.find(']') - 1)}; + } + + return std::string {url.substr(0, url.find_first_of(":/"sv))}; + } + + std::size_t select_inner_igd(const std::vector &igd_hosts, std::string_view lan_addr) { + std::size_t best = 0; + int best_bits = -1; + for (std::size_t i = 0; i < igd_hosts.size(); ++i) { + if (const auto bits = shared_prefix_length(igd_hosts[i], lan_addr); bits > best_bits) { + best = i; + best_bits = bits; + } + } + + return best; + } + + std::optional select_outer_igd(const std::vector &igd_hosts, std::size_t inner, std::string_view inner_external_addr) { + // A public external address means the inner gateway is the edge router: there is nothing in front of it to map. + if (!is_private_ipv4(inner_external_addr)) { + return std::nullopt; + } + + // The outer router sits in the same network as the inner gateway's external address. + constexpr int min_shared_bits = 16; + std::optional best; + int best_bits = min_shared_bits - 1; + for (std::size_t i = 0; i < igd_hosts.size(); ++i) { + if (i == inner) { + continue; + } + + if (const auto bits = shared_prefix_length(igd_hosts[i], inner_external_addr); bits > best_bits) { + best = i; + best_bits = bits; + } + } + + return best; + } + /** * @brief RAII helper that runs shutdown cleanup when destroyed. */ @@ -335,8 +455,9 @@ namespace upnp { platf::set_thread_name("upnp"); auto shutdown_event = mail::man->event(mail::shutdown); bool mapped = false; - IGDdatas data; - urls_t mapped_urls; + igd_t mapped_igd; + std::optional mapped_outer_igd; + bool warned_no_outer_igd = false; auto address_family = net::get_effective_address_family(net::af_from_enum_string(config::sunshine.address_family)); // Refresh UPnP rules every few minutes. They can be lost if the router reboots, @@ -356,8 +477,8 @@ namespace upnp { std::array lan_addr; - urls_t urls; - auto status = upnp::UPNP_GetValidIGDStatus(device, &urls, &data, lan_addr); + std::vector igds(1); + auto status = upnp::UPNP_GetValidIGDStatus(device, &igds.front().urls, &igds.front().data, lan_addr); if (!is_valid_igd(status)) { BOOST_LOG(error) << status_string(status); mapped = false; @@ -366,14 +487,49 @@ namespace upnp { std::string lan_addr_str {lan_addr.data()}; - BOOST_LOG(debug) << "Found valid IGD device: "sv << urls->rootdescURL; + BOOST_LOG(debug) << "Found valid IGD device: "sv << igds.front().urls->rootdescURL; + + // Behind two routers, the IGD chosen above may be the outer one, which can't reach this host. + igds.front().host = url_host(igds.front().urls->rootdescURL); + add_other_igds(device, igds); + std::vector igd_hosts; + for (const auto &igd : igds) { + igd_hosts.push_back(igd.host); + } + + const auto inner = select_inner_igd(igd_hosts, lan_addr_str); + auto &igd = igds[inner]; + if (inner != 0) { + BOOST_LOG(debug) << "Using the IGD on this host's network instead: "sv << igd.urls->rootdescURL; + } for (auto it = std::begin(mappings); it != std::end(mappings) && !shutdown_event->peek(); ++it) { - map_upnp_port(data, urls, lan_addr_str, *it); + map_upnp_port(igd.data, igd.urls, lan_addr_str, *it); } if (!mapped) { - BOOST_LOG(info) << "Completed UPnP port mappings to "sv << lan_addr_str << " via "sv << urls->rootdescURL; + BOOST_LOG(info) << "Completed UPnP port mappings to "sv << lan_addr_str << " via "sv << igd.urls->rootdescURL; + } + + // Double NAT: if the gateway's external address is private, map the same ports on the IGD in front of it, + // forwarding them to the gateway's external address. + const auto external_addr = external_address(igd); + if (const auto outer = select_outer_igd(igd_hosts, inner, external_addr)) { + auto &outer_igd = igds[*outer]; + for (auto it = std::begin(mappings); it != std::end(mappings) && !shutdown_event->peek(); ++it) { + map_upnp_port(outer_igd.data, outer_igd.urls, external_addr, *it); + } + + if (!mapped || !mapped_outer_igd || mapped_outer_igd->host != outer_igd.host) { + BOOST_LOG(info) << "Completed UPnP port mappings to "sv << external_addr << " via "sv << outer_igd.urls->rootdescURL; + } + + mapped_outer_igd = std::move(outer_igd); + warned_no_outer_igd = false; + } else if (is_private_ipv4(external_addr) && !warned_no_outer_igd) { + BOOST_LOG(warning) << "The IGD's external address "sv << external_addr + << " is private and no UPnP IGD answered in front of it: ports must be forwarded on the outer router"sv; + warned_no_outer_igd = true; } // If we are listening on IPv6 and the IGD has an IPv6 firewall enabled, try to create IPv6 firewall pinholes @@ -385,13 +541,16 @@ namespace upnp { } mapped = true; - mapped_urls = std::move(urls); + mapped_igd = std::move(igd); } while (!shutdown_event->view(REFRESH_INTERVAL)); if (mapped) { // Unmap ports upon termination BOOST_LOG(info) << "Unmapping UPNP ports..."sv; - unmap_all_upnp_ports(mapped_urls, data); + unmap_all_upnp_ports(mapped_igd.urls, mapped_igd.data); + if (mapped_outer_igd) { + unmap_all_upnp_ports(mapped_outer_igd->urls, mapped_outer_igd->data); + } } } diff --git a/src/upnp.h b/src/upnp.h index 1f68f48a3d5..e984d1e4372 100644 --- a/src/upnp.h +++ b/src/upnp.h @@ -4,6 +4,13 @@ */ #pragma once +// standard includes +#include +#include +#include +#include +#include + // lib includes #include @@ -72,6 +79,45 @@ namespace upnp { */ igd_status_e UPNP_GetValidIGDStatus(device_t &device, urls_t *urls, IGDdatas *data, std::array &lan_addr); + /** + * @brief Count the leading bits two IPv4 addresses have in common. + * @param a First IPv4 address in dotted notation. + * @param b Second IPv4 address in dotted notation. + * @return Number of shared leading bits (0 to 32), or -1 if either value is not an IPv4 address. + */ + [[nodiscard]] int shared_prefix_length(std::string_view a, std::string_view b); + + /** + * @brief Extract the host from a URL such as `http://192.168.1.1:1900/rootDesc.xml`. + * @param url The URL to parse. + * @return The host part of the URL, without port or brackets. + */ + [[nodiscard]] std::string url_host(std::string_view url); + + /** + * @brief Select the IGD of the network Sunshine is attached to. + * @details Behind two routers, the outer router often answers discovery too, and UPNP_GetValidIGD() prefers it + * because it has a public address. That router can't reach Sunshine's address, so a mapping there opens nothing. + * The gateway Sunshine is attached to is the one whose address shares the most leading bits with Sunshine's own + * address. On a tie, the earlier entry wins, so the first entry (the IGD chosen by UPNP_GetValidIGD()) is kept + * unless another IGD is strictly closer. + * @param igd_hosts Addresses of the discovered IGDs. + * @param lan_addr Sunshine's local address. + * @return Index of the selected IGD in `igd_hosts`, or 0 if the list is empty. + */ + [[nodiscard]] std::size_t select_inner_igd(const std::vector &igd_hosts, std::string_view lan_addr); + + /** + * @brief Select the IGD in front of the inner gateway when Sunshine is behind two routers (double NAT). + * @details The outer router is only looked for when the inner gateway's external address is private. It is the + * other IGD whose address shares the most leading bits (at least 16) with that external address. + * @param igd_hosts Addresses of the discovered IGDs. + * @param inner Index of the inner gateway in `igd_hosts`. + * @param inner_external_addr External address reported by the inner gateway. + * @return Index of the outer IGD in `igd_hosts`, or `std::nullopt` if there is none. + */ + [[nodiscard]] std::optional select_outer_igd(const std::vector &igd_hosts, std::size_t inner, std::string_view inner_external_addr); + /** * @brief Start UPnP port mapping and return its shutdown guard. * diff --git a/tests/unit/test_upnp.cpp b/tests/unit/test_upnp.cpp index bf0304d32a8..18c7e781d69 100644 --- a/tests/unit/test_upnp.cpp +++ b/tests/unit/test_upnp.cpp @@ -40,3 +40,66 @@ TEST(UpnpIsValidIgdTest, RejectsOutOfRange) { ASSERT_FALSE(upnp::is_valid_igd(static_cast(5))); ASSERT_FALSE(upnp::is_valid_igd(static_cast(100))); } + +TEST(UpnpSharedPrefixLengthTest, CountsSharedLeadingBits) { + EXPECT_EQ(upnp::shared_prefix_length("192.168.20.181", "192.168.20.181"), 32); + EXPECT_EQ(upnp::shared_prefix_length("192.168.20.181", "192.168.1.100"), 19); + EXPECT_EQ(upnp::shared_prefix_length("10.0.0.86", "10.0.0.1"), 25); + EXPECT_EQ(upnp::shared_prefix_length("10.0.0.1", "192.168.1.1"), 0); +} + +TEST(UpnpSharedPrefixLengthTest, RejectsNonIpv4Values) { + EXPECT_EQ(upnp::shared_prefix_length("router.local", "192.168.1.1"), -1); + EXPECT_EQ(upnp::shared_prefix_length("192.168.1.1", "fe80::1"), -1); + EXPECT_EQ(upnp::shared_prefix_length("", ""), -1); +} + +TEST(UpnpUrlHostTest, ExtractsHost) { + EXPECT_EQ(upnp::url_host("http://192.168.1.100:1900/rootDesc.xml"), "192.168.1.100"); + EXPECT_EQ(upnp::url_host("http://10.0.0.1/rootDesc.xml"), "10.0.0.1"); + EXPECT_EQ(upnp::url_host("http://[fe80::1]:5000/rootDesc.xml"), "fe80::1"); + EXPECT_EQ(upnp::url_host("192.168.1.1:80"), "192.168.1.1"); +} + +TEST(UpnpSelectInnerIgdTest, KeepsFirstIgdWhenAlone) { + EXPECT_EQ(upnp::select_inner_igd({"192.168.1.1"}, "192.168.1.20"), 0U); + EXPECT_EQ(upnp::select_inner_igd({}, "192.168.1.20"), 0U); +} + +TEST(UpnpSelectInnerIgdTest, PrefersIgdOnHostNetwork) { + // Double NAT: UPNP_GetValidIGD() chose the outer router (public address), the inner router shares the host's network. + EXPECT_EQ(upnp::select_inner_igd({"10.0.0.1", "192.168.1.100"}, "192.168.20.181"), 1U); +} + +TEST(UpnpSelectInnerIgdTest, KeepsFirstIgdOnTie) { + EXPECT_EQ(upnp::select_inner_igd({"192.168.1.1", "192.168.1.2"}, "192.168.1.20"), 0U); + EXPECT_EQ(upnp::select_inner_igd({"router.local", "gateway.local"}, "192.168.1.20"), 0U); +} + +TEST(UpnpSelectOuterIgdTest, FindsIgdInFrontOfGateway) { + const std::vector hosts {"192.168.1.100", "10.0.0.1"}; + EXPECT_EQ(upnp::select_outer_igd(hosts, 0, "10.0.0.86"), 1U); +} + +TEST(UpnpSelectOuterIgdTest, IgnoresGatewayWithPublicAddress) { + const std::vector hosts {"192.168.1.100", "10.0.0.1"}; + EXPECT_EQ(upnp::select_outer_igd(hosts, 0, "203.0.113.5"), std::nullopt); +} + +TEST(UpnpSelectOuterIgdTest, IgnoresMissingOrInvalidExternalAddress) { + const std::vector hosts {"192.168.1.100", "10.0.0.1"}; + EXPECT_EQ(upnp::select_outer_igd(hosts, 0, ""), std::nullopt); + EXPECT_EQ(upnp::select_outer_igd(hosts, 0, "not-an-address"), std::nullopt); + EXPECT_EQ(upnp::select_outer_igd(hosts, 0, "fc00::1"), std::nullopt); +} + +TEST(UpnpSelectOuterIgdTest, IgnoresInnerGatewayAndUnrelatedIgds) { + // The only other IGD is on the host's own network, not in front of the gateway. + EXPECT_EQ(upnp::select_outer_igd({"192.168.1.100", "192.168.1.2"}, 0, "10.0.0.86"), std::nullopt); + EXPECT_EQ(upnp::select_outer_igd({"10.0.0.1"}, 0, "10.0.0.86"), std::nullopt); +} + +TEST(UpnpSelectOuterIgdTest, PrefersClosestIgd) { + const std::vector hosts {"192.168.1.100", "10.0.5.1", "10.0.0.1"}; + EXPECT_EQ(upnp::select_outer_igd(hosts, 0, "10.0.0.86"), 2U); +} From a8f61966fcdc67e3dc710c303eaf71028e274ef7 Mon Sep 17 00:00:00 2001 From: Enrique Wood Date: Mon, 9 Nov 2026 23:09:03 -0300 Subject: [PATCH 2/6] refactor(upnp): split mapping pass to address Sonar findings --- src/upnp.cpp | 117 +++++++++++++++++++++++++-------------- tests/unit/test_upnp.cpp | 16 ++---- 2 files changed, 82 insertions(+), 51 deletions(-) diff --git a/src/upnp.cpp b/src/upnp.cpp index e9ccfeb6a43..74fe4efdbf9 100644 --- a/src/upnp.cpp +++ b/src/upnp.cpp @@ -448,6 +448,74 @@ namespace upnp { } } + /** + * @brief Maps every port on an IGD, stopping early on shutdown. + * @param igd The IGD to map the ports on. + * @param addr Address the ports are forwarded to. + */ + void map_all_upnp_ports(const igd_t &igd, const std::string &addr) { + auto shutdown_event = mail::man->event(mail::shutdown); + for (auto it = std::begin(mappings); it != std::end(mappings) && !shutdown_event->peek(); ++it) { + map_upnp_port(igd.data, igd.urls, addr, *it); + } + } + + /** + * @brief Find the IGD of the network this host is attached to. + * @details Behind two routers, the IGD chosen by UPNP_GetValidIGD() may be the outer one, which can't reach + * this host. The other IGDs that answered discovery are described and the closest one is selected. + * @param device Device list from upnpDiscover(). + * @param igds List holding the IGD chosen by UPNP_GetValidIGD(); the other IGDs are appended to it. + * @param igd_hosts Receives the address of each IGD in `igds`. + * @param lan_addr This host's local address. + * @return Index of the selected IGD in `igds`. + */ + static std::size_t find_gateway(const device_t &device, std::vector &igds, std::vector &igd_hosts, const std::string &lan_addr) { + igds.front().host = url_host(igds.front().urls->rootdescURL); + add_other_igds(device, igds); + for (const auto &igd : igds) { + igd_hosts.push_back(igd.host); + } + + const auto inner = select_inner_igd(igd_hosts, lan_addr); + if (inner != 0) { + BOOST_LOG(debug) << "Using the IGD on this host's network instead: "sv << igds[inner].urls->rootdescURL; + } + + return inner; + } + + /** + * @brief Behind double NAT, map the ports on the IGD in front of the gateway. + * @details If the gateway's external address is private, the same ports are mapped on the IGD in front of it, + * forwarded to the gateway's external address. That IGD is kept to remove the mappings on shutdown. + * @param igds The discovered IGDs. + * @param igd_hosts Address of each IGD in `igds`. + * @param inner Index of the gateway in `igds`. + * @param first_pass Whether this is the first mapping pass since the mappings were last completed. + */ + void map_outer_upnp_ports(std::vector &igds, const std::vector &igd_hosts, std::size_t inner, bool first_pass) { + const auto external_addr = external_address(igds[inner]); + const auto outer = select_outer_igd(igd_hosts, inner, external_addr); + if (!outer.has_value()) { + if (is_private_ipv4(external_addr) && !warned_no_outer_igd) { + BOOST_LOG(warning) << "The IGD's external address "sv << external_addr + << " is private and no UPnP IGD answered in front of it: ports must be forwarded on the outer router"sv; + warned_no_outer_igd = true; + } + return; + } + + auto &outer_igd = igds[*outer]; + map_all_upnp_ports(outer_igd, external_addr); + if (first_pass || !mapped_outer_igd.has_value() || mapped_outer_igd->host != outer_igd.host) { + BOOST_LOG(info) << "Completed UPnP port mappings to "sv << external_addr << " via "sv << outer_igd.urls->rootdescURL; + } + + mapped_outer_igd = std::move(outer_igd); + warned_no_outer_igd = false; + } + /** * @brief Maintains UPnP port forwarding rules */ @@ -456,8 +524,6 @@ namespace upnp { auto shutdown_event = mail::man->event(mail::shutdown); bool mapped = false; igd_t mapped_igd; - std::optional mapped_outer_igd; - bool warned_no_outer_igd = false; auto address_family = net::get_effective_address_family(net::af_from_enum_string(config::sunshine.address_family)); // Refresh UPnP rules every few minutes. They can be lost if the router reboots, @@ -489,48 +555,15 @@ namespace upnp { BOOST_LOG(debug) << "Found valid IGD device: "sv << igds.front().urls->rootdescURL; - // Behind two routers, the IGD chosen above may be the outer one, which can't reach this host. - igds.front().host = url_host(igds.front().urls->rootdescURL); - add_other_igds(device, igds); std::vector igd_hosts; - for (const auto &igd : igds) { - igd_hosts.push_back(igd.host); - } - - const auto inner = select_inner_igd(igd_hosts, lan_addr_str); - auto &igd = igds[inner]; - if (inner != 0) { - BOOST_LOG(debug) << "Using the IGD on this host's network instead: "sv << igd.urls->rootdescURL; - } - - for (auto it = std::begin(mappings); it != std::end(mappings) && !shutdown_event->peek(); ++it) { - map_upnp_port(igd.data, igd.urls, lan_addr_str, *it); - } + const auto inner = find_gateway(device, igds, igd_hosts, lan_addr_str); + map_all_upnp_ports(igds[inner], lan_addr_str); if (!mapped) { - BOOST_LOG(info) << "Completed UPnP port mappings to "sv << lan_addr_str << " via "sv << igd.urls->rootdescURL; + BOOST_LOG(info) << "Completed UPnP port mappings to "sv << lan_addr_str << " via "sv << igds[inner].urls->rootdescURL; } - // Double NAT: if the gateway's external address is private, map the same ports on the IGD in front of it, - // forwarding them to the gateway's external address. - const auto external_addr = external_address(igd); - if (const auto outer = select_outer_igd(igd_hosts, inner, external_addr)) { - auto &outer_igd = igds[*outer]; - for (auto it = std::begin(mappings); it != std::end(mappings) && !shutdown_event->peek(); ++it) { - map_upnp_port(outer_igd.data, outer_igd.urls, external_addr, *it); - } - - if (!mapped || !mapped_outer_igd || mapped_outer_igd->host != outer_igd.host) { - BOOST_LOG(info) << "Completed UPnP port mappings to "sv << external_addr << " via "sv << outer_igd.urls->rootdescURL; - } - - mapped_outer_igd = std::move(outer_igd); - warned_no_outer_igd = false; - } else if (is_private_ipv4(external_addr) && !warned_no_outer_igd) { - BOOST_LOG(warning) << "The IGD's external address "sv << external_addr - << " is private and no UPnP IGD answered in front of it: ports must be forwarded on the outer router"sv; - warned_no_outer_igd = true; - } + map_outer_upnp_ports(igds, igd_hosts, inner, !mapped); // If we are listening on IPv6 and the IGD has an IPv6 firewall enabled, try to create IPv6 firewall pinholes if (address_family == net::af_e::BOTH) { @@ -541,20 +574,22 @@ namespace upnp { } mapped = true; - mapped_igd = std::move(igd); + mapped_igd = std::move(igds[inner]); } while (!shutdown_event->view(REFRESH_INTERVAL)); if (mapped) { // Unmap ports upon termination BOOST_LOG(info) << "Unmapping UPNP ports..."sv; unmap_all_upnp_ports(mapped_igd.urls, mapped_igd.data); - if (mapped_outer_igd) { + if (mapped_outer_igd.has_value()) { unmap_all_upnp_ports(mapped_outer_igd->urls, mapped_outer_igd->data); } } } std::vector mappings; ///< Port mappings Sunshine should keep registered with the gateway. + std::optional mapped_outer_igd; ///< IGD in front of the gateway (double NAT) holding mappings, if any. + bool warned_no_outer_igd = false; ///< Whether the missing outer IGD has already been reported. std::jthread upnp_thread; ///< Worker thread that refreshes mappings until shutdown. }; diff --git a/tests/unit/test_upnp.cpp b/tests/unit/test_upnp.cpp index 18c7e781d69..dfe809a1df4 100644 --- a/tests/unit/test_upnp.cpp +++ b/tests/unit/test_upnp.cpp @@ -77,20 +77,17 @@ TEST(UpnpSelectInnerIgdTest, KeepsFirstIgdOnTie) { } TEST(UpnpSelectOuterIgdTest, FindsIgdInFrontOfGateway) { - const std::vector hosts {"192.168.1.100", "10.0.0.1"}; - EXPECT_EQ(upnp::select_outer_igd(hosts, 0, "10.0.0.86"), 1U); + EXPECT_EQ(upnp::select_outer_igd({"192.168.1.100", "10.0.0.1"}, 0, "10.0.0.86"), 1U); } TEST(UpnpSelectOuterIgdTest, IgnoresGatewayWithPublicAddress) { - const std::vector hosts {"192.168.1.100", "10.0.0.1"}; - EXPECT_EQ(upnp::select_outer_igd(hosts, 0, "203.0.113.5"), std::nullopt); + EXPECT_EQ(upnp::select_outer_igd({"192.168.1.100", "10.0.0.1"}, 0, "203.0.113.5"), std::nullopt); } TEST(UpnpSelectOuterIgdTest, IgnoresMissingOrInvalidExternalAddress) { - const std::vector hosts {"192.168.1.100", "10.0.0.1"}; - EXPECT_EQ(upnp::select_outer_igd(hosts, 0, ""), std::nullopt); - EXPECT_EQ(upnp::select_outer_igd(hosts, 0, "not-an-address"), std::nullopt); - EXPECT_EQ(upnp::select_outer_igd(hosts, 0, "fc00::1"), std::nullopt); + EXPECT_EQ(upnp::select_outer_igd({"192.168.1.100", "10.0.0.1"}, 0, ""), std::nullopt); + EXPECT_EQ(upnp::select_outer_igd({"192.168.1.100", "10.0.0.1"}, 0, "not-an-address"), std::nullopt); + EXPECT_EQ(upnp::select_outer_igd({"192.168.1.100", "10.0.0.1"}, 0, "fc00::1"), std::nullopt); } TEST(UpnpSelectOuterIgdTest, IgnoresInnerGatewayAndUnrelatedIgds) { @@ -100,6 +97,5 @@ TEST(UpnpSelectOuterIgdTest, IgnoresInnerGatewayAndUnrelatedIgds) { } TEST(UpnpSelectOuterIgdTest, PrefersClosestIgd) { - const std::vector hosts {"192.168.1.100", "10.0.5.1", "10.0.0.1"}; - EXPECT_EQ(upnp::select_outer_igd(hosts, 0, "10.0.0.86"), 2U); + EXPECT_EQ(upnp::select_outer_igd({"192.168.1.100", "10.0.5.1", "10.0.0.1"}, 0, "10.0.0.86"), 2U); } From 5ddce01e54f052cfcf888933765740acf035e2b2 Mon Sep 17 00:00:00 2001 From: Enrique Wood Date: Sat, 14 Nov 2026 04:23:29 -0300 Subject: [PATCH 3/6] fix(upnp): preserve mapping ownership and recover permanent leases Signed-off-by: Enrique Wood --- docs/configuration.md | 4 + src/upnp.cpp | 325 ++++++++++++++++++------------- src/upnp_mapping.h | 118 +++++++++++ tests/unit/test_upnp_mapping.cpp | 103 ++++++++++ 4 files changed, 411 insertions(+), 139 deletions(-) create mode 100644 src/upnp_mapping.h create mode 100644 tests/unit/test_upnp_mapping.cpp diff --git a/docs/configuration.md b/docs/configuration.md index f57b894e33a..c2bc5bb7897 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1555,6 +1555,10 @@ consent prompt for directory ACLs, so the Web UI shows setup steps for correctin Sunshine will attempt to open ports for streaming over the internet. If Sunshine's router is behind another router (double NAT) and both support UPnP, the ports are also opened on the outer router, forwarded to the inner router. + Existing rules for another destination are left unchanged, and only rules owned by Sunshine are removed. + Rules without an expiry are used only when the router explicitly requires them; they are recorded in + upnp_mappings.journal in Sunshine's application data directory before creation. + At the next start Sunshine attempts to remove outstanding recorded rules, even if UPnP has since been disabled. diff --git a/src/upnp.cpp b/src/upnp.cpp index 74fe4efdbf9..8d6deffb9ba 100644 --- a/src/upnp.cpp +++ b/src/upnp.cpp @@ -5,7 +5,10 @@ // standard includes #include #include +#include +#include #include +#include #include // workaround for type_t error in miniupnpc 2.3.3, see https://github.com/miniupnp/miniupnp/commit/e263ab6f56c382e10fed31347ec68095d691a0e8 // lib includes @@ -22,6 +25,7 @@ #include "rtsp.h" #include "stream.h" #include "upnp.h" +#include "upnp_mapping.h" #include "utility.h" using namespace std::literals; @@ -315,136 +319,142 @@ namespace upnp { } /** - * @brief Maps a port via UPnP. - * @param data IGDdatas from UPNP_GetValidIGD() - * @param urls urls_t from UPNP_GetValidIGD() - * @param lan_addr Local IP address to map to - * @param mapping Information about port to map - * @return `true` on success. + * @brief Append recovery intent before creating a permanent rule, or completion after removing it. + * @param mapping Rule to record. + * @param added Whether this records creation intent. + * @return Whether the complete event reached the file stream successfully. */ - bool map_upnp_port(const IGDdatas &data, const urls_t &urls, const std::string &lan_addr, const mapping_t &mapping) { - std::array intClient; - std::array intPort; - std::array desc; - std::array enabled; - std::array leaseDuration; - bool indefinite = false; - - // First check if this port is already mapped successfully - BOOST_LOG(debug) << "Checking for existing UPnP port mapping for "sv << mapping.port.wan; - auto err = UPNP_GetSpecificPortMappingEntry( - urls->controlURL, - data.first.servicetype, - // In params - mapping.port.wan.c_str(), - mapping.port.proto.c_str(), - nullptr, - // Out params - intClient.data(), - intPort.data(), - desc.data(), - enabled.data(), - leaseDuration.data() - ); - if (err == 714) { // NoSuchEntryInArray - BOOST_LOG(debug) << "Mapping entry not found for "sv << mapping.port.wan; - } else if (err == UPNPCOMMAND_SUCCESS) { - // Some routers change the description, so we can't check that here - if (std::string_view {intClient.data()} == lan_addr) { - if (std::atoi(leaseDuration.data()) == 0) { - BOOST_LOG(debug) << "Static mapping entry found for "sv << mapping.port.wan; - - // It's a static mapping, so we're done here - return true; - } else { - BOOST_LOG(debug) << "Mapping entry found for "sv << mapping.port.wan << " ("sv << leaseDuration.data() << " seconds remaining)"sv; - } - } else { - BOOST_LOG(warning) << "UPnP conflict detected with: "sv << intClient.data(); - - // Some UPnP IGDs won't let unauthenticated clients delete other conflicting port mappings - // for security reasons, but we will give it a try anyway. - err = UPNP_DeletePortMapping( - urls->controlURL, - data.first.servicetype, - mapping.port.wan.c_str(), - mapping.port.proto.c_str(), - nullptr - ); - if (err) { - BOOST_LOG(error) << "Unable to delete conflicting UPnP port mapping: "sv << err; - return false; - } - } + bool journal_mapping(const owned_mapping_t &mapping, bool added) { + std::ofstream out {journal_path(), std::ios::app}; + out << mapping_journal_event(mapping, added); + out.flush(); + out.close(); + if (!out) { + BOOST_LOG(warning) << "Could not update UPnP recovery journal; refusing an unrecorded permanent mapping"sv; + return false; + } + return true; + } + + /** + * @brief Remember a rule independently of discovery and persist permanent-rule intent before adding it. + * @param mapping Rule owned by Sunshine. + * @return Whether the rule can safely be managed. + */ + bool remember_mapping(owned_mapping_t mapping) { + auto found = std::find(owned_mappings.begin(), owned_mappings.end(), mapping); + if (mapping.permanent && (found == owned_mappings.end() || !found->permanent) && !journal_mapping(mapping, true)) { + return false; + } + if (found == owned_mappings.end()) { + owned_mappings.push_back(std::move(mapping)); } else { - BOOST_LOG(error) << "UPNP_GetSpecificPortMappingEntry() failed: "sv << err; + found->generation = generation; + found->permanent = found->permanent || mapping.permanent; + } + return true; + } - // If we get a strange error from the router, we'll assume it's some old broken IGDv1 - // device and only use indefinite lease durations to hopefully avoid confusing it. - if (err != 606) { // Unauthorized - indefinite = true; + /** + * @brief Maps one port without deleting or adopting a rule belonging to another application. + * @param data Gateway's WAN connection service. + * @param urls Gateway's control URLs. + * @param lan_addr Desired destination address. + * @param mapping Desired forwarding rule. + * @return Whether the desired destination is reachable through this mapping. + */ + bool map_upnp_port(const IGDdatas &data, const urls_t &urls, const std::string &lan_addr, const mapping_t &mapping) { + owned_mapping_t wanted {urls->controlURL, data.first.servicetype, lan_addr, mapping.port.wan, mapping.port.lan, mapping.port.proto, mapping.description, false, generation}; + // A failed refresh must not forget a rule that was successfully added earlier. + for (auto &owned : owned_mappings) { + if (owned == wanted) { + owned.generation = generation; } } - - // Add/update the port mapping - auto mapping_period = std::to_string(indefinite ? 0 : PORT_MAPPING_LIFETIME.count()); - err = UPNP_AddPortMapping( - urls->controlURL, - data.first.servicetype, - mapping.port.wan.c_str(), - mapping.port.lan.c_str(), - lan_addr.data(), - mapping.description.c_str(), - mapping.port.proto.c_str(), - nullptr, - mapping_period.c_str() - ); - - if (err != UPNPCOMMAND_SUCCESS && !indefinite) { - // This may be an old/broken IGD that doesn't like non-static mappings. - BOOST_LOG(debug) << "Trying static mapping after failure: "sv << err; - err = UPNP_AddPortMapping( - urls->controlURL, - data.first.servicetype, - mapping.port.wan.c_str(), - mapping.port.lan.c_str(), - lan_addr.data(), - mapping.description.c_str(), - mapping.port.proto.c_str(), - nullptr, - "0" - ); + std::array client {}; + std::array port {}; + std::array description {}; + std::array enabled {}; + std::array lease {}; + auto err = UPNP_GetSpecificPortMappingEntry(urls->controlURL, data.first.servicetype, mapping.port.wan.c_str(), mapping.port.proto.c_str(), nullptr, client.data(), port.data(), description.data(), enabled.data(), lease.data()); + switch (classify_mapping(err, client.data(), port.data(), description.data(), wanted)) { + case mapping_state_e::conflict: + BOOST_LOG(warning) << "UPnP port "sv << mapping.port.wan << " already forwards elsewhere; leaving it unchanged"sv; + return false; + case mapping_state_e::existing: + // A manual rule already does what we need. Never renew or remove it. + return true; + case mapping_state_e::unknown: + BOOST_LOG(warning) << "UPnP mapping ownership could not be checked: "sv << err; + return false; + case mapping_state_e::ours: + wanted.permanent = std::string_view {lease.data()} == "0"sv; + if (!remember_mapping(wanted)) { + return false; + } + if (wanted.permanent) { + return true; + } + break; + case mapping_state_e::missing: + break; } - - if (err) { - BOOST_LOG(error) << "Failed to map "sv << mapping.port.proto << ' ' << mapping.port.lan << ": "sv << err; + auto shutdown_event = mail::man->event(mail::shutdown); + if (shutdown_event->peek()) { return false; } - - BOOST_LOG(debug) << "Successfully mapped "sv << mapping.port.proto << ' ' << mapping.port.lan; - return true; + const auto period = std::to_string(PORT_MAPPING_LIFETIME.count()); + err = UPNP_AddPortMapping(urls->controlURL, data.first.servicetype, mapping.port.wan.c_str(), mapping.port.lan.c_str(), lan_addr.c_str(), mapping.description.c_str(), mapping.port.proto.c_str(), nullptr, period.c_str()); + if (err == 725 && !shutdown_event->peek()) { // OnlyPermanentLeasesSupported + wanted.permanent = true; + // Persist intent first: a process crash immediately after AddPortMapping must be recoverable. + if (!remember_mapping(wanted)) { + return false; + } + err = UPNP_AddPortMapping(urls->controlURL, data.first.servicetype, mapping.port.wan.c_str(), mapping.port.lan.c_str(), lan_addr.c_str(), mapping.description.c_str(), mapping.port.proto.c_str(), nullptr, "0"); + } + if (err != UPNPCOMMAND_SUCCESS) { + BOOST_LOG(warning) << "Failed to map "sv << mapping.port.proto << ' ' << mapping.port.wan << ": "sv << err; + return false; + } + return remember_mapping(std::move(wanted)); } /** - * @brief Unmaps all ports. - * @param urls urls_t from UPNP_GetValidIGD() - * @param data IGDdatas from UPNP_GetValidIGD() + * @brief Remove only recorded rules whose current destination, port and description still match. + * @param stale_only Whether to retain rules wanted by the current discovery pass. */ - void unmap_all_upnp_ports(const urls_t &urls, const IGDdatas &data) { - for (auto it = std::begin(mappings); it != std::end(mappings); ++it) { - auto status = UPNP_DeletePortMapping( - urls->controlURL, - data.first.servicetype, - it->port.wan.c_str(), - it->port.proto.c_str(), - nullptr - ); - - if (status && status != 714) { // NoSuchEntryInArray - BOOST_LOG(warning) << "Failed to unmap "sv << it->port.proto << ' ' << it->port.lan << ": "sv << status; - } else { - BOOST_LOG(debug) << "Successfully unmapped "sv << it->port.proto << ' ' << it->port.lan; + void unmap_owned_ports(bool stale_only = false) { + std::erase_if(owned_mappings, [this, stale_only](const owned_mapping_t &owned) { + if (stale_only && owned.generation == generation) { + return false; + } + std::array client {}; + std::array port {}; + std::array description {}; + std::array enabled {}; + std::array lease {}; + const auto lookup = UPNP_GetSpecificPortMappingEntry(owned.control_url.c_str(), owned.service.c_str(), owned.external_port.c_str(), owned.protocol.c_str(), nullptr, client.data(), port.data(), description.data(), enabled.data(), lease.data()); + const auto state = classify_mapping(lookup, client.data(), port.data(), description.data(), owned); + if (state == mapping_state_e::unknown) { + BOOST_LOG(warning) << "Could not check UPnP rule for cleanup: "sv << owned.control_url << ' ' << owned.external_port; + return false; + } + if (state == mapping_state_e::ours) { + const auto removed = UPNP_DeletePortMapping(owned.control_url.c_str(), owned.service.c_str(), owned.external_port.c_str(), owned.protocol.c_str(), nullptr); + if (removed != UPNPCOMMAND_SUCCESS && removed != 714) { + BOOST_LOG(warning) << "Failed to remove UPnP rule: "sv << removed; + return false; + } } + // Missing or replaced rules are forgotten without deleting the replacement. + return !owned.permanent || journal_mapping(owned, false); + }); + if (std::none_of(owned_mappings.begin(), owned_mappings.end(), [](const auto &mapping) { + return mapping.permanent; + })) { + std::error_code ec; + std::filesystem::remove(journal_path(), ec); } } @@ -452,12 +462,18 @@ namespace upnp { * @brief Maps every port on an IGD, stopping early on shutdown. * @param igd The IGD to map the ports on. * @param addr Address the ports are forwarded to. + * @return Whether every requested mapping succeeded before shutdown. */ - void map_all_upnp_ports(const igd_t &igd, const std::string &addr) { + bool map_all_upnp_ports(const igd_t &igd, const std::string &addr) { auto shutdown_event = mail::man->event(mail::shutdown); - for (auto it = std::begin(mappings); it != std::end(mappings) && !shutdown_event->peek(); ++it) { - map_upnp_port(igd.data, igd.urls, addr, *it); + bool complete = true; + for (const auto &mapping : mappings) { + if (shutdown_event->peek()) { + return false; + } + complete = map_upnp_port(igd.data, igd.urls, addr, mapping) && complete; } + return complete; } /** @@ -498,6 +514,12 @@ namespace upnp { const auto external_addr = external_address(igds[inner]); const auto outer = select_outer_igd(igd_hosts, inner, external_addr); if (!outer.has_value()) { + // A missing reply is not proof that a working outer mapping is obsolete. + for (auto &owned : owned_mappings) { + if (external_addr.empty() || (is_private_ipv4(external_addr) && owned.target == external_addr)) { + owned.generation = generation; + } + } if (is_private_ipv4(external_addr) && !warned_no_outer_igd) { BOOST_LOG(warning) << "The IGD's external address "sv << external_addr << " is private and no UPnP IGD answered in front of it: ports must be forwarded on the outer router"sv; @@ -507,12 +529,14 @@ namespace upnp { } auto &outer_igd = igds[*outer]; - map_all_upnp_ports(outer_igd, external_addr); - if (first_pass || !mapped_outer_igd.has_value() || mapped_outer_igd->host != outer_igd.host) { + const bool complete = map_all_upnp_ports(outer_igd, external_addr); + if (complete && (first_pass || mapped_outer_host != outer_igd.host)) { BOOST_LOG(info) << "Completed UPnP port mappings to "sv << external_addr << " via "sv << outer_igd.urls->rootdescURL; } - mapped_outer_igd = std::move(outer_igd); + if (complete) { + mapped_outer_host = outer_igd.host; + } warned_no_outer_igd = false; } @@ -522,8 +546,25 @@ namespace upnp { void upnp_thread_proc() { platf::set_thread_name("upnp"); auto shutdown_event = mail::man->event(mail::shutdown); + std::ifstream journal {journal_path()}; + std::error_code journal_error; + const bool journal_exists = std::filesystem::exists(journal_path(), journal_error); + if (journal_error || (journal_exists && !journal.is_open())) { + BOOST_LOG(error) << "Could not read UPnP recovery journal; leaving it intact and skipping new mappings"sv; + return; + } + const std::string contents {(std::istreambuf_iterator(journal)), std::istreambuf_iterator()}; + if (journal.bad()) { + BOOST_LOG(error) << "Could not finish reading UPnP recovery journal; leaving it intact"sv; + return; + } + journal.close(); + owned_mappings = read_mapping_journal(contents); + unmap_owned_ports(); + if (!config::sunshine.flags[config::flag::UPNP]) { + return; + } bool mapped = false; - igd_t mapped_igd; auto address_family = net::get_effective_address_family(net::af_from_enum_string(config::sunshine.address_family)); // Refresh UPnP rules every few minutes. They can be lost if the router reboots, @@ -557,13 +598,15 @@ namespace upnp { std::vector igd_hosts; const auto inner = find_gateway(device, igds, igd_hosts, lan_addr_str); - map_all_upnp_ports(igds[inner], lan_addr_str); + ++generation; + const bool complete = map_all_upnp_ports(igds[inner], lan_addr_str); - if (!mapped) { + if (!mapped && complete) { BOOST_LOG(info) << "Completed UPnP port mappings to "sv << lan_addr_str << " via "sv << igds[inner].urls->rootdescURL; } map_outer_upnp_ports(igds, igd_hosts, inner, !mapped); + unmap_owned_ports(true); // If we are listening on IPv6 and the IGD has an IPv6 firewall enabled, try to create IPv6 firewall pinholes if (address_family == net::af_e::BOTH) { @@ -573,22 +616,25 @@ namespace upnp { } } - mapped = true; - mapped_igd = std::move(igds[inner]); + mapped = complete; } while (!shutdown_event->view(REFRESH_INTERVAL)); - if (mapped) { - // Unmap ports upon termination - BOOST_LOG(info) << "Unmapping UPNP ports..."sv; - unmap_all_upnp_ports(mapped_igd.urls, mapped_igd.data); - if (mapped_outer_igd.has_value()) { - unmap_all_upnp_ports(mapped_outer_igd->urls, mapped_outer_igd->data); - } - } + // Discovery failures and gateway changes never discard ownership records. + unmap_owned_ports(); + } + + /** + * @brief Locate the append-only recovery journal used even when UPnP is disabled at the next start. + * @return File containing outstanding permanent-rule intents. + */ + static std::filesystem::path journal_path() { + return platf::appdata() / "upnp_mappings.journal"; } - std::vector mappings; ///< Port mappings Sunshine should keep registered with the gateway. - std::optional mapped_outer_igd; ///< IGD in front of the gateway (double NAT) holding mappings, if any. + std::vector mappings; ///< Ports requested by Sunshine. + std::vector owned_mappings; ///< Rules retained independently of discovery results. + std::string mapped_outer_host; ///< Last outer gateway logged as mapped. + std::size_t generation = 0; ///< Current successful discovery pass. bool warned_no_outer_igd = false; ///< Whether the missing outer IGD has already been reported. std::jthread upnp_thread; ///< Worker thread that refreshes mappings until shutdown. }; @@ -597,7 +643,8 @@ namespace upnp { * @brief Start UPnP port mapping and return its shutdown guard. */ std::unique_ptr start() { - if (!config::sunshine.flags[config::flag::UPNP]) { + std::error_code ec; + if (!config::sunshine.flags[config::flag::UPNP] && !std::filesystem::exists(platf::appdata() / "upnp_mappings.journal", ec)) { return nullptr; } diff --git a/src/upnp_mapping.h b/src/upnp_mapping.h new file mode 100644 index 00000000000..693e395438c --- /dev/null +++ b/src/upnp_mapping.h @@ -0,0 +1,118 @@ +/** + * @file src/upnp_mapping.h + * @brief Ownership and recovery records for UPnP port mappings. + */ +#pragma once + +// standard includes +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace upnp { + /** + * @brief A forwarding rule Sunshine owns, including its gateway and destination. + */ + struct owned_mapping_t { + std::string control_url; ///< Gateway's SOAP endpoint. + std::string service; ///< Gateway's WAN connection service. + std::string target; ///< Host or inner router receiving the forwarded traffic. + std::string external_port; ///< Gateway port. + std::string internal_port; ///< Destination port. + std::string protocol; ///< TCP or UDP. + std::string description; ///< Description used when Sunshine created the rule. + bool permanent = false; ///< Rule requires recovery after an unexpected process exit. + std::size_t generation = 0; ///< Last successful discovery pass that wanted this exact rule. + + /** + * @brief Compare persistent rule identities, excluding refresh bookkeeping. + * @param other Rule to compare. + * @return Whether both records identify the same forwarding rule. + */ + bool operator==(const owned_mapping_t &other) const { + return std::tie(control_url, service, target, external_port, internal_port, protocol, description) == + std::tie(other.control_url, other.service, other.target, other.external_port, other.internal_port, other.protocol, other.description); + } + }; + + /** + * @brief Relationship between a gateway's existing mapping and the requested rule. + */ + enum class mapping_state_e { + missing, ///< Gateway explicitly reports no such mapping. + ours, ///< Destination, port and description match Sunshine's record. + existing, ///< Someone else's rule already forwards to the requested destination. + conflict, ///< Rule forwards elsewhere and must be left untouched. + unknown, ///< Lookup failed; ownership cannot be established. + }; + + /** + * @brief Classify a lookup without treating a destination alone as proof of ownership. + * @param result MiniUPnPc lookup result. + * @param client Reported destination address. + * @param port Reported destination port. + * @param description Reported description. + * @param wanted Requested or previously recorded rule. + * @return Ownership classification. + */ + inline mapping_state_e classify_mapping(int result, std::string_view client, std::string_view port, std::string_view description, const owned_mapping_t &wanted) { + if (result == 714) { + return mapping_state_e::missing; + } + if (result != 0) { + return mapping_state_e::unknown; + } + if (client != wanted.target || port != wanted.internal_port) { + return mapping_state_e::conflict; + } + return description == wanted.description ? mapping_state_e::ours : mapping_state_e::existing; + } + + /** + * @brief Encode one append-only recovery event before adding or after removing a permanent rule. + * @param mapping Rule being recorded. + * @param added Whether this records creation intent rather than completed removal. + * @return A newline-delimited event; the leading newline isolates a previously interrupted write. + */ + inline std::string mapping_journal_event(const owned_mapping_t &mapping, bool added) { + std::ostringstream out; + out << '\n' + << (added ? '+' : '-') << ' ' << std::quoted(mapping.control_url) << ' ' << std::quoted(mapping.service) << ' ' + << std::quoted(mapping.target) << ' ' << std::quoted(mapping.external_port) << ' ' << std::quoted(mapping.internal_port) + << ' ' << std::quoted(mapping.protocol) << ' ' << std::quoted(mapping.description) << '\n'; + return out.str(); + } + + /** + * @brief Recover outstanding permanent rules, ignoring incomplete or invalid journal lines. + * @param text Previously written recovery events. + * @return Rules whose creation intent has no matching completed removal. + */ + inline std::vector read_mapping_journal(const std::string &text) { + std::vector mappings; + std::istringstream input {text}; + std::string line; + while (std::getline(input, line)) { + std::istringstream entry {line}; + char event = 0; + owned_mapping_t mapping; + if (!(entry >> event >> std::quoted(mapping.control_url) >> std::quoted(mapping.service) >> std::quoted(mapping.target) >> std::quoted(mapping.external_port) >> std::quoted(mapping.internal_port) >> std::quoted(mapping.protocol) >> std::quoted(mapping.description)) || (event != '+' && event != '-') || mapping.control_url.empty() || mapping.service.empty() || mapping.target.empty() || mapping.external_port.empty() || mapping.internal_port.empty() || (mapping.protocol != "TCP" && mapping.protocol != "UDP") || mapping.description.empty()) { + continue; + } + mapping.permanent = true; + const auto found = std::find(mappings.begin(), mappings.end(), mapping); + if (event == '+' && found == mappings.end()) { + mappings.push_back(std::move(mapping)); + } else if (event == '-' && found != mappings.end()) { + mappings.erase(found); + } + } + return mappings; + } +} // namespace upnp diff --git a/tests/unit/test_upnp_mapping.cpp b/tests/unit/test_upnp_mapping.cpp new file mode 100644 index 00000000000..a6fe3134642 --- /dev/null +++ b/tests/unit/test_upnp_mapping.cpp @@ -0,0 +1,103 @@ +/** + * @file tests/unit/test_upnp_mapping.cpp + * @brief Verify UPnP ownership boundaries and permanent-rule crash recovery. + */ + +// test includes +#include "../tests_common.h" + +// local includes +#include + +namespace { + /** + * @brief Construct a representative rule owned by Sunshine. + * @return Rule on the inner gateway. + */ + upnp::owned_mapping_t sample_mapping() { + return {"http://192.168.1.1/control", "urn:schemas-upnp-org:service:WANIPConnection:1", "192.168.1.20", "47989", "47989", "TCP", "Sunshine - Client HTTP"}; + } + + TEST(UpnpMappingOwnershipTest, DistinguishesOwnedManualAndConflictingRules) { + const auto mapping = sample_mapping(); + EXPECT_EQ(upnp::classify_mapping(0, mapping.target, mapping.internal_port, mapping.description, mapping), upnp::mapping_state_e::ours); + EXPECT_EQ(upnp::classify_mapping(0, mapping.target, mapping.internal_port, "Manual forwarding", mapping), upnp::mapping_state_e::existing); + EXPECT_EQ(upnp::classify_mapping(0, "192.168.1.99", mapping.internal_port, mapping.description, mapping), upnp::mapping_state_e::conflict); + EXPECT_EQ(upnp::classify_mapping(0, mapping.target, "48000", mapping.description, mapping), upnp::mapping_state_e::conflict); + } + + TEST(UpnpMappingOwnershipTest, LookupErrorsAreNeverProofOfOwnershipOrAbsence) { + const auto mapping = sample_mapping(); + EXPECT_EQ(upnp::classify_mapping(714, "", "", "", mapping), upnp::mapping_state_e::missing); + for (const int error : {-1, 501, 606, 718, 729}) { + EXPECT_EQ(upnp::classify_mapping(error, mapping.target, mapping.internal_port, mapping.description, mapping), upnp::mapping_state_e::unknown); + } + } + + TEST(UpnpMappingJournalTest, RecoversCreationIntentWithoutNeedingTheLastDiscoveryResult) { + const auto mapping = sample_mapping(); + const auto recovered = upnp::read_mapping_journal(upnp::mapping_journal_event(mapping, true)); + ASSERT_EQ(recovered.size(), 1); + EXPECT_EQ(recovered.front(), mapping); + EXPECT_TRUE(recovered.front().permanent); + } + + TEST(UpnpMappingJournalTest, KeepsBothGatewaysAndOldDestinationsUntilRemoved) { + auto inner = sample_mapping(); + auto outer = inner; + outer.control_url = "http://10.0.0.1/control"; + outer.target = "10.0.0.42"; + auto changed = outer; + changed.target = "10.0.0.43"; + auto events = upnp::mapping_journal_event(inner, true) + upnp::mapping_journal_event(outer, true) + upnp::mapping_journal_event(changed, true); + ASSERT_EQ(upnp::read_mapping_journal(events).size(), 3); + events += upnp::mapping_journal_event(outer, false); + const auto recovered = upnp::read_mapping_journal(events); + ASSERT_EQ(recovered.size(), 2); + EXPECT_EQ(recovered[0], inner); + EXPECT_EQ(recovered[1], changed); + } + + TEST(UpnpMappingJournalTest, RefreshesDoNotDuplicateAndCompletedRemovalClearsIntent) { + auto mapping = sample_mapping(); + auto events = upnp::mapping_journal_event(mapping, true); + mapping.generation = 123; + mapping.permanent = true; + events += upnp::mapping_journal_event(mapping, true); + EXPECT_EQ(upnp::read_mapping_journal(events).size(), 1); + events += upnp::mapping_journal_event(mapping, false); + events += upnp::mapping_journal_event(mapping, false); + EXPECT_TRUE(upnp::read_mapping_journal(events).empty()); + } + + TEST(UpnpMappingJournalTest, IgnoresInterruptedWritesWithoutLosingEarlierRules) { + const auto mapping = sample_mapping(); + const auto complete = upnp::mapping_journal_event(mapping, true); + const auto recovered = upnp::read_mapping_journal(complete + "+ \"http://incomplete"); + ASSERT_EQ(recovered.size(), 1); + EXPECT_EQ(recovered.front(), mapping); + EXPECT_TRUE(upnp::read_mapping_journal(complete + "+ \"http://incomplete" + upnp::mapping_journal_event(mapping, false)).empty()); + } + + TEST(UpnpMappingJournalTest, IgnoresInvalidEventsAndIncompleteIdentities) { + auto mapping = sample_mapping(); + auto event = upnp::mapping_journal_event(mapping, true); + event[1] = '?'; + EXPECT_TRUE(upnp::read_mapping_journal(event).empty()); + mapping.protocol = "SCTP"; + EXPECT_TRUE(upnp::read_mapping_journal(upnp::mapping_journal_event(mapping, true)).empty()); + mapping = sample_mapping(); + mapping.control_url.clear(); + EXPECT_TRUE(upnp::read_mapping_journal(upnp::mapping_journal_event(mapping, true)).empty()); + EXPECT_TRUE(upnp::read_mapping_journal("\nmalformed\n").empty()); + } + + TEST(UpnpMappingJournalTest, PreservesQuotedDescriptionsAndEndpointCharacters) { + auto mapping = sample_mapping(); + mapping.description = "Sunshine - \"Client HTTP\""; + mapping.control_url = "http://192.168.1.1/control?value=\\quoted"; + const auto recovered = upnp::read_mapping_journal(upnp::mapping_journal_event(mapping, true)); + ASSERT_EQ(recovered.size(), 1); + EXPECT_EQ(recovered.front(), mapping); + } +} // namespace From a13d24b4ca452944e4301d3bcca7d2a0b73e6e1e Mon Sep 17 00:00:00 2001 From: Enrique Wood Date: Sat, 14 Nov 2026 04:26:25 -0300 Subject: [PATCH 4/6] fix(upnp): validate recovery journal record boundaries Signed-off-by: Enrique Wood --- src/upnp.cpp | 7 ++++++- src/upnp_mapping.h | 7 ++++++- tests/unit/test_upnp_mapping.cpp | 9 +++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/upnp.cpp b/src/upnp.cpp index 8d6deffb9ba..6acefabdf57 100644 --- a/src/upnp.cpp +++ b/src/upnp.cpp @@ -325,8 +325,13 @@ namespace upnp { * @return Whether the complete event reached the file stream successfully. */ bool journal_mapping(const owned_mapping_t &mapping, bool added) { + const auto event = mapping_journal_event(mapping, added); + if (event.empty()) { + BOOST_LOG(warning) << "Invalid UPnP recovery identity; refusing an unrecorded permanent mapping"sv; + return false; + } std::ofstream out {journal_path(), std::ios::app}; - out << mapping_journal_event(mapping, added); + out << event; out.flush(); out.close(); if (!out) { diff --git a/src/upnp_mapping.h b/src/upnp_mapping.h index 693e395438c..dd6ae919b71 100644 --- a/src/upnp_mapping.h +++ b/src/upnp_mapping.h @@ -78,9 +78,14 @@ namespace upnp { * @brief Encode one append-only recovery event before adding or after removing a permanent rule. * @param mapping Rule being recorded. * @param added Whether this records creation intent rather than completed removal. - * @return A newline-delimited event; the leading newline isolates a previously interrupted write. + * @return A newline-delimited event, or an empty string for an invalid identity. */ inline std::string mapping_journal_event(const owned_mapping_t &mapping, bool added) { + for (const auto *field : {&mapping.control_url, &mapping.service, &mapping.target, &mapping.external_port, &mapping.internal_port, &mapping.protocol, &mapping.description}) { + if (field->empty() || field->find_first_of("\r\n") != std::string::npos) { + return {}; + } + } std::ostringstream out; out << '\n' << (added ? '+' : '-') << ' ' << std::quoted(mapping.control_url) << ' ' << std::quoted(mapping.service) << ' ' diff --git a/tests/unit/test_upnp_mapping.cpp b/tests/unit/test_upnp_mapping.cpp index a6fe3134642..bda7aab65b3 100644 --- a/tests/unit/test_upnp_mapping.cpp +++ b/tests/unit/test_upnp_mapping.cpp @@ -100,4 +100,13 @@ namespace { ASSERT_EQ(recovered.size(), 1); EXPECT_EQ(recovered.front(), mapping); } + + TEST(UpnpMappingJournalTest, RefusesLineBreaksThatWouldSplitARecoveryRecord) { + auto mapping = sample_mapping(); + mapping.control_url += "\nextra"; + EXPECT_TRUE(upnp::mapping_journal_event(mapping, true).empty()); + mapping = sample_mapping(); + mapping.description += '\r'; + EXPECT_TRUE(upnp::mapping_journal_event(mapping, true).empty()); + } } // namespace From 3a682f010d5aef0a3d0bdd5034a22a844c37b65b Mon Sep 17 00:00:00 2001 From: Enrique Wood Date: Sat, 14 Nov 2026 13:18:11 -0300 Subject: [PATCH 5/6] fix(upnp): do not count disabled manual mappings as open Signed-off-by: Enrique Wood --- docs/configuration.md | 1 + src/upnp.cpp | 178 +++++++++++++++------------- src/upnp_mapping.h | 142 +++++++++++++++++++++++ tests/unit/test_upnp_mapping.cpp | 191 +++++++++++++++++++++++++++++-- 4 files changed, 423 insertions(+), 89 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index c2bc5bb7897..96af7805dd8 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1556,6 +1556,7 @@ consent prompt for directory ACLs, so the Web UI shows setup steps for correctin If Sunshine's router is behind another router (double NAT) and both support UPnP, the ports are also opened on the outer router, forwarded to the inner router. Existing rules for another destination are left unchanged, and only rules owned by Sunshine are removed. + A disabled rule created by someone else is also left unchanged, and its port is reported as not opened. Rules without an expiry are used only when the router explicitly requires them; they are recorded in upnp_mappings.journal in Sunshine's application data directory before creation. At the next start Sunshine attempts to remove outstanding recorded rules, even if UPnP has since been disabled. diff --git a/src/upnp.cpp b/src/upnp.cpp index 6acefabdf57..a00186d5a31 100644 --- a/src/upnp.cpp +++ b/src/upnp.cpp @@ -212,6 +212,76 @@ namespace upnp { return best; } + /** + * @brief Gateway calls through MiniUPnPc. + */ + struct miniupnpc_gateway_t { + /** + * @brief Look up the rule on the requested gateway port. + * @param rule Rule whose gateway, service, port and protocol are looked up. + * @return The gateway's current rule. + */ + mapping_entry_t lookup(const owned_mapping_t &rule) const { + std::array client {}; + std::array port {}; + std::array description {}; + std::array enabled {}; + std::array lease {}; + const auto result = UPNP_GetSpecificPortMappingEntry(rule.control_url.c_str(), rule.service.c_str(), rule.external_port.c_str(), rule.protocol.c_str(), nullptr, client.data(), port.data(), description.data(), enabled.data(), lease.data()); + return {result, client.data(), port.data(), description.data(), enabled.data(), lease.data()}; + } + + /** + * @brief Add or renew a rule. + * @param rule Rule to add. + * @param lease Requested lease in seconds, `0` for a rule without expiry. + * @return MiniUPnPc result. + */ + int add(const owned_mapping_t &rule, const std::string &lease) const { + return UPNP_AddPortMapping(rule.control_url.c_str(), rule.service.c_str(), rule.external_port.c_str(), rule.internal_port.c_str(), rule.target.c_str(), rule.description.c_str(), rule.protocol.c_str(), nullptr, lease.c_str()); + } + + /** + * @brief Remove a rule. + * @param rule Rule to remove. + * @return MiniUPnPc result. + */ + int remove(const owned_mapping_t &rule) const { + return UPNP_DeletePortMapping(rule.control_url.c_str(), rule.service.c_str(), rule.external_port.c_str(), rule.protocol.c_str(), nullptr); + } + }; + + static_assert(mapping_gateway); + + /** + * @brief Log a mapping attempt that left the port closed. + * @param rule Requested rule. + * @param outcome Mapping outcome. + * @param error Last gateway result. + */ + static void report_mapping(const owned_mapping_t &rule, map_outcome_e outcome, int error) { + switch (outcome) { + case map_outcome_e::conflict: + BOOST_LOG(warning) << "UPnP port "sv << rule.external_port << " already forwards elsewhere; leaving it unchanged"sv; + break; + case map_outcome_e::disabled: + BOOST_LOG(warning) << "UPnP port "sv << rule.external_port << " has a disabled manual mapping; leaving it unchanged"sv; + break; + case map_outcome_e::unknown: + BOOST_LOG(warning) << "UPnP mapping ownership could not be checked: "sv << error; + break; + case map_outcome_e::failed: + BOOST_LOG(warning) << "Failed to map "sv << rule.protocol << ' ' << rule.external_port << ": "sv << error; + break; + case map_outcome_e::mapped: + case map_outcome_e::reused: + case map_outcome_e::unrecorded: + case map_outcome_e::cancelled: + // Open ports need no message; recording failures are logged where they happen. + break; + } + } + /** * @brief RAII helper that runs shutdown cleanup when destroyed. */ @@ -360,93 +430,24 @@ namespace upnp { return true; } - /** - * @brief Maps one port without deleting or adopting a rule belonging to another application. - * @param data Gateway's WAN connection service. - * @param urls Gateway's control URLs. - * @param lan_addr Desired destination address. - * @param mapping Desired forwarding rule. - * @return Whether the desired destination is reachable through this mapping. - */ - bool map_upnp_port(const IGDdatas &data, const urls_t &urls, const std::string &lan_addr, const mapping_t &mapping) { - owned_mapping_t wanted {urls->controlURL, data.first.servicetype, lan_addr, mapping.port.wan, mapping.port.lan, mapping.port.proto, mapping.description, false, generation}; - // A failed refresh must not forget a rule that was successfully added earlier. - for (auto &owned : owned_mappings) { - if (owned == wanted) { - owned.generation = generation; - } - } - std::array client {}; - std::array port {}; - std::array description {}; - std::array enabled {}; - std::array lease {}; - auto err = UPNP_GetSpecificPortMappingEntry(urls->controlURL, data.first.servicetype, mapping.port.wan.c_str(), mapping.port.proto.c_str(), nullptr, client.data(), port.data(), description.data(), enabled.data(), lease.data()); - switch (classify_mapping(err, client.data(), port.data(), description.data(), wanted)) { - case mapping_state_e::conflict: - BOOST_LOG(warning) << "UPnP port "sv << mapping.port.wan << " already forwards elsewhere; leaving it unchanged"sv; - return false; - case mapping_state_e::existing: - // A manual rule already does what we need. Never renew or remove it. - return true; - case mapping_state_e::unknown: - BOOST_LOG(warning) << "UPnP mapping ownership could not be checked: "sv << err; - return false; - case mapping_state_e::ours: - wanted.permanent = std::string_view {lease.data()} == "0"sv; - if (!remember_mapping(wanted)) { - return false; - } - if (wanted.permanent) { - return true; - } - break; - case mapping_state_e::missing: - break; - } - auto shutdown_event = mail::man->event(mail::shutdown); - if (shutdown_event->peek()) { - return false; - } - const auto period = std::to_string(PORT_MAPPING_LIFETIME.count()); - err = UPNP_AddPortMapping(urls->controlURL, data.first.servicetype, mapping.port.wan.c_str(), mapping.port.lan.c_str(), lan_addr.c_str(), mapping.description.c_str(), mapping.port.proto.c_str(), nullptr, period.c_str()); - if (err == 725 && !shutdown_event->peek()) { // OnlyPermanentLeasesSupported - wanted.permanent = true; - // Persist intent first: a process crash immediately after AddPortMapping must be recoverable. - if (!remember_mapping(wanted)) { - return false; - } - err = UPNP_AddPortMapping(urls->controlURL, data.first.servicetype, mapping.port.wan.c_str(), mapping.port.lan.c_str(), lan_addr.c_str(), mapping.description.c_str(), mapping.port.proto.c_str(), nullptr, "0"); - } - if (err != UPNPCOMMAND_SUCCESS) { - BOOST_LOG(warning) << "Failed to map "sv << mapping.port.proto << ' ' << mapping.port.wan << ": "sv << err; - return false; - } - return remember_mapping(std::move(wanted)); - } - /** * @brief Remove only recorded rules whose current destination, port and description still match. * @param stale_only Whether to retain rules wanted by the current discovery pass. */ void unmap_owned_ports(bool stale_only = false) { - std::erase_if(owned_mappings, [this, stale_only](const owned_mapping_t &owned) { + const miniupnpc_gateway_t gateway {}; + std::erase_if(owned_mappings, [this, stale_only, &gateway](const owned_mapping_t &owned) { if (stale_only && owned.generation == generation) { return false; } - std::array client {}; - std::array port {}; - std::array description {}; - std::array enabled {}; - std::array lease {}; - const auto lookup = UPNP_GetSpecificPortMappingEntry(owned.control_url.c_str(), owned.service.c_str(), owned.external_port.c_str(), owned.protocol.c_str(), nullptr, client.data(), port.data(), description.data(), enabled.data(), lease.data()); - const auto state = classify_mapping(lookup, client.data(), port.data(), description.data(), owned); + const auto entry = gateway.lookup(owned); + const auto state = classify_mapping(entry.result, entry.client, entry.port, entry.description, owned); if (state == mapping_state_e::unknown) { BOOST_LOG(warning) << "Could not check UPnP rule for cleanup: "sv << owned.control_url << ' ' << owned.external_port; return false; } if (state == mapping_state_e::ours) { - const auto removed = UPNP_DeletePortMapping(owned.control_url.c_str(), owned.service.c_str(), owned.external_port.c_str(), owned.protocol.c_str(), nullptr); + const auto removed = gateway.remove(owned); if (removed != UPNPCOMMAND_SUCCESS && removed != 714) { BOOST_LOG(warning) << "Failed to remove UPnP rule: "sv << removed; return false; @@ -470,15 +471,32 @@ namespace upnp { * @return Whether every requested mapping succeeded before shutdown. */ bool map_all_upnp_ports(const igd_t &igd, const std::string &addr) { - auto shutdown_event = mail::man->event(mail::shutdown); - bool complete = true; + std::vector rules; for (const auto &mapping : mappings) { - if (shutdown_event->peek()) { - return false; + owned_mapping_t rule {igd.urls->controlURL, igd.data.first.servicetype, addr, mapping.port.wan, mapping.port.lan, mapping.port.proto, mapping.description, false, generation}; + // A failed refresh must not forget a rule that was successfully added earlier. + for (auto &owned : owned_mappings) { + if (owned == rule) { + owned.generation = generation; + } } - complete = map_upnp_port(igd.data, igd.urls, addr, mapping) && complete; + rules.push_back(std::move(rule)); } - return complete; + + auto shutdown_event = mail::man->event(mail::shutdown); + const miniupnpc_gateway_t gateway {}; + return map_ports( + gateway, + rules, + std::to_string(PORT_MAPPING_LIFETIME.count()), + [this](const owned_mapping_t &rule) { + return remember_mapping(rule); + }, + [&shutdown_event]() { + return shutdown_event->peek(); + }, + report_mapping + ); } /** diff --git a/src/upnp_mapping.h b/src/upnp_mapping.h index dd6ae919b71..063e581df68 100644 --- a/src/upnp_mapping.h +++ b/src/upnp_mapping.h @@ -6,6 +6,9 @@ // standard includes #include +#include +#include +#include #include #include #include @@ -74,6 +77,145 @@ namespace upnp { return description == wanted.description ? mapping_state_e::ours : mapping_state_e::existing; } + /** + * @brief Whether a gateway reports a rule as switched off. + * @param enabled The rule's `NewEnabled` value; UPnP booleans may be `0`/`1`, `false`/`true` or `no`/`yes`. + * @return Whether the rule cannot forward traffic. A missing value is not proof that it is disabled. + */ + inline bool is_disabled_mapping(std::string_view enabled) { + return std::ranges::any_of(std::array {std::string_view {"0"}, std::string_view {"false"}, std::string_view {"no"}}, [enabled](std::string_view value) { + return std::ranges::equal(enabled, value, [](char a, char b) { + return std::tolower(static_cast(a)) == b; + }); + }); + } + + /** + * @brief A gateway rule as reported by a lookup. + */ + struct mapping_entry_t { + int result = 0; ///< MiniUPnPc lookup result. + std::string client; ///< Destination address. + std::string port; ///< Destination port. + std::string description; ///< Rule description. + std::string enabled; ///< Whether the rule is enabled. + std::string lease; ///< Remaining lease in seconds, `0` for a rule without expiry. + }; + + /** + * @brief Gateway calls used to manage rules: MiniUPnPc in Sunshine, a recorder in tests. + * @details `lookup` reports the rule on the requested gateway port, `add` adds or renews a rule with the given + * lease in seconds (`0` for no expiry), and `remove` deletes a rule. `add` and `remove` return MiniUPnPc results. + */ + template + concept mapping_gateway = requires(T &calls, const owned_mapping_t &rule, const std::string &lease) { + { calls.lookup(rule) } -> std::same_as; + { calls.add(rule, lease) } -> std::convertible_to; + { calls.remove(rule) } -> std::convertible_to; + }; + + /** + * @brief Result of one mapping attempt. + */ + enum class map_outcome_e { + mapped, ///< Sunshine added or renewed the rule. + reused, ///< An enabled rule already forwards to the destination and needs no renewal. + disabled, ///< Someone else's rule forwards to the destination but is disabled; left unchanged. + conflict, ///< The port forwards elsewhere; left unchanged. + unknown, ///< The lookup failed; nothing was changed. + unrecorded, ///< Ownership could not be recorded; nothing more was changed. + cancelled, ///< Shutdown was requested before the rule was added. + failed, ///< The gateway refused the rule. + }; + + /** + * @brief Whether an outcome leaves the port forwarded to the destination. + * @param outcome Mapping outcome. + * @return Whether the port is open. + */ + inline bool is_mapped(map_outcome_e outcome) { + return outcome == map_outcome_e::mapped || outcome == map_outcome_e::reused; + } + + /** + * @brief Map one port without deleting, renewing or adopting a rule belonging to another application. + * @param gateway Gateway holding the rule. + * @param wanted Desired rule. + * @param lifetime Lease requested for a renewable rule, in seconds. + * @param remember Records a rule Sunshine owns; must persist recovery intent before a rule without expiry is added. + * @param is_cancelled Whether shutdown was requested. + * @param error Receives the last gateway result. + * @return Mapping outcome. + */ + template + map_outcome_e map_port(Gateway &gateway, owned_mapping_t wanted, const std::string &lifetime, Remember &&remember, Cancelled &&is_cancelled, int &error) { + const auto entry = gateway.lookup(wanted); + error = entry.result; + const bool disabled = is_disabled_mapping(entry.enabled); + switch (classify_mapping(entry.result, entry.client, entry.port, entry.description, wanted)) { + case mapping_state_e::conflict: + return map_outcome_e::conflict; + case mapping_state_e::unknown: + return map_outcome_e::unknown; + case mapping_state_e::existing: + // Never renew or remove someone else's rule, but a disabled one does not open the port. + return disabled ? map_outcome_e::disabled : map_outcome_e::reused; + case mapping_state_e::ours: + wanted.permanent = entry.lease == "0"; + if (!remember(wanted)) { + return map_outcome_e::unrecorded; + } + if (wanted.permanent && !disabled) { + return map_outcome_e::reused; + } + // Renew the lease, or enable Sunshine's own disabled rule again. + break; + case mapping_state_e::missing: + break; + } + if (is_cancelled()) { + return map_outcome_e::cancelled; + } + error = gateway.add(wanted, lifetime); + if (error == 725 && !is_cancelled()) { // OnlyPermanentLeasesSupported + wanted.permanent = true; + // Persist intent first: a process crash immediately after adding the rule must be recoverable. + if (!remember(wanted)) { + return map_outcome_e::unrecorded; + } + error = gateway.add(wanted, "0"); + } + if (error != 0) { + return map_outcome_e::failed; + } + return remember(std::move(wanted)) ? map_outcome_e::mapped : map_outcome_e::unrecorded; + } + + /** + * @brief Map every requested rule on one gateway, stopping early on shutdown. + * @param gateway Gateway holding the rules. + * @param rules Desired rules. + * @param lifetime Lease requested for renewable rules, in seconds. + * @param remember Records a rule Sunshine owns. + * @param is_cancelled Whether shutdown was requested. + * @param report Receives each rule, its outcome and the last gateway result. + * @return Whether every requested port forwards to its destination. + */ + template + bool map_ports(Gateway &gateway, const std::vector &rules, const std::string &lifetime, Remember &&remember, Cancelled &&is_cancelled, Report &&report) { + bool complete = true; + for (const auto &rule : rules) { + if (is_cancelled()) { + return false; + } + int error = 0; + const auto outcome = map_port(gateway, rule, lifetime, remember, is_cancelled, error); + report(rule, outcome, error); + complete = is_mapped(outcome) && complete; + } + return complete; + } + /** * @brief Encode one append-only recovery event before adding or after removing a permanent rule. * @param mapping Rule being recorded. diff --git a/tests/unit/test_upnp_mapping.cpp b/tests/unit/test_upnp_mapping.cpp index bda7aab65b3..4a0b18e2ea5 100644 --- a/tests/unit/test_upnp_mapping.cpp +++ b/tests/unit/test_upnp_mapping.cpp @@ -6,16 +6,38 @@ // test includes #include "../tests_common.h" +// standard includes +#include + // local includes #include namespace { + /** + * @brief Construct a rule owned by Sunshine on a given gateway. + * @param control_url Gateway's SOAP endpoint. + * @param target Destination address. + * @param external_port Gateway and destination port. + * @return Rule for Sunshine's HTTP port. + */ + upnp::owned_mapping_t make_rule(const std::string &control_url, const std::string &target, const std::string &external_port = "47989") { + upnp::owned_mapping_t rule; + rule.control_url = control_url; + rule.service = "urn:schemas-upnp-org:service:WANIPConnection:1"; + rule.target = target; + rule.external_port = external_port; + rule.internal_port = external_port; + rule.protocol = "TCP"; + rule.description = "Sunshine - Client HTTP"; + return rule; + } + /** * @brief Construct a representative rule owned by Sunshine. * @return Rule on the inner gateway. */ upnp::owned_mapping_t sample_mapping() { - return {"http://192.168.1.1/control", "urn:schemas-upnp-org:service:WANIPConnection:1", "192.168.1.20", "47989", "47989", "TCP", "Sunshine - Client HTTP"}; + return make_rule("http://192.168.1.1/control", "192.168.1.20"); } TEST(UpnpMappingOwnershipTest, DistinguishesOwnedManualAndConflictingRules) { @@ -43,12 +65,9 @@ namespace { } TEST(UpnpMappingJournalTest, KeepsBothGatewaysAndOldDestinationsUntilRemoved) { - auto inner = sample_mapping(); - auto outer = inner; - outer.control_url = "http://10.0.0.1/control"; - outer.target = "10.0.0.42"; - auto changed = outer; - changed.target = "10.0.0.43"; + const auto inner = sample_mapping(); + const auto outer = make_rule("http://10.0.0.1/control", "10.0.0.42"); + const auto changed = make_rule("http://10.0.0.1/control", "10.0.0.43"); auto events = upnp::mapping_journal_event(inner, true) + upnp::mapping_journal_event(outer, true) + upnp::mapping_journal_event(changed, true); ASSERT_EQ(upnp::read_mapping_journal(events).size(), 3); events += upnp::mapping_journal_event(outer, false); @@ -93,9 +112,8 @@ namespace { } TEST(UpnpMappingJournalTest, PreservesQuotedDescriptionsAndEndpointCharacters) { - auto mapping = sample_mapping(); + auto mapping = make_rule("http://192.168.1.1/control?value=\\quoted", "192.168.1.20"); mapping.description = "Sunshine - \"Client HTTP\""; - mapping.control_url = "http://192.168.1.1/control?value=\\quoted"; const auto recovered = upnp::read_mapping_journal(upnp::mapping_journal_event(mapping, true)); ASSERT_EQ(recovered.size(), 1); EXPECT_EQ(recovered.front(), mapping); @@ -109,4 +127,159 @@ namespace { mapping.description += '\r'; EXPECT_TRUE(upnp::mapping_journal_event(mapping, true).empty()); } + + /** + * @brief Gateway that answers lookups from a table and records every change. + */ + struct recording_gateway_t { + std::map> entries; ///< Lookup answers by gateway port; other ports report 714. + std::vector add_results; ///< Results of successive adds; 0 once exhausted. + std::vector added_leases; ///< Lease requested by every add. + int removes = 0; ///< Number of removals. + + /** + * @brief Answer a lookup from the table. + * @param rule Requested rule. + * @return Configured answer, or 714 (no such mapping). + */ + upnp::mapping_entry_t lookup(const upnp::owned_mapping_t &rule) const { + const auto found = entries.find(rule.external_port); + return found == entries.end() ? upnp::mapping_entry_t {714} : found->second; + } + + /** + * @brief Record an add. + * @param lease Requested lease. + * @return Next configured result. + */ + int add(const upnp::owned_mapping_t &, const std::string &lease) { + const auto index = added_leases.size(); + added_leases.push_back(lease); + return index < add_results.size() ? add_results[index] : 0; + } + + /** + * @brief Record a removal. + * @return Success. + */ + int remove(const upnp::owned_mapping_t &) { + ++removes; + return 0; + } + }; + + static_assert(upnp::mapping_gateway); + + /** + * @brief Construct a lookup answer that forwards to the rule's destination. + * @param rule Requested rule. + * @param description Description reported by the gateway. + * @param enabled Reported `NewEnabled` value. + * @param lease Reported remaining lease. + * @return Lookup answer. + */ + upnp::mapping_entry_t entry_for(const upnp::owned_mapping_t &rule, const std::string &description, const std::string &enabled, const std::string &lease) { + return {0, rule.target, rule.internal_port, description, enabled, lease}; + } + + /** + * @brief One mapping pass through the production mapping path, with its records. + */ + struct mapping_pass_t { + recording_gateway_t gateway; ///< Gateway used by the pass. + std::vector remembered; ///< Rules recorded as owned by Sunshine. + std::vector outcomes; ///< Outcome reported for each rule. + + /** + * @brief Map the rules. + * @param rules Requested rules. + * @return Whether the pass completed. + */ + bool run(const std::vector &rules) { + return upnp::map_ports( + gateway, + rules, + "3600", + [this](const upnp::owned_mapping_t &rule) { + remembered.push_back(rule); + return true; + }, + [] { + return false; + }, + [this](const upnp::owned_mapping_t &, upnp::map_outcome_e outcome, int) { + outcomes.push_back(outcome); + } + ); + } + }; + + TEST(UpnpMapPortTest, DisabledManualRuleIsLeftUnchangedAndLeavesThePassIncomplete) { + const auto rule = sample_mapping(); + mapping_pass_t pass; + pass.gateway.entries[rule.external_port] = entry_for(rule, "Manual forwarding", "0", "3600"); + EXPECT_FALSE(pass.run({rule})); + EXPECT_EQ(pass.outcomes, std::vector {upnp::map_outcome_e::disabled}); + EXPECT_TRUE(pass.gateway.added_leases.empty()); + EXPECT_EQ(pass.gateway.removes, 0); + EXPECT_TRUE(pass.remembered.empty()); + } + + TEST(UpnpMapPortTest, EnabledManualRuleIsReusedWithoutRenewal) { + const auto rule = sample_mapping(); + mapping_pass_t pass; + pass.gateway.entries[rule.external_port] = entry_for(rule, "Manual forwarding", "1", "3600"); + EXPECT_TRUE(pass.run({rule})); + EXPECT_EQ(pass.outcomes, std::vector {upnp::map_outcome_e::reused}); + EXPECT_TRUE(pass.gateway.added_leases.empty()); + EXPECT_EQ(pass.gateway.removes, 0); + EXPECT_TRUE(pass.remembered.empty()); + } + + TEST(UpnpMapPortTest, DisabledManualRuleDoesNotStopTheOtherPorts) { + const auto manual = sample_mapping(); + const auto other = make_rule("http://192.168.1.1/control", "192.168.1.20", "47984"); + mapping_pass_t pass; + pass.gateway.entries[manual.external_port] = entry_for(manual, "Manual forwarding", "0", "3600"); + EXPECT_FALSE(pass.run({manual, other})); + EXPECT_EQ(pass.outcomes, (std::vector {upnp::map_outcome_e::disabled, upnp::map_outcome_e::mapped})); + EXPECT_EQ(pass.gateway.added_leases, std::vector {"3600"}); + ASSERT_EQ(pass.remembered.size(), 1); + EXPECT_EQ(pass.remembered.front(), other); + } + + TEST(UpnpMapPortTest, DisabledOwnedPermanentRuleIsEnabledAgain) { + const auto rule = sample_mapping(); + mapping_pass_t pass; + pass.gateway.entries[rule.external_port] = entry_for(rule, rule.description, "0", "0"); + pass.gateway.add_results = {725, 0}; + EXPECT_TRUE(pass.run({rule})); + EXPECT_EQ(pass.outcomes, std::vector {upnp::map_outcome_e::mapped}); + EXPECT_EQ(pass.gateway.added_leases, (std::vector {"3600", "0"})); + EXPECT_EQ(pass.gateway.removes, 0); + ASSERT_FALSE(pass.remembered.empty()); + EXPECT_TRUE(pass.remembered.front().permanent); + EXPECT_TRUE(pass.remembered.back().permanent); + } + + TEST(UpnpMapPortTest, EnabledOwnedPermanentRuleIsKeptWithoutAdding) { + const auto rule = sample_mapping(); + mapping_pass_t pass; + pass.gateway.entries[rule.external_port] = entry_for(rule, rule.description, "1", "0"); + EXPECT_TRUE(pass.run({rule})); + EXPECT_EQ(pass.outcomes, std::vector {upnp::map_outcome_e::reused}); + EXPECT_TRUE(pass.gateway.added_leases.empty()); + EXPECT_EQ(pass.gateway.removes, 0); + ASSERT_EQ(pass.remembered.size(), 1); + EXPECT_TRUE(pass.remembered.front().permanent); + } + + TEST(UpnpMapPortTest, RecognizesUpnpBooleanSpellings) { + for (const auto *value : {"0", "false", "False", "NO"}) { + EXPECT_TRUE(upnp::is_disabled_mapping(value)) << value; + } + for (const auto *value : {"1", "true", "YES", ""}) { + EXPECT_FALSE(upnp::is_disabled_mapping(value)) << value; + } + } } // namespace From 3cf711da58ff5ec6bfb75356b9419b5887a08269 Mon Sep 17 00:00:00 2001 From: Enrique Wood Date: Sat, 14 Nov 2026 13:36:30 -0300 Subject: [PATCH 6/6] fix(upnp): accept gateways that report internal port 0 Signed-off-by: Enrique Wood --- src/upnp_mapping.h | 4 +++- tests/unit/test_upnp_mapping.cpp | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/upnp_mapping.h b/src/upnp_mapping.h index 063e581df68..b075c306319 100644 --- a/src/upnp_mapping.h +++ b/src/upnp_mapping.h @@ -71,7 +71,9 @@ namespace upnp { if (result != 0) { return mapping_state_e::unknown; } - if (client != wanted.target || port != wanted.internal_port) { + // Some gateways report the internal port of an existing rule as 0 or leave it empty; that is not another destination. + const bool port_reported = !port.empty() && port != "0"; + if (client != wanted.target || (port_reported && port != wanted.internal_port)) { return mapping_state_e::conflict; } return description == wanted.description ? mapping_state_e::ours : mapping_state_e::existing; diff --git a/tests/unit/test_upnp_mapping.cpp b/tests/unit/test_upnp_mapping.cpp index 4a0b18e2ea5..c82d9f2bbb5 100644 --- a/tests/unit/test_upnp_mapping.cpp +++ b/tests/unit/test_upnp_mapping.cpp @@ -48,6 +48,15 @@ namespace { EXPECT_EQ(upnp::classify_mapping(0, mapping.target, "48000", mapping.description, mapping), upnp::mapping_state_e::conflict); } + TEST(UpnpMappingOwnershipTest, UnreportedInternalPortIsNotAnotherDestination) { + const auto mapping = sample_mapping(); + for (const auto *port : {"0", ""}) { + EXPECT_EQ(upnp::classify_mapping(0, mapping.target, port, mapping.description, mapping), upnp::mapping_state_e::ours) << port; + EXPECT_EQ(upnp::classify_mapping(0, mapping.target, port, "Manual forwarding", mapping), upnp::mapping_state_e::existing) << port; + EXPECT_EQ(upnp::classify_mapping(0, "192.168.1.99", port, mapping.description, mapping), upnp::mapping_state_e::conflict) << port; + } + } + TEST(UpnpMappingOwnershipTest, LookupErrorsAreNeverProofOfOwnershipOrAbsence) { const auto mapping = sample_mapping(); EXPECT_EQ(upnp::classify_mapping(714, "", "", "", mapping), upnp::mapping_state_e::missing);