[compiler-rt] Replace forkpty with posix_spawn

[kubamracek]

Summary

On Darwin, we currently use forkpty to communicate with the "atos" symbolizer. There are several problems that fork or forkpty 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. Since posix_spawn doesn't fork (at least on Darwin), the interceptors are not a problem. Additionally, this also fixes a latent threading problem with ptsname (it's unsafe to use this function in multithreaded programs). Yet another benefit is that we'll handle post-fork failures (e.g. sandbox disallows "exec") gracefully now.

[dvyukov]

I can't say that I fully understand what happens here, but I can rubber stamp LGTM if you wish.

FWIW for linux we have a bunch of checks if we are in symbolizer in interceptors, and do something different if we are.

lib/sanitizer_common/sanitizer_mac.cc

229 ↗

(On Diff #122853)

Don't we want to use internal_open/internal_close here? open/close are intercepted.

[kubamracek]

Updating patch.

[kubamracek]

lib/sanitizer_common/sanitizer_mac.cc

229 ↗

(On Diff #122853)

Updated patch. Although on Darwin, there's no difference, plain calls to open/close call the original functions.

[dberris]

LGTM -- nothing looks obviously broken to me here, though you probably want to get one more person to LGTM.

[kubamracek]

@george.karpenkov would you mind taking a look?

[george.karpenkov]

LGTM with a few nits, but I'm not 100% sure what's going on either..

lib/sanitizer_common/sanitizer_mac.cc

216 ↗

(On Diff #133308)

I would appreciate a comment on why do you need a pseudo-terminal here.

217 ↗

(On Diff #133308)

Is kInvalidFd equal to -1 ?

261 ↗

(On Diff #133308)

could the error happen after the file descriptor was set? (e.g. system call fails, but the pointer was still written into?) Might be safer to have multiple cleanup labels to jump to the one where you know it's safe to close the descriptor?

[kubamracek]

Updated patch.

lib/sanitizer_common/sanitizer_mac.cc

261 ↗

(On Diff #133308)

I don't think this can happen. Both master_fd and slave_fd are only assigned directly in the code above, there's no pointers involved. I think it's pretty safe to say that they're either kInvalidFd or valid fds that need to be closed.

compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc

 #include <fcntl.h>
 #include <libkern/OSAtomic.h>
 #include <mach-o/dyld.h>
 #include <mach/mach.h>
 #include <mach/vm_statistics.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>

 extern "C" pid_t __fork(void) SANITIZER_WEAK_ATTRIBUTE;
 
 int internal_fork() {
   if (&__fork)
     return __fork();
   return fork();
 }
 
-int internal_forkpty(int *amaster) {
-  int master, slave;
-  if (openpty(&master, &slave, nullptr, nullptr, nullptr) == -1) return -1;
-  int pid = internal_fork();
-  if (pid == -1) {
-    close(master);
-    close(slave);
-    return -1;
-  }
-  if (pid == 0) {
-    close(master);
-    if (login_tty(slave) != 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 {
-    *amaster = master;
-    close(slave);
-  }
-  return pid;
+fd_t internal_spawn(const char *argv[], pid_t *pid) {
+  int master_fd = kInvalidFd;
+  int slave_fd = kInvalidFd;
+  char **env = GetEnviron();
+  int res = 0;
+
+  // We need a new pseudoterminal to avoid bufferring 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) goto cleanup;
+  res = grantpt(master_fd);
+  if (res != 0) goto cleanup;
+  res = unlockpt(master_fd);
+  if (res != 0) goto cleanup;
+
+  // 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) goto cleanup;
+
+  slave_fd = internal_open(slave_pty_name, O_RDWR);
+  if (slave_fd == kInvalidFd) goto cleanup;
+
+  posix_spawn_file_actions_t actions;
+  res = posix_spawn_file_actions_init(&actions);
+  if (res != 0) goto cleanup;
+  res = posix_spawn_file_actions_adddup2(&actions, slave_fd, STDIN_FILENO);
+  if (res != 0) goto cleanup;
+  res = posix_spawn_file_actions_adddup2(&actions, slave_fd, STDOUT_FILENO);
+  if (res != 0) goto cleanup;
+  res = posix_spawn_file_actions_addclose(&actions, slave_fd);
+  if (res != 0) goto cleanup;
+  res = posix_spawn_file_actions_addclose(&actions, master_fd);
+  if (res != 0) goto cleanup;
+
+  res = posix_spawnp(pid, argv[0], &actions, NULL, const_cast<char **>(argv),
+                     env);
+  if (res != 0) goto cleanup;
+
+  internal_close(slave_fd);
+  slave_fd = 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);
+
+  return master_fd;
+
+ cleanup:
+  if (master_fd != kInvalidFd) internal_close(master_fd);
+  if (slave_fd != kInvalidFd) internal_close(slave_fd);
+  return kInvalidFd;
 }
 
 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/trunk/lib/sanitizer_common/sanitizer_posix.h

 uptr internal_unlink(const char *path);
 uptr internal_rename(const char *oldpath, const char *newpath);
 uptr internal_lseek(fd_t fd, OFF_T offset, int whence);
 
 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);
 
 // These functions call appropriate pthread_ functions directly, bypassing
 // the interceptor. They are weak and may not be present in some tools.
 SANITIZER_WEAK_ATTRIBUTE
 int real_pthread_create(void *th, void *attr, void *(*callback)(void *),
                         void *param);
 SANITIZER_WEAK_ATTRIBUTE
 int real_pthread_join(void *th, void **ret);

compiler-rt/trunk/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:
   virtual bool ReachedEndOfOutput(const char *buffer, uptr length) const {
     UNIMPLEMENTED();
   }
 
   /// The maximum number of arguments required to invoke a tool process.
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/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc

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/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cc

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:
   bool ReachedEndOfOutput(const char *buffer, uptr length) const override {
     return (length >= 1 && buffer[length - 1] == '\n');

compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

 
 #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;
+  pid_t 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);
 
-  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);
+    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;
+    }
+
     pid = StartSubprocess(path_, argv, /* stdin */ outfd[0],
                           /* stdout */ infd[1]);
     if (pid < 0) {
       internal_close(infd[0]);
       internal_close(outfd[1]);
       return false;
     }