[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

llvm-svn: 368947

Author: yln

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

@@ -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 *sname, void *oldp, uptr *oldlenp,
                       (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) {

compiler-rt/lib/sanitizer_common/sanitizer_posix.h

@@ -63,7 +63,7 @@ uptr internal_ptrace(int request, int pid, void *addr, void *data);
 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);

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h

@@ -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;

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

@@ -452,14 +452,14 @@ const char *LLVMSymbolizer::FormatAndSendCommand(const char *command_prefix,
   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');
 }

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

@@ -50,7 +50,7 @@ bool DlAddrSymbolizer::SymbolizeData(uptr addr, DataInfo *datainfo) {
 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);

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

@@ -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::StartSymbolizerSubprocess() {
     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) {

compiler-rt/test/asan/TestCases/Darwin/dladdr-demangling.cpp

@@ -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}}