llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Anthony Ha (Awfa)
<details>
<summary>Changes</summary>
`lldb-server`'s platform mode seems to have an issue with its
`--min-gdbserver-port` `--max-gdbserver-port` flags (and probably the
`--gdbserver-port` flag, but I didn't test it).
How the platform code seems to work is that it listens on a port, and whenever
there's an incoming connection, it forks the process to handle the connection.
To handle the port flags, the main process uses an instance of the helper class
`GDBRemoteCommunicationServerPlatform::PortMap`, that can be configured and
track usages of ports. The child process handling the platform connection, can
then use the port map to allocate a port for the gdb-server connection it will
make (this is another process it spawns).
However, in the current code, this works only once. After the first connection
is handled by forking a child process, the main platform listener code loops
around, and then 'forgets' about the port map. This is because this code:
```cpp
GDBRemoteCommunicationServerPlatform platform(
acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
if (!gdbserver_portmap.empty()) {
platform.SetPortMap(std::move(gdbserver_portmap));
}
```
is within the connection listening loop. This results in the
`gdbserver_portmap` being moved into the platform object at the beginning of
the first iteration of the loop, but on the second iteration, after the first
fork, the next instance of the platform object will not have its platform port
mapped.
The result of this bug is that subsequent connections to the platform, when
spawning the gdb-remote connection, will be supplied a random port - which
isn't bounded by the `--min-gdbserver-port` and `--max-gdbserver--port`
parameters passed in by the user.
This PR fixes this issue by having the port map be maintained by the parent
platform listener process. On connection, the listener allocates a single
available port from the port map, associates the child process pid with the
port, and lets the connection handling child use that single port number.
Additionally, when cleaning up child processes, the main listener process
tracks the child that exited to deallocate the previously associated port, so
it can be reused for a new connection.
For review:
- I haven't used C++ optionals before this, and was going off of surrounding
related code. Let me know if I got something wrong.
- I'm not sure where this code could be tested. This was only manually tested
by me so far.
---
Full diff: https://github.com/llvm/llvm-project/pull/88845.diff
1 Files Affected:
- (modified) lldb/tools/lldb-server/lldb-platform.cpp (+28-13)
``````````diff
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp
b/lldb/tools/lldb-server/lldb-platform.cpp
index 3e126584eb25b4..384709ba79b656 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -282,17 +282,10 @@ int main_platform(int argc, char *argv[]) {
}
}
- do {
- GDBRemoteCommunicationServerPlatform platform(
- acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
-
- if (port_offset > 0)
- platform.SetPortOffset(port_offset);
-
- if (!gdbserver_portmap.empty()) {
- platform.SetPortMap(std::move(gdbserver_portmap));
- }
+ GDBRemoteCommunicationServerPlatform platform(
+ acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
+ do {
const bool children_inherit_accept_socket = true;
Connection *conn = nullptr;
error = acceptor_up->Accept(children_inherit_accept_socket, conn);
@@ -301,13 +294,31 @@ int main_platform(int argc, char *argv[]) {
exit(socket_error);
}
printf("Connection established.\n");
+
+ std::optional<uint16_t> port = 0;
if (g_server) {
// Collect child zombie processes.
#if !defined(_WIN32)
- while (waitpid(-1, nullptr, WNOHANG) > 0)
- ;
+ auto waitResult = waitpid(-1, nullptr, WNOHANG);
+ while (waitResult > 0) {
+ // waitResult is the child pid
+ gdbserver_portmap.FreePortForProcess(waitResult);
+ waitResult = waitpid(-1, nullptr, WNOHANG);
+ }
#endif
- if (fork()) {
+ llvm::Expected<uint16_t> available_port =
+ gdbserver_portmap.GetNextAvailablePort();
+ if (available_port)
+ port = *available_port;
+
+ else {
+ fprintf(stderr, "no available port for connection - dropping...\n");
+ delete conn;
+ continue;
+ }
+ auto childPid = fork();
+ if (childPid) {
+ gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
// Parent doesn't need a connection to the lldb client
delete conn;
@@ -324,6 +335,10 @@ int main_platform(int argc, char *argv[]) {
// connections while a connection is active.
acceptor_up.reset();
}
+
+ GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
+ portmap_for_child.AllowPort(*port);
+ platform.SetPortMap(std::move(portmap_for_child));
platform.SetConnection(std::unique_ptr<Connection>(conn));
if (platform.IsConnected()) {
``````````
</details>
https://github.com/llvm/llvm-project/pull/88845
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
