diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h @@ -27,7 +27,6 @@ class GDBRemoteCommunicationServer : public GDBRemoteCommunication { public: - using PortMap = std::map; using PacketHandler = std::function; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h @@ -16,13 +16,58 @@ #include "GDBRemoteCommunicationServerCommon.h" #include "lldb/Host/Socket.h" +#include "llvm/Support/Error.h" + namespace lldb_private { namespace process_gdb_remote { class GDBRemoteCommunicationServerPlatform : public GDBRemoteCommunicationServerCommon { public: - typedef std::map PortMap; + class PortMap { + public: + // This class is used to restrict the range of ports that + // platform created debugserver/gdbserver processes will + // communicate on. + PortMap() = default; + + // Make a port map with a range of free ports + // from min_port to max_port-1. + PortMap(uint16_t min_port, uint16_t max_port); + + // Add a port to the map. If it is already in the map do not modify + // its mapping. (used ports remain used, new ports start as free) + void AllowPort(uint16_t port); + + // If we are using a port map where we can only use certain ports, + // get the next available port. + // + // If we are using a port map and we are out of ports, return an error. + // + // If we aren't using a port map, return 0 to indicate we should bind to + // port 0 and then figure out which port we used. + llvm::Expected GetNextAvailablePort(); + + // Tie a port to a process ID. Returns false if the port is not in the port + // map. If the port is already in use it will be moved to the given pid. + // FIXME: This is and GetNextAvailablePort make create a race condition if + // the portmap is shared between processes. + bool AssociatePortWithProcess(uint16_t port, lldb::pid_t pid); + + // Free the given port. Returns false if the port is not in the map. + bool FreePort(uint16_t port); + + // Free the port associated with the given pid. Returns false if there is + // no port associated with the pid. + bool FreePortForProcess(lldb::pid_t pid); + + // Returns true if there are no ports in the map, regardless of the state + // of those ports. Meaning a map with 1 used port is not empty. + bool empty() const; + + private: + std::map m_port_map; + }; GDBRemoteCommunicationServerPlatform( const Socket::SocketProtocol socket_protocol, const char *socket_scheme); @@ -35,21 +80,6 @@ // a port chosen by the OS. void SetPortMap(PortMap &&port_map); - // If we are using a port map where we can only use certain ports, - // get the next available port. - // - // If we are using a port map and we are out of ports, return UINT16_MAX - // - // If we aren't using a port map, return 0 to indicate we should bind to - // port 0 and then figure out which port we used. - uint16_t GetNextAvailablePort(); - - bool AssociatePortWithProcess(uint16_t port, lldb::pid_t pid); - - bool FreePort(uint16_t port); - - bool FreePortForProcess(lldb::pid_t pid); - void SetPortOffset(uint16_t port_offset); void SetInferiorArguments(const lldb_private::Args &args); diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp @@ -42,6 +42,69 @@ using namespace lldb_private::process_gdb_remote; using namespace lldb_private; +GDBRemoteCommunicationServerPlatform::PortMap::PortMap(uint16_t min_port, + uint16_t max_port) { + for (; min_port < max_port; ++min_port) + m_port_map[min_port] = LLDB_INVALID_PROCESS_ID; +} + +void GDBRemoteCommunicationServerPlatform::PortMap::AllowPort(uint16_t port) { + // Do not modify existing mappings + m_port_map.insert({port, LLDB_INVALID_PROCESS_ID}); +} + +llvm::Expected +GDBRemoteCommunicationServerPlatform::PortMap::GetNextAvailablePort() { + if (m_port_map.empty()) + return 0; // Bind to port zero and get a port, we didn't have any + // limitations + + for (auto &pair : m_port_map) { + if (pair.second == LLDB_INVALID_PROCESS_ID) { + pair.second = ~(lldb::pid_t)LLDB_INVALID_PROCESS_ID; + return pair.first; + } + } + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "No free port found in port map"); +} + +bool GDBRemoteCommunicationServerPlatform::PortMap::AssociatePortWithProcess( + uint16_t port, lldb::pid_t pid) { + auto pos = m_port_map.find(port); + if (pos != m_port_map.end()) { + pos->second = pid; + return true; + } + return false; +} + +bool GDBRemoteCommunicationServerPlatform::PortMap::FreePort(uint16_t port) { + std::map::iterator pos = m_port_map.find(port); + if (pos != m_port_map.end()) { + pos->second = LLDB_INVALID_PROCESS_ID; + return true; + } + return false; +} + +bool GDBRemoteCommunicationServerPlatform::PortMap::FreePortForProcess( + lldb::pid_t pid) { + if (!m_port_map.empty()) { + for (auto &pair : m_port_map) { + if (pair.second == pid) { + pair.second = LLDB_INVALID_PROCESS_ID; + return true; + } + } + } + return false; +} + +bool GDBRemoteCommunicationServerPlatform::PortMap::empty() const { + return m_port_map.empty(); +} + // GDBRemoteCommunicationServerPlatform constructor GDBRemoteCommunicationServerPlatform::GDBRemoteCommunicationServerPlatform( const Socket::SocketProtocol socket_protocol, const char *socket_scheme) @@ -95,8 +158,13 @@ Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer( const lldb_private::Args &args, std::string hostname, lldb::pid_t &pid, uint16_t &port, std::string &socket_name) { - if (port == UINT16_MAX) - port = GetNextAvailablePort(); + if (port == UINT16_MAX) { + llvm::Expected available_port = m_port_map.GetNextAvailablePort(); + if (available_port) + port = *available_port; + else + return Status(available_port.takeError()); + } // Spawn a new thread to accept the port that gets bound after binding to // port 0 (zero). @@ -152,10 +220,10 @@ std::lock_guard guard(m_spawned_pids_mutex); m_spawned_pids.insert(pid); if (port > 0) - AssociatePortWithProcess(port, pid); + m_port_map.AssociatePortWithProcess(port, pid); } else { if (port > 0) - FreePort(port); + m_port_map.FreePort(port); } return error; } @@ -453,7 +521,7 @@ bool GDBRemoteCommunicationServerPlatform::DebugserverProcessReaped( lldb::pid_t pid) { std::lock_guard guard(m_spawned_pids_mutex); - FreePortForProcess(pid); + m_port_map.FreePortForProcess(pid); m_spawned_pids.erase(pid); return true; } @@ -499,51 +567,6 @@ m_port_map = port_map; } -uint16_t GDBRemoteCommunicationServerPlatform::GetNextAvailablePort() { - if (m_port_map.empty()) - return 0; // Bind to port zero and get a port, we didn't have any - // limitations - - for (auto &pair : m_port_map) { - if (pair.second == LLDB_INVALID_PROCESS_ID) { - pair.second = ~(lldb::pid_t)LLDB_INVALID_PROCESS_ID; - return pair.first; - } - } - return UINT16_MAX; -} - -bool GDBRemoteCommunicationServerPlatform::AssociatePortWithProcess( - uint16_t port, lldb::pid_t pid) { - PortMap::iterator pos = m_port_map.find(port); - if (pos != m_port_map.end()) { - pos->second = pid; - return true; - } - return false; -} - -bool GDBRemoteCommunicationServerPlatform::FreePort(uint16_t port) { - PortMap::iterator pos = m_port_map.find(port); - if (pos != m_port_map.end()) { - pos->second = LLDB_INVALID_PROCESS_ID; - return true; - } - return false; -} - -bool GDBRemoteCommunicationServerPlatform::FreePortForProcess(lldb::pid_t pid) { - if (!m_port_map.empty()) { - for (auto &pair : m_port_map) { - if (pair.second == pid) { - pair.second = LLDB_INVALID_PROCESS_ID; - return true; - } - } - } - return false; -} - const FileSpec &GDBRemoteCommunicationServerPlatform::GetDomainSocketDir() { static FileSpec g_domainsocket_dir; static llvm::once_flag g_once_flag; diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp --- a/lldb/tools/lldb-server/lldb-platform.cpp +++ b/lldb/tools/lldb-server/lldb-platform.cpp @@ -231,7 +231,7 @@ break; } if (ch == 'P') - gdbserver_portmap[portnum] = LLDB_INVALID_PROCESS_ID; + gdbserver_portmap.AllowPort(portnum); else if (ch == 'm') min_gdbserver_port = portnum; else @@ -250,8 +250,8 @@ // Make a port map for a port range that was specified. if (min_gdbserver_port && min_gdbserver_port < max_gdbserver_port) { - for (uint16_t port = min_gdbserver_port; port < max_gdbserver_port; ++port) - gdbserver_portmap[port] = LLDB_INVALID_PROCESS_ID; + gdbserver_portmap = GDBRemoteCommunicationServerPlatform::PortMap( + min_gdbserver_port, max_gdbserver_port); } else if (min_gdbserver_port || max_gdbserver_port) { fprintf(stderr, "error: --min-gdbserver-port (%u) is not lower than " "--max-gdbserver-port (%u)\n", diff --git a/lldb/unittests/Process/gdb-remote/CMakeLists.txt b/lldb/unittests/Process/gdb-remote/CMakeLists.txt --- a/lldb/unittests/Process/gdb-remote/CMakeLists.txt +++ b/lldb/unittests/Process/gdb-remote/CMakeLists.txt @@ -4,6 +4,7 @@ GDBRemoteCommunicationServerTest.cpp GDBRemoteCommunicationTest.cpp GDBRemoteTestUtils.cpp + PortMapTest.cpp LINK_LIBS lldbCore diff --git a/lldb/unittests/Process/gdb-remote/PortMapTest.cpp b/lldb/unittests/Process/gdb-remote/PortMapTest.cpp new file mode 100644 --- /dev/null +++ b/lldb/unittests/Process/gdb-remote/PortMapTest.cpp @@ -0,0 +1,115 @@ +//===-- PortMapTest.cpp ---------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "llvm/Testing/Support/Error.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h" + +using namespace lldb_private::process_gdb_remote; + +TEST(PortMapTest, Constructors) { + // Default construct to empty map + GDBRemoteCommunicationServerPlatform::PortMap p1; + ASSERT_TRUE(p1.empty()); + + // Empty means no restrictions, return 0 and and bind to get a port + llvm::Expected available_port = p1.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::HasValue(0)); + + // Adding any port makes it not empty + p1.AllowPort(1); + ASSERT_FALSE(p1.empty()); + + // So we will return the added port this time + available_port = p1.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::HasValue(1)); + + // Construct from a range of ports + GDBRemoteCommunicationServerPlatform::PortMap p2(1, 4); + ASSERT_FALSE(p2.empty()); + + // Use up all the ports + for (uint16_t expected = 1; expected < 4; ++expected) { + available_port = p2.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::HasValue(expected)); + p2.AssociatePortWithProcess(*available_port, 1); + } + + // Now we fail since we're not an empty port map but all ports are used + available_port = p2.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Failed()); +} + +TEST(PortMapTest, FreePort) { + GDBRemoteCommunicationServerPlatform::PortMap p(1, 4); + // Use up all the ports + for (uint16_t port = 1; port < 4; ++port) { + p.AssociatePortWithProcess(port, 1); + } + + llvm::Expected available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Failed()); + + // Can't free a port that isn't in the map + ASSERT_FALSE(p.FreePort(0)); + ASSERT_FALSE(p.FreePort(4)); + + // After freeing a port it becomes available + ASSERT_TRUE(p.FreePort(2)); + available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::HasValue(2)); +} + +TEST(PortMapTest, FreePortForProcess) { + GDBRemoteCommunicationServerPlatform::PortMap p; + p.AllowPort(1); + p.AllowPort(2); + ASSERT_TRUE(p.AssociatePortWithProcess(1, 11)); + ASSERT_TRUE(p.AssociatePortWithProcess(2, 22)); + + // All ports have been used + llvm::Expected available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Failed()); + + // Can't free a port for a process that doesn't have any + ASSERT_FALSE(p.FreePortForProcess(33)); + + // You can move a used port to a new pid + ASSERT_TRUE(p.AssociatePortWithProcess(1, 99)); + + ASSERT_TRUE(p.FreePortForProcess(22)); + available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded()); + ASSERT_EQ(2, *available_port); + + // proces 22 no longer has a port + ASSERT_FALSE(p.FreePortForProcess(22)); +} + +TEST(PortMapTest, AllowPort) { + GDBRemoteCommunicationServerPlatform::PortMap p; + + // Allow port 1 and tie it to process 11 + p.AllowPort(1); + ASSERT_TRUE(p.AssociatePortWithProcess(1, 11)); + + // Allowing it a second time shouldn't change existing mapping + p.AllowPort(1); + llvm::Expected available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Failed()); + + // A new port is marked as free when allowed + p.AllowPort(2); + available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::HasValue(2)); + + // 11 should still be tied to port 1 + ASSERT_TRUE(p.FreePortForProcess(11)); +}