https://github.com/labath created
https://github.com/llvm/llvm-project/pull/126935
Currently we're creating inheritable (`~FD_CLOEXEC`) file descriptors in the
(few) cases where we need to pass an FD to a subprocess. The problem with these
is that, in a multithreaded application such as lldb, there's essentially no
way to prevent them from being leaked into processes other than the intended
one.
A safer (though still not completely safe) approach is to mark the descriptors
as FD_CLOEXEC and only clear this flag in the subprocess. We currently have
something that almost does that, which is the ability to add a
`DuplicateFileAction` to our `ProcessLaunchInfo` struct (the duplicated file
descriptor will be created with the flag cleared). The problem with *that* is
that this approach is completely incompatible with Windows.
Windows equivalents of file descriptors are `HANDLE`s, but these do not have
user controlled values -- applications are expected to work with whatever
HANDLE values are assigned by the OS. In unix terms, there is no equivalent to
the `dup2` syscall (only `dup`).
To find a way out of this conundrum, and create a miniscule API surface that
works uniformly across platforms, this PR proposes to extend the
`DuplicateFileAction` API to support duplicating a file descriptor onto itself.
Currently, this operation does nothing (it leaves the FD_CLOEXEC flag set),
because that's how `dup2(fd, fd)` behaves, but I think it's not completely
unreasonable to say that this operation should clear the FD_CLOEXEC flag, just
like it would do if one was using different fd values. This would enable us to
pass a windows HANDLE as itself through the ProcessLaunchInfo API.
This PR implements the unix portion of this idea. Macos and non-macos launchers
are updated to clear FD_CLOEXEC flag when duplicating a file descriptor onto
itself, and I've created a test which enables passing a FD_CLOEXEC file
descritor to the subprocess. For the windows portion, please see the follow-up
PR.
>From 75e6dd4f81a9fe4bd303ba4e3b0301b69d47b523 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pa...@labath.sk>
Date: Sun, 20 Oct 2024 02:55:02 +0200
Subject: [PATCH] [lldb/Host] Enable inheriting "non-inheritable" FDs
Currently we're creating inheritable (`~FD_CLOEXEC`) file descriptors in
the (few) cases where we need to pass an FD to a subprocess. The problem
with these is that, in a multithreaded application such as lldb, there's
essentially no way to prevent them from being leaked into processes
other than the intended one.
A safer (though still not completely safe) approach is to mark the
descriptors as FD_CLOEXEC and only clear this flag in the subprocess. We
currently have something that almost does that, which is the ability to
add a `DuplicateFileAction` to our `ProcessLaunchInfo` struct (the
duplicated file descriptor will be created with the flag cleared). The
problem with *that* is that this approach is completely incompatible
with Windows.
Windows equivalents of file descriptors are `HANDLE`s, but these do not
have user controlled values -- applications are expected to work with
whatever HANDLE values are assigned by the OS. In unix terms, there is
no equivalent to the `dup2` syscall (only `dup`).
To find a way out of this conundrum, and create a miniscule API surface
that works uniformly across platforms, this PR proposes to extend the
`DuplicateFileAction` API to support duplicating a file descriptor onto
itself. Currently, this operation does nothing (it leaves the FD_CLOEXEC
flag set), because that's how `dup2(fd, fd)` behaves, but I think it's
not completely unreasonable to say that this operation should clear the
FD_CLOEXEC flag, just like it would do if one was using different fd
values. This would enable us to pass a windows HANDLE as itself through
the ProcessLaunchInfo API.
This PR implements the unix portion of this idea. Macos and non-macos
launchers are updated to clear FD_CLOEXEC flag when duplicating a file
descriptor onto itself, and I've created a test which enables passing a
FD_CLOEXEC file descritor to the subprocess. For the windows portion,
please see the follow-up PR.
---
lldb/source/Host/macosx/objcxx/Host.mm | 11 +++-
.../Host/posix/ProcessLauncherPosixFork.cpp | 11 +++-
lldb/unittests/Host/HostTest.cpp | 55 ++++++++++++++++++-
3 files changed, 73 insertions(+), 4 deletions(-)
diff --git a/lldb/source/Host/macosx/objcxx/Host.mm
b/lldb/source/Host/macosx/objcxx/Host.mm
index 5b3d04a6b587b..38edac45dedbd 100644
--- a/lldb/source/Host/macosx/objcxx/Host.mm
+++ b/lldb/source/Host/macosx/objcxx/Host.mm
@@ -1100,7 +1100,7 @@ static bool AddPosixSpawnFileAction(void *_file_actions,
const FileAction *info,
else if (info->GetActionArgument() == -1)
error = Status::FromErrorString(
"invalid duplicate fd for posix_spawn_file_actions_adddup2(...)");
- else {
+ else if (info->GetFD() != info->GetActionArgument()) {
error =
Status(::posix_spawn_file_actions_adddup2(file_actions,
info->GetFD(),
info->GetActionArgument()),
@@ -1110,6 +1110,15 @@ static bool AddPosixSpawnFileAction(void *_file_actions,
const FileAction *info,
"error: {0}, posix_spawn_file_actions_adddup2 "
"(action={1}, fd={2}, dup_fd={3})",
error, file_actions, info->GetFD(),
info->GetActionArgument());
+ } else {
+ error =
+ Status(::posix_spawn_file_actions_addinherit_np(file_actions,
info->GetFD()),
+ eErrorTypePOSIX);
+ if (error.Fail())
+ LLDB_LOG(log,
+ "error: {0}, posix_spawn_file_actions_addinherit_np "
+ "(action={1}, fd={2})",
+ error, file_actions, info->GetFD());
}
break;
diff --git a/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
b/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
index 3e956290c3055..c3f1896172c41 100644
--- a/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ b/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -17,6 +17,7 @@
#include "llvm/Support/Errno.h"
#include <climits>
+#include <fcntl.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <unistd.h>
@@ -121,8 +122,14 @@ struct ForkLaunchInfo {
ExitWithError(error_fd, "close");
break;
case FileAction::eFileActionDuplicate:
- if (dup2(action.fd, action.arg) == -1)
- ExitWithError(error_fd, "dup2");
+ if (action.fd != action.arg) {
+ if (dup2(action.fd, action.arg) == -1)
+ ExitWithError(error_fd, "dup2");
+ } else {
+ if (fcntl(action.fd, F_SETFD,
+ fcntl(action.fd, F_GETFD) & ~FD_CLOEXEC) == -1)
+ ExitWithError(error_fd, "fcntl");
+ }
break;
case FileAction::eFileActionOpen:
DupDescriptor(error_fd, action.path.c_str(), action.fd, action.arg);
diff --git a/lldb/unittests/Host/HostTest.cpp b/lldb/unittests/Host/HostTest.cpp
index a1d8a3b7f485a..b9ad3e9ae1e41 100644
--- a/lldb/unittests/Host/HostTest.cpp
+++ b/lldb/unittests/Host/HostTest.cpp
@@ -7,12 +7,24 @@
//===----------------------------------------------------------------------===//
#include "lldb/Host/Host.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/Pipe.h"
+#include "lldb/Host/ProcessLaunchInfo.h"
#include "lldb/Utility/ProcessInfo.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Testing/Support/Error.h"
#include "gtest/gtest.h"
+// From TestMain.cpp.
+extern const char *TestMainArgv0;
+
using namespace lldb_private;
using namespace llvm;
+static cl::opt<uint64_t> test_arg("test-arg");
+
TEST(Host, WaitStatusFormat) {
EXPECT_EQ("W01", formatv("{0:g}", WaitStatus{WaitStatus::Exit, 1}).str());
EXPECT_EQ("X02", formatv("{0:g}", WaitStatus{WaitStatus::Signal, 2}).str());
@@ -45,4 +57,45 @@ TEST(Host, ProcessInstanceInfoCumulativeSystemTimeIsValid) {
EXPECT_TRUE(info.CumulativeSystemTimeIsValid());
info.SetCumulativeSystemTime(ProcessInstanceInfo::timespec{1, 0});
EXPECT_TRUE(info.CumulativeSystemTimeIsValid());
-}
\ No newline at end of file
+}
+
+#ifdef LLVM_ON_UNIX
+TEST(Host, LaunchProcessDuplicatesHandle) {
+ static constexpr llvm::StringLiteral test_msg("Hello subprocess!");
+
+ SubsystemRAII<FileSystem> subsystems;
+
+ if (test_arg) {
+ Pipe pipe(LLDB_INVALID_PIPE, test_arg);
+ size_t bytes_written;
+ if (pipe.WriteWithTimeout(test_msg.data(), test_msg.size(),
+ std::chrono::microseconds(0), bytes_written)
+ .Success() &&
+ bytes_written == test_msg.size())
+ exit(0);
+ exit(1);
+ }
+ Pipe pipe;
+
ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).takeError(),
+ llvm::Succeeded());
+ ProcessLaunchInfo info;
+ info.SetExecutableFile(FileSpec(TestMainArgv0),
+ /*add_exe_file_as_first_arg=*/true);
+ info.GetArguments().AppendArgument(
+ "--gtest_filter=Host.LaunchProcessDuplicatesHandle");
+ info.GetArguments().AppendArgument(
+ ("--test-arg=" + llvm::Twine::utohexstr(pipe.GetWritePipe())).str());
+ info.AppendDuplicateFileAction(pipe.GetWritePipe(), pipe.GetWritePipe());
+ info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback);
+ ASSERT_THAT_ERROR(Host::LaunchProcess(info).takeError(), llvm::Succeeded());
+ pipe.CloseWriteFileDescriptor();
+
+ char msg[100];
+ size_t bytes_read;
+ ASSERT_THAT_ERROR(pipe.ReadWithTimeout(msg, sizeof(msg),
+ std::chrono::seconds(10), bytes_read)
+ .takeError(),
+ llvm::Succeeded());
+ ASSERT_EQ(llvm::StringRef(msg, bytes_read), test_msg);
+}
+#endif
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
