From f1678fc7c2a1612c5964acfb7a3b259062715e9b Mon Sep 17 00:00:00 2001 From: Andrew Haberlandt Date: Fri, 5 Dec 2025 00:02:25 -0800 Subject: [PATCH 1/4] Replace pty with pipe in macOS posix_spawn path --- .../lib/sanitizer_common/sanitizer_mac.cpp | 86 ++++--------------- .../lib/sanitizer_common/sanitizer_posix.h | 2 +- .../sanitizer_symbolizer_posix_libcdep.cpp | 33 ++++--- 3 files changed, 34 insertions(+), 87 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp index a6f757173728b..9f01499f29b21 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp @@ -281,53 +281,36 @@ int internal_sysctlbyname(const char *sname, void *oldp, uptr *oldlenp, (size_t)newlen); } -static fd_t internal_spawn_impl(const char *argv[], const char *envp[], - pid_t *pid) { - fd_t primary_fd = kInvalidFd; - fd_t secondary_fd = kInvalidFd; +bool internal_spawn(const char *argv[], const char *envp[], + pid_t *pid, fd_t fd_stdin, fd_t fd_stdout) { + // NOTE: Caller ensures that fd_stdin and fd_stdout are not 0, 1, or 2, since this can + // break communication. + int res; auto fd_closer = at_scope_exit([&] { - internal_close(primary_fd); - internal_close(secondary_fd); + internal_close(fd_stdin); + internal_close(fd_stdout); }); - // We need a new pseudoterminal to avoid buffering problems. The 'atos' tool - // in particular detects when it's talking to a pipe and forgets to flush the - // output stream after sending a response. - primary_fd = posix_openpt(O_RDWR); - if (primary_fd == kInvalidFd) - return kInvalidFd; - - int res = grantpt(primary_fd) || unlockpt(primary_fd); - if (res != 0) return kInvalidFd; - - // Use TIOCPTYGNAME instead of ptsname() to avoid threading problems. - char secondary_pty_name[128]; - res = ioctl(primary_fd, TIOCPTYGNAME, secondary_pty_name); - if (res == -1) return kInvalidFd; - - secondary_fd = internal_open(secondary_pty_name, O_RDWR); - if (secondary_fd == kInvalidFd) - return kInvalidFd; - // File descriptor actions posix_spawn_file_actions_t acts; res = posix_spawn_file_actions_init(&acts); - if (res != 0) return kInvalidFd; + if (res != 0) return false; auto acts_cleanup = at_scope_exit([&] { posix_spawn_file_actions_destroy(&acts); }); - res = posix_spawn_file_actions_adddup2(&acts, secondary_fd, STDIN_FILENO) || - posix_spawn_file_actions_adddup2(&acts, secondary_fd, STDOUT_FILENO) || - posix_spawn_file_actions_addclose(&acts, secondary_fd); - if (res != 0) return kInvalidFd; + res = posix_spawn_file_actions_adddup2(&acts, fd_stdin, STDIN_FILENO) || + posix_spawn_file_actions_adddup2(&acts, fd_stdout, STDOUT_FILENO) || + posix_spawn_file_actions_addclose(&acts, fd_stdin) || + posix_spawn_file_actions_addclose(&acts, fd_stdout); + if (res != 0) return false; // Spawn attributes posix_spawnattr_t attrs; res = posix_spawnattr_init(&attrs); - if (res != 0) return kInvalidFd; + if (res != 0) return false; auto attrs_cleanup = at_scope_exit([&] { posix_spawnattr_destroy(&attrs); @@ -336,50 +319,15 @@ static fd_t internal_spawn_impl(const char *argv[], const char *envp[], // In the spawned process, close all file descriptors that are not explicitly // described by the file actions object. This is Darwin-specific extension. res = posix_spawnattr_setflags(&attrs, POSIX_SPAWN_CLOEXEC_DEFAULT); - if (res != 0) return kInvalidFd; + if (res != 0) return false; // posix_spawn char **argv_casted = const_cast(argv); char **envp_casted = const_cast(envp); res = posix_spawn(pid, argv[0], &acts, &attrs, argv_casted, envp_casted); - if (res != 0) return kInvalidFd; - - // Disable echo in the new terminal, disable CR. - struct termios termflags; - tcgetattr(primary_fd, &termflags); - termflags.c_oflag &= ~ONLCR; - termflags.c_lflag &= ~ECHO; - tcsetattr(primary_fd, TCSANOW, &termflags); - - // On success, do not close primary_fd on scope exit. - fd_t fd = primary_fd; - primary_fd = kInvalidFd; - - return fd; -} - -fd_t internal_spawn(const char *argv[], const char *envp[], pid_t *pid) { - // The client program may close its stdin and/or stdout and/or stderr thus - // allowing open/posix_openpt to reuse file descriptors 0, 1 or 2. In this - // case the communication is broken if either the parent or the child tries to - // close or duplicate these descriptors. We temporarily reserve these - // descriptors here to prevent this. - fd_t low_fds[3]; - size_t count = 0; - - for (; count < 3; count++) { - low_fds[count] = posix_openpt(O_RDWR); - if (low_fds[count] >= STDERR_FILENO) - break; - } - - fd_t fd = internal_spawn_impl(argv, envp, pid); + if (res != 0) return false; - for (; count > 0; count--) { - internal_close(low_fds[count]); - } - - return fd; + return true; } uptr internal_rename(const char *oldpath, const char *newpath) { diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_posix.h b/compiler-rt/lib/sanitizer_common/sanitizer_posix.h index b5491c540dc08..259388da3747e 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_posix.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_posix.h @@ -67,7 +67,7 @@ uptr internal_ptrace(int request, int pid, void *addr, void *data); uptr internal_waitpid(int pid, int *status, int options); int internal_fork(); -fd_t internal_spawn(const char *argv[], const char *envp[], pid_t *pid); +bool internal_spawn(const char *argv[], const char *envp[], pid_t *pid, fd_t stdin, fd_t stdout); int internal_sysctl(const int *name, unsigned int namelen, void *oldp, uptr *oldlenp, const void *newp, uptr newlen); diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp index 7eb0c9756d64a..d8506a506141f 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp @@ -156,30 +156,29 @@ bool SymbolizerProcess::StartSymbolizerSubprocess() { Printf("\n"); } + fd_t infd[2] = {}, outfd[2] = {}; + if (!CreateTwoHighNumberedPipes(infd, outfd)) { + Report( + "WARNING: Can't create a socket pair to start " + "external symbolizer (errno: %d)\n", + errno); + return false; + } + if (use_posix_spawn_) { # if SANITIZER_APPLE - fd_t fd = internal_spawn(argv, const_cast(GetEnvP()), &pid); - if (fd == kInvalidFd) { + bool success = internal_spawn(argv, const_cast(GetEnvP()), &pid, outfd[0], infd[1]); + if (!success) { Report("WARNING: failed to spawn external symbolizer (errno: %d)\n", errno); + internal_close(infd[0]); + internal_close(outfd[1]); return false; } - - input_fd_ = fd; - output_fd_ = fd; # else // SANITIZER_APPLE UNIMPLEMENTED(); # endif // SANITIZER_APPLE } else { - fd_t infd[2] = {}, outfd[2] = {}; - if (!CreateTwoHighNumberedPipes(infd, outfd)) { - Report( - "WARNING: Can't create a socket pair to start " - "external symbolizer (errno: %d)\n", - errno); - return false; - } - pid = StartSubprocess(path_, argv, GetEnvP(), /* stdin */ outfd[0], /* stdout */ infd[1]); if (pid < 0) { @@ -187,11 +186,11 @@ bool SymbolizerProcess::StartSymbolizerSubprocess() { internal_close(outfd[1]); return false; } - - input_fd_ = infd[0]; - output_fd_ = outfd[1]; } + input_fd_ = infd[0]; + output_fd_ = outfd[1]; + CHECK_GT(pid, 0); // Check that symbolizer subprocess started successfully. From 1dd09afc138567b0a66056e7c5fd8c80c892bb88 Mon Sep 17 00:00:00 2001 From: Andrew Haberlandt Date: Fri, 5 Dec 2025 00:03:02 -0800 Subject: [PATCH 2/4] SymbolizerProcess should hold on to the read end of the stdin pipe so we don't get SIGPIPE --- compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp | 5 ++++- .../lib/sanitizer_common/sanitizer_symbolizer_internal.h | 6 +++++- .../lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp | 8 ++++++++ .../sanitizer_symbolizer_posix_libcdep.cpp | 3 +++ 4 files changed, 20 insertions(+), 2 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp index 9f01499f29b21..3810b94fe5fee 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp @@ -285,10 +285,13 @@ bool internal_spawn(const char *argv[], const char *envp[], pid_t *pid, fd_t fd_stdin, fd_t fd_stdout) { // NOTE: Caller ensures that fd_stdin and fd_stdout are not 0, 1, or 2, since this can // break communication. + // + // NOTE: Caller is responsible for closing fd_stdin after the process has died. int res; auto fd_closer = at_scope_exit([&] { - internal_close(fd_stdin); + // NOTE: We intentionally do not close fd_stdin since this can + // cause us to receive a fatal SIGPIPE if the process dies. internal_close(fd_stdout); }); diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h index 2345aee985541..6442a2980bf2f 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h @@ -83,7 +83,7 @@ class SymbolizerProcess { const char *SendCommand(const char *command); protected: - ~SymbolizerProcess() {} + ~SymbolizerProcess(); /// The maximum number of arguments required to invoke a tool process. static const unsigned kArgVMax = 16; @@ -114,6 +114,10 @@ class SymbolizerProcess { fd_t input_fd_; fd_t output_fd_; + // We hold on to the child's stdin fd (the read end of the pipe) + // so that when we write to it, we don't get a SIGPIPE + fd_t child_stdin_fd_; + InternalMmapVector buffer_; static const uptr kMaxTimesRestarted = 5; diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp index 565701c85d978..eaedb8679bc88 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp @@ -480,6 +480,7 @@ SymbolizerProcess::SymbolizerProcess(const char *path, bool use_posix_spawn) : path_(path), input_fd_(kInvalidFd), output_fd_(kInvalidFd), + child_stdin_fd_(kInvalidFd), times_restarted_(0), failed_to_start_(false), reported_invalid_path_(false), @@ -488,6 +489,11 @@ SymbolizerProcess::SymbolizerProcess(const char *path, bool use_posix_spawn) CHECK_NE(path_[0], '\0'); } +SymbolizerProcess::~SymbolizerProcess() { + if (child_stdin_fd_ != kInvalidFd) + CloseFile(child_stdin_fd_); +} + static bool IsSameModule(const char *path) { if (const char *ProcessName = GetProcessName()) { if (const char *SymbolizerName = StripModuleName(path)) { @@ -533,6 +539,8 @@ bool SymbolizerProcess::Restart() { CloseFile(input_fd_); if (output_fd_ != kInvalidFd) CloseFile(output_fd_); + if (child_stdin_fd_ != kInvalidFd) + CloseFile(output_fd_); return StartSymbolizerSubprocess(); } diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp index d8506a506141f..44cb1ae0ca4f2 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp @@ -191,6 +191,9 @@ bool SymbolizerProcess::StartSymbolizerSubprocess() { input_fd_ = infd[0]; output_fd_ = outfd[1]; + // We intentionally hold on to the read-end so that we don't get a SIGPIPE + child_stdin_fd_ = outfd[0]; + CHECK_GT(pid, 0); // Check that symbolizer subprocess started successfully. From 0bec80fdef6b40a0ed0e51a964edb5fba9f0fce5 Mon Sep 17 00:00:00 2001 From: Andrew Haberlandt Date: Fri, 5 Dec 2025 00:41:31 -0800 Subject: [PATCH 3/4] clang-format --- .../lib/sanitizer_common/sanitizer_mac.cpp | 26 ++++++++++++------- .../lib/sanitizer_common/sanitizer_posix.h | 3 ++- .../sanitizer_symbolizer_libcdep.cpp | 2 +- .../sanitizer_symbolizer_posix_libcdep.cpp | 3 ++- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp index 3810b94fe5fee..3f8de8dd064a5 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp @@ -281,12 +281,13 @@ int internal_sysctlbyname(const char *sname, void *oldp, uptr *oldlenp, (size_t)newlen); } -bool internal_spawn(const char *argv[], const char *envp[], - pid_t *pid, fd_t fd_stdin, fd_t fd_stdout) { - // NOTE: Caller ensures that fd_stdin and fd_stdout are not 0, 1, or 2, since this can - // break communication. +bool internal_spawn(const char* argv[], const char* envp[], pid_t* pid, + fd_t fd_stdin, fd_t fd_stdout) { + // NOTE: Caller ensures that fd_stdin and fd_stdout are not 0, 1, or 2, since + // this can break communication. // - // NOTE: Caller is responsible for closing fd_stdin after the process has died. + // NOTE: Caller is responsible for closing fd_stdin after the process has + // died. int res; auto fd_closer = at_scope_exit([&] { @@ -298,7 +299,8 @@ bool internal_spawn(const char *argv[], const char *envp[], // File descriptor actions posix_spawn_file_actions_t acts; res = posix_spawn_file_actions_init(&acts); - if (res != 0) return false; + if (res != 0) + return false; auto acts_cleanup = at_scope_exit([&] { posix_spawn_file_actions_destroy(&acts); @@ -308,12 +310,14 @@ bool internal_spawn(const char *argv[], const char *envp[], posix_spawn_file_actions_adddup2(&acts, fd_stdout, STDOUT_FILENO) || posix_spawn_file_actions_addclose(&acts, fd_stdin) || posix_spawn_file_actions_addclose(&acts, fd_stdout); - if (res != 0) return false; + if (res != 0) + return false; // Spawn attributes posix_spawnattr_t attrs; res = posix_spawnattr_init(&attrs); - if (res != 0) return false; + if (res != 0) + return false; auto attrs_cleanup = at_scope_exit([&] { posix_spawnattr_destroy(&attrs); @@ -322,13 +326,15 @@ bool internal_spawn(const char *argv[], const char *envp[], // In the spawned process, close all file descriptors that are not explicitly // described by the file actions object. This is Darwin-specific extension. res = posix_spawnattr_setflags(&attrs, POSIX_SPAWN_CLOEXEC_DEFAULT); - if (res != 0) return false; + if (res != 0) + return false; // posix_spawn char **argv_casted = const_cast(argv); char **envp_casted = const_cast(envp); res = posix_spawn(pid, argv[0], &acts, &attrs, argv_casted, envp_casted); - if (res != 0) return false; + if (res != 0) + return false; return true; } diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_posix.h b/compiler-rt/lib/sanitizer_common/sanitizer_posix.h index 259388da3747e..063408b8360c1 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_posix.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_posix.h @@ -67,7 +67,8 @@ uptr internal_ptrace(int request, int pid, void *addr, void *data); uptr internal_waitpid(int pid, int *status, int options); int internal_fork(); -bool internal_spawn(const char *argv[], const char *envp[], pid_t *pid, fd_t stdin, fd_t stdout); +bool internal_spawn(const char* argv[], const char* envp[], pid_t* pid, + fd_t stdin, fd_t stdout); int internal_sysctl(const int *name, unsigned int namelen, void *oldp, uptr *oldlenp, const void *newp, uptr newlen); diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp index eaedb8679bc88..8ee0e23e8b575 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp @@ -476,7 +476,7 @@ const char *LLVMSymbolizer::FormatAndSendCommand(const char *command_prefix, return symbolizer_process_->SendCommand(buffer_); } -SymbolizerProcess::SymbolizerProcess(const char *path, bool use_posix_spawn) +SymbolizerProcess::SymbolizerProcess(const char* path, bool use_posix_spawn) : path_(path), input_fd_(kInvalidFd), output_fd_(kInvalidFd), diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp index 44cb1ae0ca4f2..29c73e3e1cac1 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp @@ -167,7 +167,8 @@ bool SymbolizerProcess::StartSymbolizerSubprocess() { if (use_posix_spawn_) { # if SANITIZER_APPLE - bool success = internal_spawn(argv, const_cast(GetEnvP()), &pid, outfd[0], infd[1]); + bool success = internal_spawn(argv, const_cast(GetEnvP()), + &pid, outfd[0], infd[1]); if (!success) { Report("WARNING: failed to spawn external symbolizer (errno: %d)\n", errno); From 2f8a68b924d5cf3d77cc92bca32cb4ebfc3fc4c5 Mon Sep 17 00:00:00 2001 From: Andrew Haberlandt Date: Fri, 5 Dec 2025 15:44:40 -0800 Subject: [PATCH 4/4] Fix typo in fd cleanup --- .../lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp index 8ee0e23e8b575..cc31d3d8056f9 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp @@ -539,8 +539,10 @@ bool SymbolizerProcess::Restart() { CloseFile(input_fd_); if (output_fd_ != kInvalidFd) CloseFile(output_fd_); - if (child_stdin_fd_ != kInvalidFd) - CloseFile(output_fd_); + if (child_stdin_fd_ != kInvalidFd) { + CloseFile(child_stdin_fd_); + child_stdin_fd_ = kInvalidFd; // Don't free in destructor + } return StartSymbolizerSubprocess(); }