[lldb] Fixed the error unable to launch a GDB server in API tests

[slydiman]

TestPlatformLaunchGDBServer.py runs ldb-server w/o parameters --min-gdbserver-port, --max-gdbserver-port or --gdbserver-port. So gdbserver_portmap is empty and gdbserver_portmap.GetNextAvailablePort() will return 0. Do not call portmap_for_child.AllowPort(0) in this case. Otherwise portmap_for_child.GetNextAvailablePort() will allocate and never free the port 0 and next call portmap_for_child.GetNextAvailablePort() will fail.

Added few asserts in GDBRemoteCommunicationServerPlatform::PortMap to avoid such issue in the future.

This patch fixes a bug added in #88845. The behaviour is very close to #97537 w/o parameters --min-gdbserver-port, --max-gdbserver-port and --gdbserver-port.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dmitry Vasilyev (slydiman)

Changes

TestPlatformLaunchGDBServer.py runs ldb-server w/o parameters --min-gdbserver-port, --max-gdbserver-port or --gdbserver-port. So gdbserver_portmap is empty and gdbserver_portmap.GetNextAvailablePort() will return 0. Do not pass 0 to portmap_for_child in this case. Added few asserts in GDBRemoteCommunicationServerPlatform::PortMap to avoid such issue in the future.

This patch fixes #97537.

It seems StartDebugserverProcess() ignores the port anyway and parameters --min-gdbserver-port, --max-gdbserver-port and --gdbserver-port are useless at all, but it is out of scope of this patch.

---

Full diff: https://github.com/llvm/llvm-project/pull/98833.diff

2 Files Affected:

• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp (+2)
• (modified) lldb/tools/lldb-server/lldb-platform.cpp (+6-4)

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
index 5285ec1d3db4e..65f1cc12ba307 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
@@ -46,11 +46,13 @@ using namespace lldb_private;
 
 GDBRemoteCommunicationServerPlatform::PortMap::PortMap(uint16_t min_port,
                                                        uint16_t max_port) {
+  assert(min_port);
   for (; min_port < max_port; ++min_port)
     m_port_map[min_port] = LLDB_INVALID_PROCESS_ID;
 }
 
 void GDBRemoteCommunicationServerPlatform::PortMap::AllowPort(uint16_t port) {
+  assert(port);
   // Do not modify existing mappings
   m_port_map.insert({port, LLDB_INVALID_PROCESS_ID});
 }
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp
index cfd0a3797d810..7148a1d2a3094 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -313,9 +313,11 @@ int main_platform(int argc, char *argv[]) {
       GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
       llvm::Expected<uint16_t> available_port =
           gdbserver_portmap.GetNextAvailablePort();
-      if (available_port)
-        portmap_for_child.AllowPort(*available_port);
-      else {
+      if (available_port) {
+        // GetNextAvailablePort() may return 0 if gdbserver_portmap is empty.
+        if (*available_port)
+          portmap_for_child.AllowPort(*available_port);
+      } else {
         llvm::consumeError(available_port.takeError());
         fprintf(stderr,
                 "no available gdbserver port for connection - dropping...\n");
@@ -352,7 +354,7 @@ int main_platform(int argc, char *argv[]) {
     if (platform.IsConnected()) {
       if (inferior_arguments.GetArgumentCount() > 0) {
         lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;
-        std::optional<uint16_t> port = 0;
+        std::optional<uint16_t> port;
         std::string socket_name;
         Status error = platform.LaunchGDBServer(inferior_arguments,
                                                 "", // hostname

[DavidSpickett]

Did you consider making AllowPort(0) do nothing?

My guess is that this port dance is complicated enough as it is, and AllowPort(0) doing nothing would be more implicit behaviour we have to pick apart later.

[slydiman]

Did you consider making AllowPort(0) do nothing?

GetNextAvailablePort() returns 0 in case of the empty map. LaunchGDBServer() does not update the map with the pid if the port is 0. So 0 is the special value which means any. Adding 0 to the map causes an unexpected behavior.

if GetNextAvailablePort() allocated the port 0 after AllowPort(0), it will never be freed because of conditions if (*port > 0) here

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

Lines 209 to 221 in 7b135f7

	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)
	m_port_map.AssociatePortWithProcess(*port, pid);
	} else {
	if (*port > 0)
	m_port_map.FreePort(*port);
	}

[slydiman]

The best solution is simply to remove all code related to gdb port mapping, because it does not work at all. Try to run netstat and you will be surprised.

1. lldb-server in gdbserver mode completely ignores the port from the url tcp://[hostname]:port. It binds to port 0 and it is hardcoded.
2. Currently lldb-server in platform mode always runs lldb-server gdbserver tcp://[hostname]:0 because of the bug added 4 years ago and no one noticed this issue.

If gdb port parameters specified, now it causes issues like #97537, but a random port will be used for connection anyway.

If no gdb port parameters specified, the port map must be empty and must stay empty. AllowPort(0) makes the port map not empty and causes an unexpected behavior. This patch fixes this issue.

[DavidSpickett]

The best solution is simply to remove all code related to gdb port mapping, because it does not work at all.

On the one hand I agree, on the other I use this feature a lot and it works for my limited uses (the race condition aside).
That use is doing early architecture work on simulators, where sometimes I can do the IP tables dance to be able to use any port,
sometimes I can't.

If I can find an alternative that allows me to run tests in enironments with limited port forwarding, I'd be happy for it to be removed.

lldb-server in gdbserver mode completely ignores the port from the url tcp://[hostname]:port. It binds to port 0 and it is hardcoded.

Not sure what you mean here, I don't see that happening on my machine:

$ ./bin/lldb-server gdbserver tcp://127.0.0.1:54321 -- /tmp/test.o
<...>
$ sudo netstat -tulpn
Active Internet connections (only servers)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       PID/Program name
tcp        0      0 127.0.0.1:54321         0.0.0.0:*               LISTEN      2345153/./bin/lldb-

Perhaps I am spelling the port option differently, or you mean a lldb-server gdbserver launched by a platform ignores the port.

[DavidSpickett]

AllowPort(0) makes the port map not empty and causes an unexpected behavior.

Agreed, so silent UB would just be making things worse.

[slydiman]

Agreed, so silent UB would just be making things worse.

So, this patch fixes that issue, right?

Perhaps I am spelling the port option differently, or you mean a lldb-server gdbserver launched by a platform ignores the port.

It seems it depends on the system.

If I can find an alternative that allows me to run tests in enironments with limited port forwarding, I'd be happy for it to be removed.

Finally we just configured VMs to use a network bridge with own IP w/o any port limits and do not map ports manually to host's ports anymore.

Note I'm working on a patch to fix #97537 and all port mapping related issues. The idea is to use threads for platform mode and use a common port map for all connections instead of making a new portmap_for_child with 1 port.

[DavidSpickett]

So, this patch fixes that issue, right?

Yes I think so, give me some time to test it myself.

[DavidSpickett]

I tested this with the port mapping I usually use for simulator development and it works fine.

Thanks for the fix.