[compiler-rt] r368947 - [sanitizer_common] Replace forkpty with posix_spawn on Darwin
Julian Lettner via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 14 17:18:55 PDT 2019
Author: yln
Date: Wed Aug 14 17:18:55 2019
New Revision: 368947
URL: http://llvm.org/viewvc/llvm-project?rev=368947&view=rev
Log:
[sanitizer_common] Replace forkpty with posix_spawn on Darwin
On Darwin, we currently use forkpty to communicate with the "atos"
symbolizer. There are several problems that fork[pty] has, e.g. that
after fork, interceptors are still active and this sometimes causes
crashes or hangs. This is especially problematic for TSan, which uses
interceptors for OS-provided locks and mutexes, and even Libc functions
use those.
This patch replaces forkpty with posix_spawn on Darwin. Since
posix_spawn doesn't fork (at least on Darwin), the interceptors are not
a problem. Another benefit is that we'll handle post-fork failures (e.g.
sandbox disallows "exec") gracefully now.
Related revisions and previous attempts that were blocked by or had to
be revered due to test failures:
https://reviews.llvm.org/D48451
https://reviews.llvm.org/D40032
Reviewed By: kubamracek
Differential Revision: https://reviews.llvm.org/D65253
Modified:
compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cpp
compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h
compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h
compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp
compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
compiler-rt/trunk/test/asan/TestCases/Darwin/dladdr-demangling.cpp
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cpp
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cpp?rev=368947&r1=368946&r2=368947&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cpp (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cpp Wed Aug 14 17:18:55 2019
@@ -64,7 +64,9 @@ extern "C" {
#include <pthread.h>
#include <sched.h>
#include <signal.h>
+#include <spawn.h>
#include <stdlib.h>
+#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/resource.h>
#include <sys/stat.h>
@@ -239,27 +241,84 @@ int internal_sysctlbyname(const char *sn
(size_t)newlen);
}
-int internal_forkpty(int *aparent) {
- int parent, worker;
- if (openpty(&parent, &worker, nullptr, nullptr, nullptr) == -1) return -1;
- int pid = internal_fork();
- if (pid == -1) {
- close(parent);
- close(worker);
- return -1;
+static fd_t internal_spawn_impl(const char *argv[], pid_t *pid) {
+ fd_t master_fd = kInvalidFd;
+ fd_t slave_fd = kInvalidFd;
+
+ auto fd_closer = at_scope_exit([&] {
+ internal_close(master_fd);
+ internal_close(slave_fd);
+ });
+
+ // 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.
+ master_fd = posix_openpt(O_RDWR);
+ if (master_fd == kInvalidFd) return kInvalidFd;
+
+ int res = grantpt(master_fd) || unlockpt(master_fd);
+ if (res != 0) return kInvalidFd;
+
+ // Use TIOCPTYGNAME instead of ptsname() to avoid threading problems.
+ char slave_pty_name[128];
+ res = ioctl(master_fd, TIOCPTYGNAME, slave_pty_name);
+ if (res == -1) return kInvalidFd;
+
+ slave_fd = internal_open(slave_pty_name, O_RDWR);
+ if (slave_fd == kInvalidFd) return kInvalidFd;
+
+ posix_spawn_file_actions_t acts;
+ res = posix_spawn_file_actions_init(&acts);
+ if (res != 0) return kInvalidFd;
+
+ auto fa_cleanup = at_scope_exit([&] {
+ posix_spawn_file_actions_destroy(&acts);
+ });
+
+ char **env = GetEnviron();
+ res = posix_spawn_file_actions_adddup2(&acts, slave_fd, STDIN_FILENO) ||
+ posix_spawn_file_actions_adddup2(&acts, slave_fd, STDOUT_FILENO) ||
+ posix_spawn_file_actions_addclose(&acts, slave_fd) ||
+ posix_spawn_file_actions_addclose(&acts, master_fd) ||
+ posix_spawn(pid, argv[0], &acts, NULL, const_cast<char **>(argv), env);
+ if (res != 0) return kInvalidFd;
+
+ // Disable echo in the new terminal, disable CR.
+ struct termios termflags;
+ tcgetattr(master_fd, &termflags);
+ termflags.c_oflag &= ~ONLCR;
+ termflags.c_lflag &= ~ECHO;
+ tcsetattr(master_fd, TCSANOW, &termflags);
+
+ // On success, do not close master_fd on scope exit.
+ fd_t fd = master_fd;
+ master_fd = kInvalidFd;
+
+ return fd;
+}
+
+fd_t internal_spawn(const char *argv[], 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;
}
- if (pid == 0) {
- close(parent);
- if (login_tty(worker) != 0) {
- // We already forked, there's not much we can do. Let's quit.
- Report("login_tty failed (errno %d)\n", errno);
- internal__exit(1);
- }
- } else {
- *aparent = parent;
- close(worker);
+
+ fd_t fd = internal_spawn_impl(argv, pid);
+
+ for (; count > 0; count--) {
+ internal_close(low_fds[count]);
}
- return pid;
+
+ return fd;
}
uptr internal_rename(const char *oldpath, const char *newpath) {
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h?rev=368947&r1=368946&r2=368947&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h Wed Aug 14 17:18:55 2019
@@ -63,7 +63,7 @@ uptr internal_ptrace(int request, int pi
uptr internal_waitpid(int pid, int *status, int options);
int internal_fork();
-int internal_forkpty(int *amaster);
+fd_t internal_spawn(const char *argv[], pid_t *pid);
int internal_sysctl(const int *name, unsigned int namelen, void *oldp,
uptr *oldlenp, const void *newp, uptr newlen);
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h?rev=368947&r1=368946&r2=368947&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h Wed Aug 14 17:18:55 2019
@@ -76,7 +76,7 @@ class SymbolizerTool {
// SymbolizerProcess may not be used from two threads simultaneously.
class SymbolizerProcess {
public:
- explicit SymbolizerProcess(const char *path, bool use_forkpty = false);
+ explicit SymbolizerProcess(const char *path, bool use_posix_spawn = false);
const char *SendCommand(const char *command);
protected:
@@ -114,7 +114,7 @@ class SymbolizerProcess {
uptr times_restarted_;
bool failed_to_start_;
bool reported_invalid_path_;
- bool use_forkpty_;
+ bool use_posix_spawn_;
};
class LLVMSymbolizerProcess;
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp?rev=368947&r1=368946&r2=368947&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp Wed Aug 14 17:18:55 2019
@@ -452,14 +452,14 @@ const char *LLVMSymbolizer::FormatAndSen
return symbolizer_process_->SendCommand(buffer_);
}
-SymbolizerProcess::SymbolizerProcess(const char *path, bool use_forkpty)
+SymbolizerProcess::SymbolizerProcess(const char *path, bool use_posix_spawn)
: path_(path),
input_fd_(kInvalidFd),
output_fd_(kInvalidFd),
times_restarted_(0),
failed_to_start_(false),
reported_invalid_path_(false),
- use_forkpty_(use_forkpty) {
+ use_posix_spawn_(use_posix_spawn) {
CHECK(path_);
CHECK_NE(path_[0], '\0');
}
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp?rev=368947&r1=368946&r2=368947&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp Wed Aug 14 17:18:55 2019
@@ -50,7 +50,7 @@ bool DlAddrSymbolizer::SymbolizeData(upt
class AtosSymbolizerProcess : public SymbolizerProcess {
public:
explicit AtosSymbolizerProcess(const char *path, pid_t parent_pid)
- : SymbolizerProcess(path, /*use_forkpty*/ true) {
+ : SymbolizerProcess(path, /*use_posix_spawn*/ true) {
// Put the string command line argument in the object so that it outlives
// the call to GetArgV.
internal_snprintf(pid_str_, sizeof(pid_str_), "%d", parent_pid);
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp?rev=368947&r1=368946&r2=368947&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp Wed Aug 14 17:18:55 2019
@@ -33,10 +33,6 @@
#include <sys/wait.h>
#include <unistd.h>
-#if SANITIZER_MAC
-#include <util.h> // for forkpty()
-#endif // SANITIZER_MAC
-
// C++ demangling function, as required by Itanium C++ ABI. This is weak,
// because we do not require a C++ ABI library to be linked to a program
// using sanitizers; if it's not present, we'll just use the mangled name.
@@ -151,80 +147,32 @@ bool SymbolizerProcess::StartSymbolizerS
return false;
}
- int pid = -1;
-
- int infd[2];
- internal_memset(&infd, 0, sizeof(infd));
- int outfd[2];
- internal_memset(&outfd, 0, sizeof(outfd));
- if (!CreateTwoHighNumberedPipes(infd, outfd)) {
- Report("WARNING: Can't create a socket pair to start "
- "external symbolizer (errno: %d)\n", errno);
- return false;
- }
+ const char *argv[kArgVMax];
+ GetArgV(path_, argv);
+ pid_t pid;
- if (use_forkpty_) {
+ if (use_posix_spawn_) {
#if SANITIZER_MAC
- fd_t fd = kInvalidFd;
-
- // forkpty redirects stdout and stderr into a single stream, so we would
- // receive error messages as standard replies. To avoid that, let's dup
- // stderr and restore it in the child.
- int saved_stderr = dup(STDERR_FILENO);
- CHECK_GE(saved_stderr, 0);
-
- // We only need one pipe, for stdin of the child.
- close(outfd[0]);
- close(outfd[1]);
-
- // Use forkpty to disable buffering in the new terminal.
- pid = internal_forkpty(&fd);
- if (pid == -1) {
- // forkpty() failed.
- Report("WARNING: failed to fork external symbolizer (errno: %d)\n",
+ fd_t fd = internal_spawn(argv, &pid);
+ if (fd == kInvalidFd) {
+ Report("WARNING: failed to spawn external symbolizer (errno: %d)\n",
errno);
return false;
- } else if (pid == 0) {
- // Child subprocess.
-
- // infd[0] is the child's reading end.
- close(infd[1]);
-
- // Set up stdin to read from the pipe.
- CHECK_GE(dup2(infd[0], STDIN_FILENO), 0);
- close(infd[0]);
-
- // Restore stderr.
- CHECK_GE(dup2(saved_stderr, STDERR_FILENO), 0);
- close(saved_stderr);
-
- const char *argv[kArgVMax];
- GetArgV(path_, argv);
- execv(path_, const_cast<char **>(&argv[0]));
- internal__exit(1);
}
- // Input for the child, infd[1] is our writing end.
- output_fd_ = infd[1];
- close(infd[0]);
-
- // Continue execution in parent process.
input_fd_ = fd;
-
- close(saved_stderr);
-
- // Disable echo in the new terminal, disable CR.
- struct termios termflags;
- tcgetattr(fd, &termflags);
- termflags.c_oflag &= ~ONLCR;
- termflags.c_lflag &= ~ECHO;
- tcsetattr(fd, TCSANOW, &termflags);
+ output_fd_ = fd;
#else // SANITIZER_MAC
UNIMPLEMENTED();
#endif // SANITIZER_MAC
} else {
- const char *argv[kArgVMax];
- GetArgV(path_, argv);
+ 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, /* stdin */ outfd[0],
/* stdout */ infd[1]);
if (pid < 0) {
Modified: compiler-rt/trunk/test/asan/TestCases/Darwin/dladdr-demangling.cpp
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/asan/TestCases/Darwin/dladdr-demangling.cpp?rev=368947&r1=368946&r2=368947&view=diff
==============================================================================
--- compiler-rt/trunk/test/asan/TestCases/Darwin/dladdr-demangling.cpp (original)
+++ compiler-rt/trunk/test/asan/TestCases/Darwin/dladdr-demangling.cpp Wed Aug 14 17:18:55 2019
@@ -19,7 +19,7 @@ class MyClass {
// CHECK-DLADDR: Using dladdr symbolizer
// CHECK: {{.*ERROR: AddressSanitizer: heap-use-after-free on address}}
// CHECK: {{READ of size 1 at 0x.* thread T0}}
- // CHECK-DLADDR: failed to fork
+ // CHECK-DLADDR: failed to spawn external symbolizer
// CHECK: {{ #0 0x.* in MyClass::my_function\(int\)}}
// CHECK: {{freed by thread T0 here:}}
// CHECK: {{ #0 0x.* in wrap_free}}
More information about the llvm-commits
mailing list