[Lldb-commits] [lldb] 98e87f7 - [lldb] Error when there are no ports to launch a gdbserver on
David Spickett via lldb-commits
lldb-commits at lists.llvm.org
Mon Nov 30 02:19:22 PST 2020
Author: David Spickett
Date: 2020-11-30T10:19:14Z
New Revision: 98e87f76d0f486122d76b334232102e0d7a9254d
URL: https://github.com/llvm/llvm-project/commit/98e87f76d0f486122d76b334232102e0d7a9254d
DIFF: https://github.com/llvm/llvm-project/commit/98e87f76d0f486122d76b334232102e0d7a9254d.diff
LOG: [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
Added:
lldb/unittests/Process/gdb-remote/PortMapTest.cpp
Modified:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
lldb/tools/lldb-server/lldb-platform.cpp
lldb/unittests/Process/gdb-remote/CMakeLists.txt
Removed:
################################################################################
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
index 63567bb9b5de..a1cf70f9cd1a 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<uint16_t, lldb::pid_t>;
using PacketHandler =
std::function<PacketResult(StringExtractorGDBRemote &packet,
Status &error, bool &interrupt, bool &quit)>;
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
index 7e94afb9ec68..ae1260221e56 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<uint16_t>
+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<uint16_t, lldb::pid_t>::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<uint16_t> 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<std::recursive_mutex> 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<std::recursive_mutex> 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 8b3122d1423e..69261dd2ad64 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<uint16_t, lldb::pid_t> 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<uint16_t> 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<uint16_t, lldb::pid_t> m_port_map;
+ };
GDBRemoteCommunicationServerPlatform(
const Socket::SocketProtocol socket_protocol, const char *socket_scheme);
@@ -35,21 +82,6 @@ class GDBRemoteCommunicationServerPlatform
// 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 ba3b6c59185c..05bfe71117c6 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 abb30e022e49..7988fff3e70c 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 000000000000..496a55be7ffc
--- /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<uint16_t> 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<uint16_t> 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<uint16_t> 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<uint16_t> 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));
+}
More information about the lldb-commits
mailing list