[lldb] Error when there are no ports to launch a gdbserver on

[DavidSpickett]

Summary

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.

[DavidSpickett]

I have found existing tests that launch an lldb-server with specific arguments, using an existing lldb-server platform to do it.

The ideal test would be:

• run lldb-server with min port some known to be available port, max is that +1
• connect the client
• send qLaunchGDBServer twice, expect that the second one has an E response

What I can't work out how to do is decide which ports you'd need to use for the --min/max-gdbserver-port options. I couldn't find a way to predict what ports would be
available before you launch the lldb-server. Or guarantee that any ports that were available wouldn't get used up in the time between you finding them and launching the new lldb-server.

You have to give lldb-server at least one port number to use so giving it none and expecting error doesn't work.
Giving it a port you know is already used doesn't help either, since this code would still say it's available but then fail connect to it.

So presented without a test (and the most minimal code change). Ideas welcome though.

[labath]

Yeah, I don't think it's possible to write a reliable end-to-end test for this feature. A unit test might be an option -- I don't know how hard it would be to mock the parts needed (starting a debug server). One could also consider refactoring this code to avoid the usage of a valid port number to represent the "no port" state (and while doing so, making the code easier to unit test).

While I would encourage you to try some of these things out, I wouldn't say that that's really required for this bugfix.

BTW, the --min/max-gdbserver-port is a constant source of bug reports (last one was last weel -- I'll have to reply to that one) and ideally I'd just do away with it. It just leaves a bad FTP taste in the mouth..

[DavidSpickett]

I hadn't considered using a unit test instead, I'll give that a go.

[DavidSpickett]

• Move PortMap into a class we can unittest
• Use llvm::Expected for GetNextAvailablePort

[DavidSpickett]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h

30

Only the one in GDBRemoteCommunicationServerPlatform is/was used.

[labath]

Looks good. Just some cosmetic details...

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h

30

cool

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp

74

auto, maybe

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h

29–30

Add some comments to explain what these do.

lldb/tools/lldb-server/lldb-platform.cpp

234

Why the cast?

lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp

1 ↗

(On Diff #307356)

Please put this under unittests/Process/gdb-remote. The tests in here are a failed experiment, and I'll hopefully get rid of them soon.

24–25 ↗

(On Diff #307356)

ASSERT_THAT_EXPECTED(p1.GetNextAvailablePort(), llvm::HasValue(0));

33–34 ↗

(On Diff #307356)

Same, here, and further on...

[DavidSpickett]

• Use auto for map find
• Document PortMap functions
• Remove unnecessary casts
• Move tests to Process/gdb-remote
• Cleanup Expected asserts

The behaviour of AssociatePortWithProcess is a bit
non obvious (that it can move a port to a different pid)
but I'm not inclined to change it now.

So I've just added a FIXME to note that this usage
will probably fail if we do later share the PortMap.

[DavidSpickett]

lldb/tools/lldb-server/lldb-platform.cpp

234

Me misreading a warning I think, removed here and below.

[labath]

I was just asking for comments on the two constructors, but having them elsewhere is nice too.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h

29–31

This looks like a class-level comment. Could you also say a few words about what the constructor does specifically.

[DavidSpickett]

• Document default constructor behaviour.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h

 
 namespace lldb_private {
 namespace process_gdb_remote {
 
 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)>;
 
   GDBRemoteCommunicationServer(const char *comm_name,
                                const char *listener_name);
 
   ~GDBRemoteCommunicationServer() override;

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h

 
 #include <map>
 #include <mutex>
 #include <set>
 
 #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;
-
-  GDBRemoteCommunicationServerPlatform(
-      const Socket::SocketProtocol socket_protocol, const char *socket_scheme);
-
-  ~GDBRemoteCommunicationServerPlatform() override;
-
-  Status LaunchProcess() override;
-
-  // Set both ports to zero to let the platform automatically bind to
-  // a port chosen by the OS.
-  void SetPortMap(PortMap &&port_map);
+  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 UINT16_MAX
+    // 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.
-  uint16_t GetNextAvailablePort();
+    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);
+
+  ~GDBRemoteCommunicationServerPlatform() override;
+
+  Status LaunchProcess() override;
+
+  // Set both ports to zero to let the platform automatically bind to
+  // a port chosen by the OS.
+  void SetPortMap(PortMap &&port_map);
+
   void SetPortOffset(uint16_t port_offset);
 
   void SetInferiorArguments(const lldb_private::Args &args);
 
   Status LaunchGDBServer(const lldb_private::Args &args, std::string hostname,
                          lldb::pid_t &pid, uint16_t &port,
                          std::string &socket_name);
 

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp

 #include "lldb/Utility/UriParser.h"
 
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 
 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)
     : GDBRemoteCommunicationServerCommon("gdb-remote.server",
                                          "gdb-remote.server.rx_packet"),
       m_socket_protocol(socket_protocol), m_socket_scheme(socket_scheme),
       m_spawned_pids_mutex(), m_port_map(), m_port_offset(0) {
   m_pending_gdb_server.pid = LLDB_INVALID_PROCESS_ID;

 }
 
 // Destructor
 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).
 
   // ignore the hostname send from the remote end, just use the ip address that
   // we're currently communicating with as the hostname
 
   // Spawn a debugserver and try to get the port it listens to.
#endif
   Status error = StartDebugserverProcess(
       url.str().c_str(), nullptr, debugserver_launch_info, port_ptr, &args, -1);
 
   pid = debugserver_launch_info.GetProcessID();
   if (pid != LLDB_INVALID_PROCESS_ID) {
     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;
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerPlatform::Handle_qLaunchGDBServer(
     StringExtractorGDBRemote &packet) {
   // Spawn a local debugserver as a platform so we can then attach or launch a
GDBRemoteCommunicationServerPlatform::Handle_jSignalsInfo(
   StreamString response;
   signal_array.Dump(response);
   return SendPacketNoLock(response.GetString());
 }
 
 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;
 }
 
 Status GDBRemoteCommunicationServerPlatform::LaunchProcess() {
   if (!m_process_launch_info.GetArguments().GetArgumentCount())
     return Status("%s: no process command line specified to launch",
                   __FUNCTION__);
Status GDBRemoteCommunicationServerPlatform::LaunchProcess() {
 
   return error;
 }
 
 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;
 
   llvm::call_once(g_once_flag, []() {
     const char *domainsocket_dir_env =
         ::getenv("LLDB_DEBUGSERVER_DOMAINSOCKET_DIR");
     if (domainsocket_dir_env != nullptr)

lldb/tools/lldb-server/lldb-platform.cpp

case 'M': {
       if (portnum < LOW_PORT || portnum > HIGH_PORT) {
         llvm::errs() << llvm::formatv("error: port number {0} is not in the "
                                       "valid user port range of {1} - {2}\n",
                                       portnum, LOW_PORT, HIGH_PORT);
         option_error = 1;
         break;
       }
       if (ch == 'P')
-        gdbserver_portmap[portnum] = LLDB_INVALID_PROCESS_ID;
+        gdbserver_portmap.AllowPort(portnum);
       else if (ch == 'm')
         min_gdbserver_port = portnum;
       else
         max_gdbserver_port = portnum;
     } break;
 
     case 'h': /* fall-through is intentional */
     case '?':
       show_usage = true;
       break;
     }
   }
 
   if (!LLDBServerUtilities::SetupLogging(log_file, log_channels, 0))
     return -1;
 
   // 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",
             min_gdbserver_port, max_gdbserver_port);
     option_error = 3;
   }
 
   // Print usage and exit if no listening port is specified.

lldb/unittests/Process/gdb-remote/CMakeLists.txt

 add_lldb_unittest(ProcessGdbRemoteTests
   GDBRemoteClientBaseTest.cpp
   GDBRemoteCommunicationClientTest.cpp
   GDBRemoteCommunicationServerTest.cpp
   GDBRemoteCommunicationTest.cpp
   GDBRemoteTestUtils.cpp
+  PortMapTest.cpp
 
   LINK_LIBS
     lldbCore
     lldbHost
     lldbPluginPlatformMacOSX
     lldbPluginProcessUtility
     lldbPluginProcessGDBRemote
 
     LLVMTestingSupport
 
   LINK_COMPONENTS
     Support
   )

lldb/unittests/Process/gdb-remote/PortMapTest.cpp

+//===-- 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));
+}