[sanitizer] Make atos stdin a non-tty pipe to make sure it's not stuck waiting for user input

[kubamracek]

Summary

On macOS, we often symbolicate using atos (when llvm-symbolizer is not found). The current way we invoke atos involves creating a pseudo-terminal to make sure atos doesn't buffer its output. This however also makes atos think that it's stdin is interactive and in some error situations it will ask the user to enter some input instead of just printing out an error message. For example, when Developer Mode isn't enabled on a machine, atos cannot examine processes, and it will ask the user to enter an administrator's password, which will make the sanitized process get stuck. This patch only connects the pseudo-terminal to the stdout of atos, and uses a regular pipe as its stdin.

I'm not adding a testcase, because to reproduce the problem you have to disable Developer Mode on your machine, which is not something that can be done as a lit test. But if your machine has Developer Mode disabled, you will already get test failures of other tests.

[zaks.anna]

LGTM

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

187 ↗

(On Diff #79669)

This might be a bit more readable:

`// infd[0] is the child's reading end.
close(infd[1]);`

`// Setup stdin to serve as the infd pipe.
close(STDIN_FILENO);
CHECK_GE(dup2(infd[0], STDIN_FILENO), 0);
close(infd[0]);`

[zaks.anna]

Other than the nit, this looks good!

[filcab]

LGTM

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

114 ↗

(On Diff #79669)

Maybe extract the 5 to be a constant?
Also, why 5? It looks to me that 4 should be enough (I was planning on 3, which would make a bigger difference, but then noticed you want two pairs):
worst case: 0, 1, and 2 are closed.
pipe() creates 0, 1 <- Not good, keep trying
pipe() creates 2, X <- Not good, keep trying
pipe() creates Y, X <- ok, since 0, 1, and 2 are already taken by previous pipes.
pipe() creates A, B <- ok

189 ↗

(On Diff #79669)

dup2 will close STDIN_FILENO by itself. No need to close it first.

[filcab]

Nevermind the CreateTwoHighNumberedPipes comment. I tried deleting but couldn't. I saw you're just moving code around, so let's not refactor it now.

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

 
 const char *DemangleSwiftAndCXX(const char *name) {
   if (!name) return nullptr;
   if (const char *swift_demangled_name = DemangleSwift(name))
     return swift_demangled_name;
   return DemangleCXXABI(name);
 }
 
+static bool CreateTwoHighNumberedPipes(int *infd_, int *outfd_) {
+  int *infd = NULL;
+  int *outfd = NULL;
+  // The client program may close its stdin and/or stdout and/or stderr
+  // thus allowing socketpair to reuse file descriptors 0, 1 or 2.
+  // In this case the communication between the forked processes may be
+  // broken if either the parent or the child tries to close or duplicate
+  // these descriptors. The loop below produces two pairs of file
+  // descriptors, each greater than 2 (stderr).
+  int sock_pair[5][2];
+  for (int i = 0; i < 5; i++) {
+    if (pipe(sock_pair[i]) == -1) {
+      for (int j = 0; j < i; j++) {
+        internal_close(sock_pair[j][0]);
+        internal_close(sock_pair[j][1]);
+      }
+      return false;
+    } else if (sock_pair[i][0] > 2 && sock_pair[i][1] > 2) {
+      if (infd == NULL) {
+        infd = sock_pair[i];
+      } else {
+        outfd = sock_pair[i];
+        for (int j = 0; j < i; j++) {
+          if (sock_pair[j] == infd) continue;
+          internal_close(sock_pair[j][0]);
+          internal_close(sock_pair[j][1]);
+        }
+        break;
+      }
+    }
+  }
+  CHECK(infd);
+  CHECK(outfd);
+  infd_[0] = infd[0];
+  infd_[1] = infd[1];
+  outfd_[0] = outfd[0];
+  outfd_[1] = outfd[1];
+  return true;
+}
+
 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;
+  int pid = -1;
+
+  int infd[2] = {};
+  int 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_forkpty_) {
 #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",
              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_ = output_fd_ = fd;
+    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);
 #else  // SANITIZER_MAC
     UNIMPLEMENTED();
 #endif  // SANITIZER_MAC
   } else {
-    int *infd = NULL;
-    int *outfd = NULL;
-    // The client program may close its stdin and/or stdout and/or stderr
-    // thus allowing socketpair to reuse file descriptors 0, 1 or 2.
-    // In this case the communication between the forked processes may be
-    // broken if either the parent or the child tries to close or duplicate
-    // these descriptors. The loop below produces two pairs of file
-    // descriptors, each greater than 2 (stderr).
-    int sock_pair[5][2];
-    for (int i = 0; i < 5; i++) {
-      if (pipe(sock_pair[i]) == -1) {
-        for (int j = 0; j < i; j++) {
-          internal_close(sock_pair[j][0]);
-          internal_close(sock_pair[j][1]);
-        }
-        Report("WARNING: Can't create a socket pair to start "
-               "external symbolizer (errno: %d)\n", errno);
-        return false;
-      } else if (sock_pair[i][0] > 2 && sock_pair[i][1] > 2) {
-        if (infd == NULL) {
-          infd = sock_pair[i];
-        } else {
-          outfd = sock_pair[i];
-          for (int j = 0; j < i; j++) {
-            if (sock_pair[j] == infd) continue;
-            internal_close(sock_pair[j][0]);
-            internal_close(sock_pair[j][1]);
-          }
-          break;
-        }
-      }
-    }
-    CHECK(infd);
-    CHECK(outfd);
-
     const char *argv[kArgVMax];
     GetArgV(path_, argv);
     pid = StartSubprocess(path_, argv, /* stdin */ outfd[0],
                           /* stdout */ infd[1]);
     if (pid < 0) {
       internal_close(infd[0]);
       internal_close(outfd[1]);
       return false;
     }
 
     input_fd_ = infd[0];
     output_fd_ = outfd[1];
   }
 
+  CHECK_GT(pid, 0);
+
   // Check that symbolizer subprocess started successfully.
   SleepForMillis(kSymbolizerStartupTimeMillis);
   if (!IsProcessRunning(pid)) {
     // Either waitpid failed, or child has already exited.
     Report("WARNING: external symbolizer didn't start up correctly!\n");
     return false;
   }