move upnp mapper to have one per listen_socket_t. simplify the upnp logic to only deal with a single network.

include/libtorrent/aux_/session_impl.hpp

@@ -235,6 +235,7 @@ namespace aux {
 		aux::handler_storage<TORRENT_READ_HANDLER_MAX_SIZE> udp_handler_storage;
 
 		std::shared_ptr<natpmp> natpmp_mapper;
+		std::shared_ptr<upnp> upnp_mapper;
 
 		// set to true when we receive an incoming connection from this listen
 		// socket
@@ -693,7 +694,7 @@ namespace aux {
 			void start_ip_notifier();
 			void start_lsd();
 			void start_natpmp();
-			upnp* start_upnp();
+			void start_upnp();
 
 			void stop_ip_notifier();
 			void stop_lsd();
@@ -835,6 +836,7 @@ namespace aux {
 			void on_lsd_peer(tcp::endpoint const& peer, sha1_hash const& ih) override;
 
 			void start_natpmp(aux::listen_socket_t& s);
+			void start_upnp(aux::listen_socket_t& s);
 
 			void set_external_address(std::shared_ptr<listen_socket_t> const& sock, address const& ip
 				, ip_source_t source_type, address const& source);
@@ -1193,7 +1195,6 @@ namespace aux {
 			// this is deducted from the connect speed
 			int m_boost_connections = 0;
 
-			std::shared_ptr<upnp> m_upnp;
 			std::shared_ptr<lsd> m_lsd;
 
 #if TORRENT_ABI_VERSION == 1

include/libtorrent/upnp.hpp

@@ -35,7 +35,6 @@ POSSIBILITY OF SUCH DAMAGE.
 
 #include "libtorrent/socket.hpp"
 #include "libtorrent/error_code.hpp"
-#include "libtorrent/broadcast_socket.hpp"
 #include "libtorrent/deadline_timer.hpp"
 #include "libtorrent/enum_net.hpp"
 #include "libtorrent/resolver.hpp"
@@ -149,7 +148,10 @@ struct TORRENT_EXTRA_EXPORT upnp final
 {
 	upnp(io_service& ios
 		, std::string const& user_agent
-		, aux::portmap_callback& cb);
+		, aux::portmap_callback& cb
+		, address_v4 const& listen_address
+		, address_v4 const& netmask
+		, std::string listen_device);
 	~upnp();
 
 	void set_user_agent(std::string const& v) { m_user_agent = v; }
@@ -196,12 +198,15 @@ private:
 
 	std::shared_ptr<upnp> self() { return shared_from_this(); }
 
+	void open_multicast_socket(udp::socket& s, error_code& ec);
+	void open_unicast_socket(udp::socket& s, error_code& ec);
+
 	void map_timer(error_code const& ec);
 	void try_map_upnp();
 	void discover_device_impl();
 
 	void resend_request(error_code const& e);
-	void on_reply(udp::endpoint const& from, span<char const> buffer);
+	void on_reply(udp::socket& s, error_code const& ec);
 
 	struct rootdevice;
 	void next(rootdevice& d, port_mapping_t i);
@@ -329,7 +334,8 @@ private:
 
 	// the udp socket used to send and receive
 	// multicast messages on the network
-	broadcast_socket m_socket;
+	udp::socket m_multicast_socket;
+	udp::socket m_unicast_socket;
 
 	// used to resend udp packets in case
 	// they time out
@@ -350,9 +356,11 @@ private:
 
 	std::string m_model;
 
-	// cache of interfaces
+	// the network this UPnP mapper is associated with. Don't talk to any other
-	mutable std::vector<ip_interface> m_interfaces;
+	// network
-	mutable time_point m_last_if_update;
+	address_v4 m_listen_address;
+	address_v4 m_netmask;
+	std::string m_device;
 };
 
 }

src/session_impl.cpp

@@ -1985,6 +1985,12 @@ namespace aux {
 				start_natpmp(*s);
 		}
 
+		if (m_settings.get_bool(settings_pack::enable_upnp))
+		{
+			for (auto const& s : new_sockets)
+				start_upnp(*s);
+		}
+
 		if (map_ports)
 		{
 			for (auto const& s : m_listen_sockets)
@@ -2041,11 +2047,11 @@ namespace aux {
 			map_port(*s.natpmp_mapper, portmap_protocol::udp, make_tcp(udp_ep)
 				, s.udp_port_mapping[portmap_transport::natpmp].mapping);
 		}
-		if ((mask & remap_upnp) && m_upnp)
+		if ((mask & remap_upnp) && s.upnp_mapper)
 		{
-			map_port(*m_upnp, portmap_protocol::tcp, tcp_ep
+			map_port(*s.upnp_mapper, portmap_protocol::tcp, tcp_ep
 				, s.tcp_port_mapping[portmap_transport::upnp].mapping);
-			map_port(*m_upnp, portmap_protocol::udp, make_tcp(udp_ep)
+			map_port(*s.upnp_mapper, portmap_protocol::udp, make_tcp(udp_ep)
 				, s.udp_port_mapping[portmap_transport::upnp].mapping);
 		}
 	}
@@ -6421,12 +6427,19 @@ namespace aux {
 	{
 		if (!m_settings.get_bool(settings_pack::anonymous_mode))
 		{
-			if (m_upnp)
+			for (auto& s : m_listen_sockets)
-				m_upnp->set_user_agent(m_settings.get_str(settings_pack::user_agent));
+			{
+				if (!s->upnp_mapper) continue;
+				s->upnp_mapper->set_user_agent(m_settings.get_str(settings_pack::user_agent));
+			}
 			return;
 		}
 
-		if (m_upnp) m_upnp->set_user_agent("");
+		for (auto& s : m_listen_sockets)
+		{
+			if (!s->upnp_mapper) continue;
+			s->upnp_mapper->set_user_agent("");
+		}
 	}
 
 #if TORRENT_ABI_VERSION == 1
@@ -6644,24 +6657,39 @@ namespace aux {
 		}
 	}
 
-	upnp* session_impl::start_upnp()
+	void session_impl::start_upnp()
 	{
 		INVARIANT_CHECK;
-
-		if (m_upnp) return m_upnp.get();
-
-		// the upnp constructor may fail and call the callbacks
-		m_upnp = std::make_shared<upnp>(m_io_service
-			, m_settings.get_bool(settings_pack::anonymous_mode)
-				? "" : m_settings.get_str(settings_pack::user_agent)
-			, *this);
-		m_upnp->start();
-
 		for (auto& s : m_listen_sockets)
 		{
+			start_upnp(*s);
 			remap_ports(remap_upnp, *s);
 		}
-		return m_upnp.get();
+	}
+
+	void session_impl::start_upnp(aux::listen_socket_t& s)
+	{
+		// until we support SSDP over an IPv6 network (
+		// https://en.wikipedia.org/wiki/Simple_Service_Discovery_Protocol )
+		// there's no point in starting upnp on one.
+		if (is_v6(s.local_endpoint))
+			return;
+
+		// there's no point in starting the UPnP mapper for a network that doesn't
+		// have a gateway. The whole point is to forward ports through the gateway
+		if (!(s.flags & listen_socket_t::has_gateway))
+			return;
+
+		if (!s.upnp_mapper)
+		{
+			// the upnp constructor may fail and call the callbacks
+			// into the session_impl.
+			s.upnp_mapper = std::make_shared<upnp>(m_io_service
+				, m_settings.get_bool(settings_pack::anonymous_mode)
+				? "" : m_settings.get_str(settings_pack::user_agent)
+				, *this, s.local_endpoint.address().to_v4(), s.netmask.to_v4(), s.device);
+			s.upnp_mapper->start();
+		}
 	}
 
 	std::vector<port_mapping_t> session_impl::add_port_mapping(portmap_protocol const t
@@ -6669,10 +6697,10 @@ namespace aux {
 		, int const local_port)
 	{
 		std::vector<port_mapping_t> ret;
-		if (m_upnp) ret.push_back(m_upnp->add_mapping(t, external_port
-			, tcp::endpoint({}, static_cast<std::uint16_t>(local_port))));
 		for (auto& s : m_listen_sockets)
 		{
+			if (s->upnp_mapper) ret.push_back(s->upnp_mapper->add_mapping(t, external_port
+				, tcp::endpoint({}, static_cast<std::uint16_t>(local_port))));
 			if (s->natpmp_mapper) ret.push_back(s->natpmp_mapper->add_mapping(t, external_port
 				, tcp::endpoint({}, static_cast<std::uint16_t>(local_port))));
 		}
@@ -6681,9 +6709,9 @@ namespace aux {
 
 	void session_impl::delete_port_mapping(port_mapping_t handle)
 	{
-		if (m_upnp) m_upnp->delete_mapping(handle);
 		for (auto& s : m_listen_sockets)
 		{
+			if (s->upnp_mapper) s->upnp_mapper->delete_mapping(handle);
 			if (s->natpmp_mapper) s->natpmp_mapper->delete_mapping(handle);
 		}
 	}
@@ -6717,15 +6745,14 @@ namespace aux {
 
 	void session_impl::stop_upnp()
 	{
-		if (!m_upnp) return;
-
-		m_upnp->close();
 		for (auto& s : m_listen_sockets)
 		{
+			if (!s->upnp_mapper) continue;
 			s->tcp_port_mapping[portmap_transport::upnp] = listen_port_mapping();
 			s->udp_port_mapping[portmap_transport::upnp] = listen_port_mapping();
+			s->upnp_mapper->close();
+			s->upnp_mapper.reset();
 		}
-		m_upnp.reset();
 	}
 
 	external_ip session_impl::external_address() const

src/upnp.cpp

@@ -30,13 +30,13 @@ POSSIBILITY OF SUCH DAMAGE.
 
 */
 
+#include "libtorrent/config.hpp"
 #include "libtorrent/socket.hpp"
 #include "libtorrent/socket_io.hpp"
 #include "libtorrent/upnp.hpp"
 #include "libtorrent/io.hpp"
 #include "libtorrent/parse_url.hpp"
 #include "libtorrent/xml_parse.hpp"
-#include "libtorrent/enum_net.hpp"
 #include "libtorrent/random.hpp"
 #include "libtorrent/aux_/time.hpp" // for aux::time_now()
 #include "libtorrent/aux_/escape_string.hpp" // for convert_from_native
@@ -96,17 +96,22 @@ upnp::rootdevice& upnp::rootdevice::operator=(rootdevice&&) = default;
 // interface by default
 upnp::upnp(io_service& ios
 	, std::string const& user_agent
-	, aux::portmap_callback& cb)
+	, aux::portmap_callback& cb
+	, address_v4 const& listen_address
+	, address_v4 const& netmask
+	, std::string listen_device)
 	: m_user_agent(user_agent)
 	, m_callback(cb)
 	, m_io_service(ios)
 	, m_resolver(ios)
-	, m_socket(udp::endpoint(make_address_v4("239.255.255.250"
+	, m_multicast_socket(ios)
-		, ignore_error), 1900))
+	, m_unicast_socket(ios)
 	, m_broadcast_timer(ios)
 	, m_refresh_timer(ios)
 	, m_map_timer(ios)
-	, m_last_if_update(min_time())
+	, m_listen_address(listen_address)
+	, m_netmask(netmask)
+	, m_device(std::move(listen_device))
 {
 }
 
@@ -115,14 +120,74 @@ void upnp::start()
 	TORRENT_ASSERT(is_single_thread());
 
 	error_code ec;
-	m_socket.open(std::bind(&upnp::on_reply, self(), _1, _2)
+	open_multicast_socket(m_multicast_socket, ec);
-		, lt::get_io_service(m_refresh_timer), ec);
+#ifndef TORRENT_DISABLE_LOGGING
+	if (ec && should_log())
+	{
+		log("failed to open multicast socket: \"%s\""
+			, convert_from_native(ec.message()).c_str());
+		m_disabled = true;
+		return;
+	}
+#endif
 
-	m_mappings.reserve(10);
+	open_unicast_socket(m_unicast_socket, ec);
+#ifndef TORRENT_DISABLE_LOGGING
+	if (ec && should_log())
+	{
+		log("failed to open unicast socket: \"%s\""
+			, convert_from_native(ec.message()).c_str());
+		m_disabled = true;
+		return;
+	}
+#endif
+
+	m_mappings.reserve(2);
 
 	discover_device_impl();
 }
 
+namespace {
+	address_v4 const ssdp_multicast_addr = make_address_v4("239.255.255.250");
+	int const ssdp_port = 1900;
+
+}
+
+void upnp::open_multicast_socket(udp::socket& s, error_code& ec)
+{
+	using namespace boost::asio::ip::multicast;
+	s.open(udp::v4(), ec);
+	if (ec) return;
+	s.set_option(udp::socket::reuse_address(true), ec);
+	if (ec) return;
+	s.bind(udp::endpoint(m_listen_address, ssdp_port), ec);
+	if (ec) return;
+	s.set_option(join_group(ssdp_multicast_addr), ec);
+	if (ec) return;
+	s.set_option(hops(255), ec);
+	if (ec) return;
+	s.set_option(enable_loopback(true), ec);
+	if (ec) return;
+	s.set_option(outbound_interface(m_listen_address), ec);
+	if (ec) return;
+
+	ADD_OUTSTANDING_ASYNC("upnp::on_reply");
+	s.async_receive(boost::asio::null_buffers{}
+		, std::bind(&upnp::on_reply, self(), std::ref(s), _1));
+}
+
+void upnp::open_unicast_socket(udp::socket& s, error_code& ec)
+{
+	s.open(udp::v4(), ec);
+	if (ec) return;
+	s.bind(udp::endpoint(m_listen_address, 0), ec);
+	if (ec) return;
+
+	ADD_OUTSTANDING_ASYNC("upnp::on_reply");
+	s.async_receive(boost::asio::null_buffers{}
+		, std::bind(&upnp::on_reply, self(), std::ref(s), _1));
+}
+
 upnp::~upnp() = default;
 
 #ifndef TORRENT_DISABLE_LOGGING
@@ -161,18 +226,25 @@ void upnp::discover_device_impl()
 	// simulate packet loss
 	if (m_retry_count & 1)
 #endif
-	m_socket.send(msearch, sizeof(msearch) - 1, ec);
 
-	if (ec)
+	error_code mcast_ec;
+	error_code ucast_ec;
+	m_multicast_socket.send_to(boost::asio::buffer(msearch, sizeof(msearch) - 1)
+		, udp::endpoint(ssdp_multicast_addr, ssdp_port), 0, mcast_ec);
+	m_unicast_socket.send_to(boost::asio::buffer(msearch, sizeof(msearch) - 1)
+		, udp::endpoint(ssdp_multicast_addr, ssdp_port), 0, ucast_ec);
+
+	if (mcast_ec && ucast_ec)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
 		if (should_log())
 		{
-			log("broadcast failed: %s. Aborting."
+			log("multicast send failed: \"%s\" and \"%s\". Aborting."
-				, convert_from_native(ec.message()).c_str());
+				, convert_from_native(mcast_ec.message()).c_str()
+				, convert_from_native(ucast_ec.message()).c_str());
 		}
 #endif
-		disable(ec);
+		disable(mcast_ec);
 		return;
 	}
 
@@ -360,11 +432,22 @@ void upnp::connect(rootdevice& d)
 	}
 }
 
-void upnp::on_reply(udp::endpoint const& from, span<char const> buffer)
+void upnp::on_reply(udp::socket& s, error_code const& ec)
 {
 	TORRENT_ASSERT(is_single_thread());
+	COMPLETE_ASYNC("upnp::on_reply");
+
+	if (ec == boost::asio::error::operation_aborted) return;
+	if (m_closing) return;
+
 	std::shared_ptr<upnp> me(self());
 
+	std::array<char, 1500> buffer{};
+	udp::endpoint from;
+	error_code err;
+	int const len = static_cast<int>(s.receive_from(boost::asio::buffer(buffer)
+		, from, 0, err));
+
 	// parse out the url for the device
 
 /*
@@ -391,37 +474,22 @@ void upnp::on_reply(udp::endpoint const& from, span<char const> buffer)
 	Server:Microsoft-Windows-NT/5.1 UPnP/1.0 UPnP-Device-Host/1.0
 
 */
-	error_code ec;
-	if (clock_type::now() - seconds(60) > m_last_if_update)
-	{
-		m_interfaces = enum_net_interfaces(m_io_service, ec);
-#ifndef TORRENT_DISABLE_LOGGING
-		if (ec && should_log())
-		{
-			log("when receiving response from: %s: %s"
-				, print_endpoint(from).c_str(), convert_from_native(ec.message()).c_str());
-		}
-#endif
-		m_last_if_update = aux::time_now();
-	}
 
-	if (!ec && !in_local_network(m_interfaces, from.address()))
+	ADD_OUTSTANDING_ASYNC("upnp::on_reply");
+	s.async_receive(boost::asio::null_buffers{}
+		, std::bind(&upnp::on_reply, self(), std::ref(s), _1));
+
+	if (err) return;
+
+	if (!match_addr_mask(m_listen_address, from.address(), m_netmask))
 	{
 #ifndef TORRENT_DISABLE_LOGGING
 		if (should_log())
 		{
-			char msg[400];
+			log("ignoring response from: %s. IP is not on local network. (addr: %s mask: %s)"
-			int num_chars = std::snprintf(msg, sizeof(msg)
+				, print_endpoint(from).c_str()
-				, "ignoring response from: %s. IP is not on local network. "
+				, m_listen_address.to_string().c_str()
-				, print_endpoint(from).c_str());
+				, m_netmask.to_string().c_str());
-
-			for (auto const& iface : m_interfaces)
-			{
-				num_chars += std::snprintf(msg + num_chars, sizeof(msg) - std::size_t(num_chars), "(%s,%s) "
-					, print_address(iface.interface_address).c_str(), print_address(iface.netmask).c_str());
-				if (num_chars >= int(sizeof(msg))) break;
-			}
-			log("%s", msg);
 		}
 #endif
 		return;
@@ -429,7 +497,7 @@ void upnp::on_reply(udp::endpoint const& from, span<char const> buffer)
 
 	http_parser p;
 	bool error = false;
-	p.incoming(buffer, error);
+	p.incoming({buffer.data(), len}, error);
 	if (error)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
@@ -498,16 +566,16 @@ void upnp::on_reply(udp::endpoint const& from, span<char const> buffer)
 		std::string auth;
 		// we don't have this device in our list. Add it
 		std::tie(protocol, auth, d.hostname, d.port, d.path)
-			= parse_url_components(d.url, ec);
+			= parse_url_components(d.url, err);
 		if (d.port == -1) d.port = protocol == "http" ? 80 : 443;
 
-		if (ec)
+		if (err)
 		{
 #ifndef TORRENT_DISABLE_LOGGING
 			if (should_log())
 			{
 				log("invalid URL %s from %s: %s"
-					, d.url.c_str(), print_endpoint(from).c_str(), convert_from_native(ec.message()).c_str());
+					, d.url.c_str(), print_endpoint(from).c_str(), convert_from_native(err.message()).c_str());
 			}
 #endif
 			return;
@@ -579,7 +647,7 @@ void upnp::on_reply(udp::endpoint const& from, span<char const> buffer)
 	// check back in a little bit to see if we have seen any
 	// devices at one of our default routes. If not, we want to override
 	// ignoring them and use them instead (better than not working).
-	m_map_timer.expires_from_now(seconds(1), ec);
+	m_map_timer.expires_from_now(seconds(1), err);
 	ADD_OUTSTANDING_ASYNC("upnp::map_timer");
 	m_map_timer.async_wait(std::bind(&upnp::map_timer, self(), _1));
 }
@@ -1026,7 +1094,8 @@ void upnp::disable(error_code const& ec)
 	m_broadcast_timer.cancel(e);
 	m_refresh_timer.cancel(e);
 	m_map_timer.cancel(e);
-	m_socket.close();
+	m_unicast_socket.close(e);
+	m_multicast_socket.close(e);
 }
 
 void find_error_code(int const type, string_view string, error_code_parse_state& state)
@@ -1522,7 +1591,8 @@ void upnp::close()
 	m_broadcast_timer.cancel(ec);
 	m_map_timer.cancel(ec);
 	m_closing = true;
-	m_socket.close();
+	m_unicast_socket.close(ec);
+	m_multicast_socket.close(ec);
 
 	for (auto& dev : m_devices)
 	{

test/test_session.cpp

@@ -548,12 +548,14 @@ TORRENT_TEST(reopen_network_sockets)
 
 	lt::session s(p);
 
-	// NAT-PMP will be disabled when we only listen on loopback
+	// NAT-PMP nad UPnP will be disabled when we only listen on loopback
-	TEST_CHECK(count_alerts(s, 2, 2));
+	TEST_CHECK(count_alerts(s, 2, 0));
 
+	// this is a bit of a pointless test now, since neither UPnP nor NAT-PMP are
+	// enabled for loopback
 	s.reopen_network_sockets(session_handle::reopen_map_ports);
 
-	TEST_CHECK(count_alerts(s, 0, 2));
+	TEST_CHECK(count_alerts(s, 0, 0));
 
 	s.reopen_network_sockets({});
 

test/test_upnp.cpp

@@ -125,8 +125,6 @@ struct callback_info
 
 std::list<callback_info> callbacks;
 
-namespace // TODO: remove this nested namespace
-{
 	struct upnp_callback final : aux::portmap_callback
 	{
 		void on_port_mapping(port_mapping_t const mapping
@@ -154,12 +152,50 @@ namespace // TODO: remove this nested namespace
 		}
 	#endif
 	};
+
+ip_interface pick_upnp_interface()
+{
+	lt::io_service ios;
+	error_code ec;
+	std::vector<ip_route> const routes = enum_routes(ios, ec);
+	if (ec)
+	{
+		std::cerr << "failed to enumerate routes: " << ec.message() << '\n';
+		TEST_CHECK(false);
+		return {};
+	}
+	std::vector<ip_interface> const ifs = enum_net_interfaces(ios, ec);
+	if (ec)
+	{
+		std::cerr << "failed to enumerate network interfaces: " << ec.message() << '\n';
+		TEST_CHECK(false);
+		return {};
+	}
+	int idx = 0;
+	auto const iface = std::find_if(ifs.begin(), ifs.end(), [&](ip_interface const& face)
+		{
+			std::cerr << " - " << idx << ' ' << face.interface_address.to_string() << ' ' << face.name << '\n';
+			++idx;
+			if (!face.interface_address.is_v4()) return false;
+			if (is_loopback(face.interface_address)) return false;
+			auto const route = std::find_if(routes.begin(), routes.end(), [&](ip_route const& r)
+				{ return r.destination.is_unspecified() && string_view(face.name) == r.name; });
+			if (route == routes.end()) return false;
+			return true;
+		});
+
+	if (iface == ifs.end())
+	{
+		std::cerr << "could not find an IPv4 interface to run UPnP test over!\n";
+		TEST_CHECK(false);
+		return {};
+	}
+	std::cout << "starting upnp on: " << iface->interface_address.to_string() << ' ' << iface->name << '\n';
+	return *iface;
 }
 
 void run_upnp_test(char const* root_filename, char const* control_name, int igd_version)
 {
-	lt::io_service ios;
-
 	g_port = start_web_server();
 
 	std::vector<char> buf;
@@ -191,12 +227,18 @@ void run_upnp_test(char const* root_filename, char const* control_name, int igd_
 	sock = new broadcast_socket(udp::endpoint(address_v4::from_string("239.255.255.250")
 		, 1900));
 
+	lt::io_service ios;
+
 	sock->open(&incoming_msearch, ios, ec);
 
 	std::string user_agent = "test agent";
 
+	// pick an appropriate interface to run this test on
+	auto const ipf = pick_upnp_interface();
+
 	upnp_callback cb;
-	auto upnp_handler = std::make_shared<upnp>(ios, user_agent, cb);
+	auto upnp_handler = std::make_shared<upnp>(ios, user_agent, cb
+		, ipf.interface_address.to_v4(), ipf.netmask.to_v4(), ipf.name);
 	upnp_handler->start();
 
 	for (int i = 0; i < 20; ++i)
@@ -280,9 +322,14 @@ TORRENT_TEST(upnp)
 
 TORRENT_TEST(upnp_max_mappings)
 {
+	// pick an appropriate interface to run this test on
 	lt::io_service ios;
+
+	auto const ipf = pick_upnp_interface();
+
 	upnp_callback cb;
-	auto upnp_handler = std::make_shared<upnp>(ios, "test agent", cb);
+	auto upnp_handler = std::make_shared<upnp>(ios, "", cb
+		, ipf.interface_address.to_v4(), ipf.netmask.to_v4(), ipf.name);
 
 	for (int i = 0; i < 50; ++i)
 	{