[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Rumeet Dhindsa via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd9c5613e856c: Update logic to close inherited file 
descriptors. (authored by rdhindsa).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include 
 #include 
@@ -143,9 +144,32 @@
 // 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)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(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 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 and
+// 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 {
+  // Since /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)


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include 
 #include 
@@ -143,9 +144,32 @@
 // 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)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(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 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 and
+// 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 {
+  // Since /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)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Rumeet Dhindsa via Phabricator via lldb-commits]

rdhindsa updated this revision to Diff 367311.
rdhindsa added a comment.

Updated comments as suggested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include 
 #include 
@@ -143,9 +144,32 @@
 // 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)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(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 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 and
+// 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 {
+  // Since /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)


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include 
 #include 
@@ -143,9 +144,32 @@
 // 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)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(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 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 and
+// 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 {
+  // Since /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)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Small nit about the comments, but otherwise LGTM




Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:160
+// Don't close first three entries since they are
+// stdin/stdout/stderr
+if (fd > 2 && !info.GetFileActionForFD(fd) && fd != error_fd)

nit: Comments should be sentences ending with a period.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:167
+} else {
+  // /proc/self/fd didn't work - trying the slow way instead
+  int max_fd = sysconf(_SC_OPEN_MAX);

nit: Same as above. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Rumeet Dhindsa via Phabricator via lldb-commits]

rdhindsa updated this revision to Diff 367286.
rdhindsa marked 7 inline comments as done.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include 
 #include 
@@ -143,9 +144,32 @@
 // 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)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(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 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)


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include 
 #include 
@@ -143,9 +144,32 @@
 // 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)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(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 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)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148
+
+std::string proc_fd_path = "/proc/self/fd";
+std::error_code EC;

MaskRay wrote:
> 
Can this be a StringRef?



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:149
+std::string proc_fd_path = "/proc/self/fd";
+std::error_code EC;
+bool result;





Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:155
+  // Directory iterator doesn't ensure any sequence.
+  for (llvm::sys::fs::directory_iterator iter(proc_fd_path, EC), FileEnd;
+   iter != FileEnd && !EC; iter.increment(EC)) {




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148
+
+std::string proc_fd_path = "/proc/self/fd";
+std::error_code EC;




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added a comment.

LGTM. Some nits




Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:161
+// stdin/stdout/stderr
+if ((fd > 2) && !info.GetFileActionForFD(fd) && fd != error_fd)
+  files_to_close.push_back(fd);

drop parens around `fd > 2`



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:164
+  }
+  for (auto _to_close : files_to_close)
+close(file_to_close);

`auto &` -> int



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:168
+  // /proc/self/fd didn't work - trying the slow way instead
+  for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++fd)
+if (!info.GetFileActionForFD(fd) && fd != error_fd) {

Cache `sysconf(_SC_OPEN_MAX)` to avoid repeated calls.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:169
+  for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++fd)
+if (!info.GetFileActionForFD(fd) && fd != error_fd) {
+  close(fd);

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.
Herald added a subscriber: JDevlieghere.

LGTM. Anyone else have any issues?




Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148
 
+std::string proc_fd_path = "/proc/self/fd";
+std::filesystem::path fp(proc_fd_path);

MaskRay wrote:
> /proc/self/fd is generally unavailable on non-Linux OSes.
which is why the "else" clause is still intact. If the file fails to open, we 
do what we did before.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Rumeet Dhindsa via Phabricator via lldb-commits]

rdhindsa updated this revision to Diff 367067.
rdhindsa added a comment.
Herald added a project: LLDB.

Updated it to use llvm's filesystem.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include 
 #include 
@@ -143,9 +144,32 @@
 // 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)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(fd);
+
+std::string 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 files_to_close;
+  // Directory iterator doesn't ensure any sequence.
+  for (llvm::sys::fs::directory_iterator iter(proc_fd_path, EC), FileEnd;
+   iter != FileEnd && !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 (auto _to_close : files_to_close)
+close(file_to_close);
+} else {
+  // /proc/self/fd didn't work - trying the slow way instead
+  for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++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)


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include 
 #include 
@@ -143,9 +144,32 @@
 // 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)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(fd);
+
+std::string 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 files_to_close;
+  // Directory iterator doesn't ensure any sequence.
+  for (llvm::sys::fs::directory_iterator iter(proc_fd_path, EC), FileEnd;
+   iter != FileEnd && !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 (auto _to_close : files_to_close)
+close(file_to_close);
+} else {
+  // /proc/self/fd didn't work - trying the slow way instead
+  for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++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)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148
 
+std::string proc_fd_path = "/proc/self/fd";
+std::filesystem::path fp(proc_fd_path);

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

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`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Jim Ingham via lldb-commits]

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


> On Aug 12, 2021, at 3:56 PM, David Blaikie via Phabricator 
>  wrote:
> 
> dblaikie added a comment.
> 
> 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.
> 
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D105732/new/
> 
> https://reviews.llvm.org/D105732
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Rumeet Dhindsa via Phabricator via lldb-commits]

rdhindsa added a comment.

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

Thank you for the review!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Rumeet Dhindsa via Phabricator via lldb-commits]

rdhindsa added a comment.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Rumeet Dhindsa via Phabricator via lldb-commits]

rdhindsa updated this revision to Diff 358429.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105732/new/

https://reviews.llvm.org/D105732

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -16,6 +16,7 @@
 #include "llvm/Support/Errno.h"
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -143,10 +144,30 @@
 // 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)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(fd);
 
+std::string proc_fd_path = "/proc/self/fd";
+std::filesystem::path fp(proc_fd_path);
+if (std::filesystem::is_directory(fp)) {
+  std::vector files_to_close;
+  // Directory iterator doesn't ensure any sequence.
+  for (auto  : std::filesystem::directory_iterator(proc_fd_path)) {
+int fd =
+std::stoi(entry.path().string().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 (auto _to_close : files_to_close)
+close(file_to_close);
+} else {
+  // /proc/self/fd didn't work - trying the slow way instead
+  for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++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");


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -16,6 +16,7 @@
 #include "llvm/Support/Errno.h"
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -143,10 +144,30 @@
 // 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)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(fd);
 
+std::string proc_fd_path = "/proc/self/fd";
+std::filesystem::path fp(proc_fd_path);
+if (std::filesystem::is_directory(fp)) {
+  std::vector files_to_close;
+  // Directory iterator doesn't ensure any sequence.
+  for (auto  : std::filesystem::directory_iterator(proc_fd_path)) {
+int fd =
+std::stoi(entry.path().string().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 (auto _to_close : files_to_close)
+close(file_to_close);
+} else {
+  // /proc/self/fd didn't work - trying the slow way instead
+  for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++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");
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits