llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Pavel Labath (labath) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/126935.diff 3 Files Affected: - (modified) lldb/source/Host/macosx/objcxx/Host.mm (+10-1) - (modified) lldb/source/Host/posix/ProcessLauncherPosixFork.cpp (+9-2) - (modified) lldb/unittests/Host/HostTest.cpp (+40) ``````````diff diff --git a/lldb/source/Host/macosx/objcxx/Host.mm b/lldb/source/Host/macosx/objcxx/Host.mm index bb270f6a44e43..e187bf98188ae 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 8c6d503fc7fe2..698524349e16a 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> @@ -122,8 +123,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 ed1df6de001ea..222de62ab6697 100644 --- a/lldb/unittests/Host/HostTest.cpp +++ b/lldb/unittests/Host/HostTest.cpp @@ -9,8 +9,10 @@ #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/Support/FileSystem.h" #include "llvm/Testing/Support/Error.h" @@ -87,3 +89,41 @@ TEST(Host, LaunchProcessSetsArgv0) { ASSERT_THAT_ERROR(Host::LaunchProcess(info).takeError(), Succeeded()); ASSERT_THAT(exit_status.get_future().get(), 0); } + +#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, (lldb::pipe_t)test_arg.getValue()); + llvm::Expected<size_t> bytes_written = + pipe.Write(test_msg.data(), test_msg.size()); + if (bytes_written && *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((uint64_t)pipe.GetWritePipe())).str()); + info.AppendDuplicateFileAction((uint64_t)pipe.GetWritePipe(), + (uint64_t)pipe.GetWritePipe()); + info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback); + ASSERT_THAT_ERROR(Host::LaunchProcess(info).takeError(), llvm::Succeeded()); + pipe.CloseWriteFileDescriptor(); + + char msg[100]; + llvm::Expected<size_t> bytes_read = + pipe.Read(msg, sizeof(msg), std::chrono::seconds(10)); + ASSERT_THAT_EXPECTED(bytes_read, llvm::Succeeded()); + ASSERT_EQ(llvm::StringRef(msg, *bytes_read), test_msg); +} +#endif `````````` </details> https://github.com/llvm/llvm-project/pull/126935 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
