From 98e87f76d0f486122d76b334232102e0d7a9254d Mon Sep 17 00:00:00 2001 From: David Spickett Date: Tue, 17 Nov 2020 15:41:04 +0000 Subject: [PATCH] [lldb] Error when there are no ports to launch a gdbserver on Previously if you did: $ lldb-server platform --server <...> --min-gdbserver-port 12346 --max-gdbserver-port 12347 (meaning only use port 12346 for gdbservers) Then tried to launch two gdbservers on the same connection, the second one would return port 65535. Which is a real port number but it actually means lldb-server didn't find one it was allowed to use. send packet: $qLaunchGDBServer;<...> read packet: $pid:1919;port:12346;#c0 <...> send packet: $qLaunchGDBServer;<...> read packet: $pid:1927;port:65535;#c7 This situation should be an error even if port 65535 does happen to be available on the current machine. To fix this make PortMap it's own class within GDBRemoteCommunicationServerPlatform. This almost the same as the old typedef but for GetNextAvailablePort() returning an llvm::Expected. This means we have to handle not finding a port, by returning an error packet. Also add unit tests for this new PortMap class. Reviewed By: labath Differential Revision: https://reviews.llvm.org/D91634 --- .../gdb-remote/GDBRemoteCommunicationServer.h | 1 - .../GDBRemoteCommunicationServerPlatform.cpp | 123 ++++++++++++--------- .../GDBRemoteCommunicationServerPlatform.h | 64 ++++++++--- lldb/tools/lldb-server/lldb-platform.cpp | 6 +- lldb/unittests/Process/gdb-remote/CMakeLists.txt | 1 + lldb/unittests/Process/gdb-remote/PortMapTest.cpp | 115 +++++++++++++++++++ 6 files changed, 240 insertions(+), 70 deletions(-) create mode 100644 lldb/unittests/Process/gdb-remote/PortMapTest.cpp diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h index 63567bb..a1cf70f 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h @@ -27,7 +27,6 @@ class ProcessGDBRemote; class GDBRemoteCommunicationServer : public GDBRemoteCommunication { public: - using PortMap = std::map; using PacketHandler = std::function; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp index 7e94afb..ae12602 100644 --- 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; 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 @@ GDBRemoteCommunicationServerPlatform::~GDBRemoteCommunicationServerPlatform() {} 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 @@ Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer( 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 @@ GDBRemoteCommunicationServerPlatform::Handle_jSignalsInfo( 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 @@ void GDBRemoteCommunicationServerPlatform::SetPortMap(PortMap &&port_map) { 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/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h index 8b3122d..69261dd 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h @@ -16,13 +16,60 @@ #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. + + // Construct an empty map, where empty means any port is allowed. + 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 +82,6 @@ public: // 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/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp index ba3b6c5..05bfe71 100644 --- a/lldb/tools/lldb-server/lldb-platform.cpp +++ b/lldb/tools/lldb-server/lldb-platform.cpp @@ -231,7 +231,7 @@ int main_platform(int argc, char *argv[]) { 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 @@ int main_platform(int argc, char *argv[]) { // 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 index abb30e0..7988fff 100644 --- a/lldb/unittests/Process/gdb-remote/CMakeLists.txt +++ b/lldb/unittests/Process/gdb-remote/CMakeLists.txt @@ -4,6 +4,7 @@ add_lldb_unittest(ProcessGdbRemoteTests 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 index 0000000..496a55b --- /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)); +} -- 2.7.4