[lldb] Update logic to close inherited file descriptors.

[rdhindsa]

Summary

This patch adds the support to close all inherited fds into the child process by iterating over /proc/self/fd entries. 

Currently, it iterates over all the files, for file descriptors from 3 to _SC_OPEN_MAX, which causes lldb to be slower. In comparison, the number of file descriptors held by the child process are quite small. Hence this patch makes the child process faster.

[rdhindsa]

Ping.

[dblaikie]

Might be worth checking who wrote some of this/nearby code and whether they're still active in the community and see if they can review this - not sure if Pavel is around/has the bandwidth to review this.

[clayborg]

Sorry for the delay. This seems like it should work just fine. Where is this actually used? I thought most clients should be using posix_spawn() as this is the preferred launching mechanism over using fork/exec

[rdhindsa]

fork is being called in LaunchProcess function in this file(ProcessLauncherPosixFork.cpp) itself.

Thank you for the review!

[dblaikie]

In D105732#2942355, @clayborg wrote:

Sorry for the delay. This seems like it should work just fine. Where is this actually used? I thought most clients should be using posix_spawn() as this is the preferred launching mechanism over using fork/exec

Looks like posix_spawn is used in the macosx host, but not in the posix host (where, as @rdhindsa mentioned, fork is used), for whatever that's worth.

[jingham]

Do modern Linux's not have posix_spawn? If it exists that's a better interface, and lets the system handle a lot of the complicated machinations you have to do by hand if you roll it yourself out of fork and exec.

[dblaikie]

In D105732#2942716, @jingham wrote:

Do modern Linux's not have posix_spawn? If it exists that's a better interface, and lets the system handle a lot of the complicated machinations you have to do by hand if you roll it yourself out of fork and exec.

https://man7.org/linux/man-pages/man3/posix_spawn.3.html - looks like it's there. I was reporting on the current implementation/adding some detail on the current state of things.

[jingham]

Sure, thanks for the context!

Actually, the posix_spawn usage is only in the darwin Host code, probably because for posix_spawn to work for debugging
you need a couple of non-standard flags that were added on Darwin for us (the CoreOS folks really wanted us to use posix_spawn).
So you probably can't use it on Linux w/o some kernel support.

Jim

[MaskRay]

Drive-by: std::filesystem is unavailable for GCC<8. The minimum GCC version is 5, so std::filesystem isn't suitable. You may use include/llvm/Support/FileSystem.h

[MaskRay]

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

174

/proc/self/fd is generally unavailable on non-Linux OSes.

[rdhindsa]

Updated it to use llvm's filesystem.

[clayborg]

LGTM. Anyone else have any issues?

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

174

which is why the "else" clause is still intact. If the file fails to open, we do what we did before.

[MaskRay]

LGTM. Some nits

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

148

161

drop parens around fd > 2

164

auto & -> int

168

Cache sysconf(_SC_OPEN_MAX) to avoid repeated calls.

169

one-line simple statements don't need braces. https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

[JDevlieghere]

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

148

Can this be a StringRef?

149

155

[JDevlieghere]

Small nit about the comments, but otherwise LGTM

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

160

nit: Comments should be sentences ending with a period.

167

nit: Same as above.

[rdhindsa]

Updated comments as suggested.

[labath]

BTW, since linux-5.9, there's a close_range(2) syscall. Given that's a pretty recent kernel, it may not be very useful right now, but it sounds like a way to go for the future.

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

 //===-- ProcessLauncherPosixFork.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 "lldb/Host/posix/ProcessLauncherPosixFork.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostProcess.h"
 #include "lldb/Host/Pipe.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include <climits>
 #include <sys/ptrace.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
 #include <sstream>
 #include <csignal>
if (info.GetFlags().Test(eLaunchFlagDebug)) {
     // Do not inherit setgid powers.
     if (setgid(getgid()) != 0)
       ExitWithError(error_fd, "setgid");
 
     // HACK:
     // Close everything besides stdin, stdout, and stderr that has no file
     // action to avoid leaking. Only do this when debugging, as elsewhere we
     // actually rely on passing open descriptors to child processes.
-    for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++fd)
+
+    const llvm::StringRef proc_fd_path = "/proc/self/fd";
+    std::error_code ec;
+    bool result;
+    ec = llvm::sys::fs::is_directory(proc_fd_path, result);
+    if (result) {
+      std::vector<int> files_to_close;
+      // Directory iterator doesn't ensure any sequence.
+      for (llvm::sys::fs::directory_iterator iter(proc_fd_path, ec), file_end;
+           iter != file_end && !ec; iter.increment(ec)) {
+        int fd = std::stoi(iter->path().substr(proc_fd_path.size() + 1));
+
+        // Don't close first three entries since they are
+        // stdin/stdout/stderr
+        if (fd > 2 && !info.GetFileActionForFD(fd) && fd != error_fd)
+          files_to_close.push_back(fd);
+      }
+      for (int file_to_close : files_to_close)
+        close(file_to_close);
+    } else {
+      // /proc/self/fd didn't work - trying the slow way instead
+      int max_fd = sysconf(_SC_OPEN_MAX);
+      for (int fd = 3; fd < max_fd; ++fd)
         if (!info.GetFileActionForFD(fd) && fd != error_fd)
           close(fd);
+    }
 
     // Start tracing this child that is about to exec.
     if (ptrace(PT_TRACE_ME, 0, nullptr, 0) == -1)
       ExitWithError(error_fd, "ptrace");
   }
 
   // Execute.  We should never return...
   execve(argv[0], const_cast<char *const *>(argv), envp);
 
 #if defined(__linux__)