[lldb] Use correct path for lldb-server executable

[yuvald-sweet-security]

Hey,

This solves an issue where running lldb-server-20 with a non-absolute path (for example, when it's installed into /usr/bin and the user runs it as lldb-server-20 ... and not /usr/bin/lldb-server-20 ...) fails with error: spawn_process failed: execve failed: No such file or directory. The underlying issue is that when run that way, it attempts to execute a binary named lldb-server-20 from its current directory. This is also a mild security hazard because lldb-server is often being run as root in the directory /tmp, meaning that an unprivileged user can create the file /tmp/lldb-server-20 and lldb-server will execute it as root. (although, well, it's a debugging server we're talking about, so that may not be a real concern)

I haven't previously contributed to this project; if you want me to change anything in the code please don't hesitate to let me know.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Yuval Deutscher (yuvald-sweet-security)

Changes

Hey,

This solves an issue where running lldb-server-20 with a non-absolute path (for example, when it's installed into /usr/bin and the user runs it as lldb-server-20 ... and not /usr/bin/lldb-server-20 ...) fails with error: spawn_process failed: execve failed: No such file or directory. The underlying issue is that when run that way, it attempts to execute a binary named lldb-server-20 from its current directory. This is also a mild security hazard because lldb-server is often being run as root in the directory /tmp, meaning that an unprivileged user can create the file /tmp/lldb-server-20 and lldb-server will execute it as root.

I haven't previously contributed to this project; if you want me to change anything in the code please don't hesitate to let me know.

---

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

1 Files Affected:

• (modified) lldb/tools/lldb-server/lldb-platform.cpp (+16-1)

diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp
index 880b45b989b9c..103e1ac02843d 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -545,13 +545,28 @@ int main_platform(int argc, char *argv[]) {
 
   MainLoop main_loop;
   {
+    char progpath[1024];
+#if defined(_WIN32)
+    if (GetModuleFileName(NULL, progpath, sizeof(progpath)) == 0) {
+      printf("Error retrieving executable path.\n");
+      return 1;
+    }
+#else
+    ssize_t len = readlink("/proc/self/exe", progpath, sizeof(progpath) - 1);
+    if (len == -1) {
+      perror("readlink");
+      return 1;
+    }
+    path[len] = '\0';
+#endif
+
     llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> platform_handles =
         platform_sock->Accept(
             main_loop, [progname, gdbserver_port, &inferior_arguments, log_file,
                         log_channels, &main_loop,
                         &platform_handles](std::unique_ptr<Socket> sock_up) {
               printf("Connection established.\n");
-              Status error = spawn_process(progname, sock_up.get(),
+              Status error = spawn_process(progpath, sock_up.get(),
                                            gdbserver_port, inferior_arguments,
                                            log_file, log_channels, main_loop);
               if (error.Fail()) {

[labath]

I'm wondering if the right fix isn't to call FileSystem::ResolveExecutableLocation on argv[0]. That should should give us more-or-less the same algorithm that the shell used when starting lldb-server.

[yuvald-sweet-security]

I'm wondering if the right fix isn't to call FileSystem::ResolveExecutableLocation on argv[0]. That should should give us more-or-less the same algorithm that the shell used when starting lldb-server.

Perhaps, but note that technically argv[0] doesn't really have to be related to the name of the executable at all - the user can pretty much pass whatever value they want as argv[0] when calling execve (although I agree that it would be a very peculiar workflow for someone to run lldb-server with an argv[0] that isn't the name of the binary).

On another note, I am currently debugging a few other regressions introduced in LLDB 20 that relate to the way the platform mode server executes another server internally. For example, while this patch makes lldb-server work as intended if e.g. installed to /usr/local/bin/lldb-server, if I install it as /usr/local/bin/not-lldb-server, then it functions right when invoked as not-lldb-server but does not function right if invoked with an absolute path because LLDB does not correctly deduce the support exe directory, which is very curious behaviour indeed:

$ sudo ln -s /home/user/random/llvm-project/build/bin/lldb-server /usr/local/bin/not-lldb-server

$ not-lldb-server platform --server --listen '*:1338' --log-channels "lldb all"
Listen to [*]:1338
Listen to [*]:0
Connection established.
started monitoring child process.
lldb-platform launched '/home/user/random/llvm-project/build/bin/lldb-server platform --child-platform-fd 8 --gdbserver-port 37663 --log-channels lldb all', pid=49632
thread created
0x587b5409a160 Socket::Close (fd = 8)
pid = 49632
::waitpid(49632, &status, 0)...
Launching debugserver url='', fd=8...
shlib dir -> `(empty)`
support exe dir -> `/home/user/random/llvm-project/build/bin/`
started monitoring child process.
thread created
GDBRemoteCommunicationServerPlatform::LaunchGDBServer() debugserver launched successfully as pid 49672
pid = 49672
::waitpid(49672, &status, 0)...
0x00007FFE94504A88 Communication::Disconnect ()
0x587b5409d900 Socket::Close (fd = 8)
::waitpid(49672, &status, 0) => pid = 49672, status = 0x0
pid = 49672 thread exiting...
^C

$ /usr/local/bin/not-lldb-server platform --server --listen '*:1338' --log-channels "lldb all"                                                                                                                              Listen to [*]:1338
Listen to [*]:0
Connection established.
started monitoring child process.
lldb-platform launched '/home/user/random/llvm-project/build/bin/lldb-server platform --child-platform-fd 8 --gdbserver-port 40461 --log-channels lldb all', pid=49799
0x5d0837133160 Socket::Close (fd = 8)
thread created
pid = 49799
::waitpid(49799, &status, 0)...
Launching debugserver url='', fd=8...
shlib dir -> `/usr/local/bin/`
Attempting to derive the path /bin relative to liblldb install path: /usr/local/bin
Derived the path as: /usr/local/bin
support exe dir -> `/usr/local/bin/`
GDBRemoteCommunicationServerPlatform::LaunchGDBServer() debugserver launch failed: unable to locate lldb-server
0x00007FFE655B2738 Communication::Disconnect ()
0x5d0837136900 Socket::Close (fd = 8)
Launching debugserver url='', fd=8...
GDBRemoteCommunicationServerPlatform::LaunchGDBServer() debugserver launch failed: unable to locate lldb-server
0x00007FFE655B2738 Communication::Disconnect ()
0x5d083716e340 Socket::Close (fd = 8)

(This is an important workflow because that is precisely the way that lldb-server gets installed in distro packaging, e.g. lldb-server-20 links to the lldb-server binary in the LLVM installation directory)

EDIT: I'll make a separate PR for this issue
EDIT2: #131609

[yuvald-sweet-security]

Ah, turns out there's a HostInfo::GetProgramFileSpec() function which solves both these issues.

[JDevlieghere]

LGTM

[labath]

Ah, turns out there's a HostInfo::GetProgramFileSpec() function which solves both these issues.

I am aware of that function. The reason I did not suggest it is because GetProgramFileSpec (on linux) returns the canonical path (with all symlinks resolved). That's something I am generally trying to avoid because it can break some instalation types -- where the installation consists of a symlink farm and the expected installation layout is given by the symlink tree. (It can also fix some installations which expect exactly the opposite -- the debian use case in one of them).

It's very hard to come up with a solution which makes everyone happy, which is why e.g. gcc/clang have a flag (-no-canonical-prefixes) to control this kind of behavior. I'm not thrilled by the idea of adding that to lldb, nor I think it would be practical for an interactive tool.

Whatever we do, I think it is better to resolve the symlink as late as possible as that gives us more options down the line. What I don't understand is why doesn't this work for your use case already -- GetModuleFileSpecForHostAddress calls FileSystem::Resolve, which should give you the canonical path for the library. Does that fail for some reason? It is it because the (relative) path returned by GetModuleFileSpecForHostAddress is resolved relative to CWD (but in reality we've executed it by finding the executable on the path)?

[yuvald-sweet-security]

Ah, turns out there's a HostInfo::GetProgramFileSpec() function which solves both these issues.

I am aware of that function. The reason I did not suggest it is because GetProgramFileSpec (on linux) returns the canonical path (with all symlinks resolved). That's something I am generally trying to avoid because it can break some instalation types -- where the installation consists of a symlink farm and the expected installation layout is given by the symlink tree. (It can also fix some installations which expect exactly the opposite -- the debian use case in one of them).

I understand, but I think that for lldb-server this isn't much of an issue as it seems relatively stand-alone. I'd be more worried if this code attempted to execute a binary that isn't lldb-server from the LLVM binary directory; however, since this code is strictly used for lldb-server to re-execute itself with different arguments, I don't think relying on GetProgramFileSpec is an issue - and it is definitely better than using argv[0], which might as well be the canonical path, depending on how the user executed it.

Whatever we do, I think it is better to resolve the symlink as late as possible as that gives us more options down the line. What I don't understand is why doesn't this work for your use case already -- GetModuleFileSpecForHostAddress calls FileSystem::Resolve, which should give you the canonical path for the library. Does that fail for some reason? It is it because the (relative) path returned by GetModuleFileSpecForHostAddress is resolved relative to CWD (but in reality we've executed it by finding the executable on the path)?

I'm assuming this is referring to the other issue, which I discuss in #131609? Because the issue in the current PR is not related to GetModuleFileSpecForHostAddress - argv[0] is piped directly into spawn_process without any canonicalization of paths.

[labath]

Ah, turns out there's a HostInfo::GetProgramFileSpec() function which solves both these issues.

I am aware of that function. The reason I did not suggest it is because GetProgramFileSpec (on linux) returns the canonical path (with all symlinks resolved). That's something I am generally trying to avoid because it can break some instalation types -- where the installation consists of a symlink farm and the expected installation layout is given by the symlink tree. (It can also fix some installations which expect exactly the opposite -- the debian use case in one of them).

I understand, but I think that for lldb-server this isn't much of an issue as it seems relatively stand-alone. I'd be more worried if this code attempted to execute a binary that isn't lldb-server from the LLVM binary directory; however, since this code is strictly used for lldb-server to re-execute itself with different arguments, I don't think relying on GetProgramFileSpec is an issue - and it is definitely better than using argv[0], which might as well be the canonical path, depending on how the user executed it.

Not exactly. I my use case, lldb-server would be a symlink to something like /cas/dead/beef/baadf00d. The symlink value is supposed to be an implementation detail of the CAS system. While the user could execute that directly, it wouldn't be totally surprising that it does not work.

And the lldb-server binary is not completely standalone as it needs to execute the gdbserver binary (which on linux is actually still the same binary, but the code kind of doesn't know that), which is why you have #131609. In this scenario, there's no way to find the lldb-server binary (or anything else) because the CAS tree contains only a bunch of hex files.

That said, I'm slightly warming up towards using GetProgramFileSpec because I realized it is (mostly) possible (I'm not saying you have to do that) to implement it to return a non-canonical path by looking at auxiliary vector (AT_EXECFN). One tricky aspect of that is that if you execute the binary using a relative path (./lldb-server) then the auxiliary vector will contain a relative path, which means one would have to fetch it and make it absolute sufficiently early so that the CWD doesn't change.

Whatever we do, I think it is better to resolve the symlink as late as possible as that gives us more options down the line. What I don't understand is why doesn't this work for your use case already -- GetModuleFileSpecForHostAddress calls FileSystem::Resolve, which should give you the canonical path for the library. Does that fail for some reason? It is it because the (relative) path returned by GetModuleFileSpecForHostAddress is resolved relative to CWD (but in reality we've executed it by finding the executable on the path)?

I'm assuming this is referring to the other issue, which I discuss in #131609? Because the issue in the current PR is not related to GetModuleFileSpecForHostAddress - argv[0] is piped directly into spawn_process without any canonicalization of paths.

Kind of yes, but the two issues are related. The issue with symlinks is that once you resolve them, there's no way to go back. I'm asking why (in the scenario you describe in that PR) does GetModuleFileSpecForHostAddress return "lldb-server-19" (instead of "/usr/bin/lldb-server-19" or "/usr/lib/llvm-19/bin/lldb-server")

[yuvald-sweet-security]

Kind of yes, but the two issues are related. The issue with symlinks is that once you resolve them, there's no way to go back. I'm asking why (in the scenario you describe in that PR) does GetModuleFileSpecForHostAddress return "lldb-server-19" (instead of "/usr/bin/lldb-server-19" or "/usr/lib/llvm-19/bin/lldb-server")

So, regarding that issue - first of all, GetModuleFileSpecForHostAddress doesn't follow symlinks. I'm not sure if that's a bug or intended behavior but I don't see anything in FileSystem::Resolve that attempts to follow symlinks - it just makes a relative or ~-path absolute.

Now, what you said about resolving relative to cwd instead of relative to $PATH also happens there, but that's actually the reason the entire thing works in the first place - the fact that this resolves relative to cwd causes ComputePathRelativeToLibrary to fail and then ComputeSupportExeDirectory falls back to using GetProgramFileSpec. So, this bug masks the first bug.

[yuvald-sweet-security]

Not exactly. I my use case, lldb-server would be a symlink to something like /cas/dead/beef/baadf00d. The symlink value is supposed to be an implementation detail of the CAS system. While the user could execute that directly, it wouldn't be totally surprising that it does not work.

Then maybe we should just execute the path given by GetProgramFileSpec but pass our argv[0] into execve's argv[0] (I don't know if the current process execution abstraction allows that)

[labath]

Kind of yes, but the two issues are related. The issue with symlinks is that once you resolve them, there's no way to go back. I'm asking why (in the scenario you describe in that PR) does GetModuleFileSpecForHostAddress return "lldb-server-19" (instead of "/usr/bin/lldb-server-19" or "/usr/lib/llvm-19/bin/lldb-server")

So, regarding that issue - first of all, GetModuleFileSpecForHostAddress doesn't follow symlinks. I'm not sure if that's a bug or intended behavior but I don't see anything in FileSystem::Resolve that attempts to follow symlinks - it just makes a relative or ~-path absolute.

I'm sorry, my bad. I saw the tilde resolution code and somehow assumed that canonicalization must follow. I think the lack of symlink resolution is intentional.

Now, what you said about resolving relative to cwd instead of relative to $PATH also happens there, but that's actually the reason the entire thing works in the first place - the fact that this resolves relative to cwd causes ComputePathRelativeToLibrary to fail

.. because of this check, presumably

and then ComputeSupportExeDirectory falls back to using GetProgramFileSpec. So, this bug masks the first bug.

Got it. And I think I finally understand what those checks in ComputeSupportExeDirectory are doing.

Not exactly. I my use case, lldb-server would be a symlink to something like /cas/dead/beef/baadf00d. The symlink value is supposed to be an implementation detail of the CAS system. While the user could execute that directly, it wouldn't be totally surprising that it does not work.

Then maybe we should just execute the path given by GetProgramFileSpec but pass our argv[0] into execve's argv[0] (I don't know if the current process execution abstraction allows that)

I think that'd be fine, given that I know how to get GetProgramFileSpec to return the non-canonical path if that becomes necessary. And it matches what the code was doing originally. Process(Launch)Info has a [GS]etArg0 method, but it looks like ProcessLauncherPosixFork doesn't actually make use of it. It shouldn't be too hard to change that though. Would you like to try that? (If not, I can do it, probably next week)

[yuvald-sweet-security]

I wrote something, lets see if it works

Process(Launch)Info has a [GS]etArg0 method, but it looks like ProcessLauncherPosixFork doesn't actually make use of it

It looks like nothing makes use of it except for the qemu launcher; the posix launcher just uses the first element in info.GetArguments(). I set both to be safe.

EDIT: nevermind, I see what you mean. But now it's kind of awkard that we have a redundancy of information between the argv vector and the arg0 field. It seems that the intention of whoever designed ProcessInfo many years ago was that m_arg0 would be argv[0], and m_arguments would be argv[1..] (as evident from this comment). However, this was ignored and nowadays both major launchers (ProcessLauncherPosixFork and ProcessLauncherWindows) completely ignore m_arg0 and treat m_arguments as the full args vector.

[DavidSpickett]

I think this #63468 is the same sort of issue. Not sure if it's the same though.

As you can tell from me saying "I will get back to this" 2 years ago, the knowledge of any of that has left my head by now, but thank you for looking into this. I know it's not easy to unpick.

[labath]

Sorry about the delay. I wasn't quite happy with the hedging in the implementation, and I also wanted to write a test for it, but I figured it would be easier if I did it myself. I now have #133093. You can rebase your patch on top of that when it lands.

[yuvald-sweet-security]

I think this #63468 is the same sort of issue. Not sure if it's the same though.

As you can tell from me saying "I will get back to this" 2 years ago, the knowledge of any of that has left my head by now, but thank you for looking into this. I know it's not easy to unpick.

The issue you linked is probably the other issue which I encountered and fixed recently (#131609). The issue fixed in the PR we're currently writing in is a regression introduced in LLVM 20 due to a fundamental change in how lldb-server platform mode works, and so it doesn't make sense to me that it existed 2 years ago.

[yuvald-sweet-security]

@labath I rebased it

[labath]

Awesome. Thanks for your patience.

[yuvald-sweet-security]

@JDevlieghere @labath how do I get this PR merged? I don't think I have access to merge on my own; will one of you merge it, or am I missing something here?

[github-actions]

@yuvald-sweet-security Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

[DavidSpickett]

/cherry-pick 945c494

[llvmbot]

/pull-request #134072