[sanitizer_common] Replace forkpty with posix_spawn on Darwin

[yln]

Summary

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

[kubamracek]

Thanks! This looks like a great step in the right direction. An overall question first: Should we use posix_spawn for *all platforms*? IIRC, unifying the launch paths was discussed on mailing lists before and sounds desirable.

[alex]

I had a patch to use posix_spawn on all posix platforms, but hit some issues I didn't quite have the time to debug all the way through. It's a good idea in my view.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cc

53 ↗

(On Diff #211630)

Do we have use_posix_spawn = false on mac anywhere?

[yln]

In D65253#1607640, @kubamracek wrote:

Thanks! This looks like a great step in the right direction. An overall question first: Should we use posix_spawn for *all platforms*? IIRC, unifying the launch paths was discussed on mailing lists before and sounds desirable.

I agree that this should be the end goal. For this patch, I would prefer to keep it Darwin-only to reduce the scope, especially since multiple people already attempted this before.
Also, I wouldn't be able to debug this locally for other platforms. How about doing this as a first step (of course I want to avoid doing anything that would prevent adopting this on other platforms) and going from there?

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cc

53 ↗

(On Diff #211630)

We have LLVMSymbolizerProcess which doesn't specify it and therefore gets the default false.

[kubamracek]

LGTM, even if it's just for Darwin now.

I do think, though, that LLVMSymbolizerProcess should also use posix_spawn *on Darwin*. Can we get that (as a separate change)?

compiler-rt/lib/sanitizer_common/sanitizer_mac.cc

257 ↗

(On Diff #211630)

There's a bunch of different reasons why we can fail to spawn. Should we Report() to log the actual failed API?

[yln]

Fixed failing test closed-fds.cpp: external symbolizer should continue to work
even when client program implements any of std{in,out,err}. Implemented similiar
functionality as CreateTwoHighNumberedPipes for internal_spawn.

[yln]

In D65253#1629737, @kubamracek wrote:

I do think, though, that LLVMSymbolizerProcess should also use posix_spawn *on Darwin*. Can we get that (as a separate change)?

Changed in:
https://github.com/llvm/llvm-project/commit/bb99437f6d45af7bbc88e07b8e4dbc1528802508

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

 #include <mach-o/dyld.h>
 #include <mach/mach.h>
 #include <mach/mach_time.h>
 #include <mach/vm_statistics.h>
 #include <malloc/malloc.h>
 #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>
 #include <sys/sysctl.h>
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
 #include <util.h>

 }
 
 int internal_sysctlbyname(const char *sname, void *oldp, uptr *oldlenp,
                           const void *newp, uptr newlen) {
   return sysctlbyname(sname, oldp, (size_t *)oldlenp, const_cast<void *>(newp),
                       (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;
-  }
-  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);
+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;
+
+  for (count = 0; count < 3; count++) {
+    low_fds[count] = posix_openpt(O_RDWR);
+    if (low_fds[count] >= STDERR_FILENO)
+      break;
+
+  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) {
   return rename(oldpath, newpath);
 }
 
 uptr internal_ftruncate(fd_t fd, uptr size) {
   return ftruncate(fd, size);

compiler-rt/lib/sanitizer_common/sanitizer_posix.h

 #if SANITIZER_NETBSD
 uptr internal_ptrace(int request, int pid, void *addr, int data);
 #else
 uptr internal_ptrace(int request, int pid, void *addr, void *data);
 #endif
 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);
 int internal_sysctlbyname(const char *sname, void *oldp, uptr *oldlenp,
                           const void *newp, uptr newlen);
 
 // These functions call appropriate pthread_ functions directly, bypassing
 // the interceptor. They are weak and may not be present in some tools.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h

public:
   }
 };
 
 // SymbolizerProcess encapsulates communication between the tool and
 // external symbolizer program, running in a different subprocess.
 // 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:
   /// The maximum number of arguments required to invoke a tool process.
   static const unsigned kArgVMax = 6;
 
   // Customizable by subclasses.
   virtual bool StartSymbolizerSubprocess();
private:
   static const uptr kBufferSize = 16 * 1024;
   char buffer_[kBufferSize];
 
   static const uptr kMaxTimesRestarted = 5;
   static const int kSymbolizerStartupTimeMillis = 10;
   uptr times_restarted_;
   bool failed_to_start_;
   bool reported_invalid_path_;
-  bool use_forkpty_;
+  bool use_posix_spawn_;
 };
 
 class LLVMSymbolizerProcess;
 
 // This tool invokes llvm-symbolizer in a subprocess. It should be as portable
 // as the llvm-symbolizer tool is.
 class LLVMSymbolizer : public SymbolizerTool {
  public:

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

if (internal_snprintf(buffer_, kBufferSize, "%s \"%s:%s\" 0x%zx\n",
                           module_offset) >= static_cast<int>(kBufferSize)) {
       Report("WARNING: Command buffer too small");
       return nullptr;
     }
   }
   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');
 }
 
 static bool IsSameModule(const char* path) {
   if (const char* ProcessName = GetProcessName()) {
     if (const char* SymbolizerName = StripModuleName(path)) {
       return !internal_strcmp(ProcessName, SymbolizerName);

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

bool DlAddrSymbolizer::SymbolizeData(uptr addr, DataInfo *datainfo) {
   datainfo->name = internal_strdup(demangled);
   datainfo->start = (uptr)info.dli_saddr;
   return true;
 }
 
 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);
   }
 
  private:
   virtual bool StartSymbolizerSubprocess() override {
     // Configure sandbox before starting atos process.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

 
 #include <dlfcn.h>   // for dlsym()
 #include <errno.h>
 #include <stdint.h>
 #include <stdlib.h>
 #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.
 namespace __cxxabiv1 {
   extern "C" SANITIZER_WEAK_ATTRIBUTE
   char *__cxa_demangle(const char *mangled, char *buffer,
                                   size_t *length, int *status);
 }
bool SymbolizerProcess::StartSymbolizerSubprocess() {
   if (!FileExists(path_)) {
     if (!reported_invalid_path_) {
       Report("WARNING: invalid path to external symbolizer!\n");
       reported_invalid_path_ = true;
     }
     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) {
       internal_close(infd[0]);
       internal_close(outfd[1]);
       return false;
     }