[sanitizer_common] [Darwin] Replace pty with pipe on posix_spawn path for spawning symbolizer (#170809)
Due to a legacy incompatibility with `atos`, we were allocating a pty
whenever we spawned the symbolizer. This is no longer necessary and we
can use a regular ol' pipe.
This PR is split into two commits:
- The first removes the pty allocation and replaces it with a pipe. This
relocates the `CreateTwoHighNumberedPipes` call to be common to the
`posix_spawn` and `StartSubprocess` path.
- The second commit adds the `child_stdin_fd_` field to
`SymbolizerProcess`, storing the read end of the stdin pipe. By holding
on to this fd for the lifetime of the symbolizer, we are able to avoid
getting SIGPIPE (which would occur when we write to a pipe whose
read-end had been closed due to the death of the symbolizer). This will
be very close to solving #120915, but this PR is intentionally not
touching the non-posix_spawn path.
rdar://165894284
NOKEYCHECK=True
GitOrigin-RevId: dc92bd03c965a200724a51ec6d5e89f2d449f1dd
diff --git a/lib/sanitizer_common/sanitizer_mac.cpp b/lib/sanitizer_common/sanitizer_mac.cpp
index a6f7571..3f8de8d 100644
--- a/lib/sanitizer_common/sanitizer_mac.cpp
+++ b/lib/sanitizer_common/sanitizer_mac.cpp
@@ -281,53 +281,43 @@
(size_t)newlen);
}
-static fd_t internal_spawn_impl(const char *argv[], const char *envp[],
- pid_t *pid) {
- fd_t primary_fd = kInvalidFd;
- fd_t secondary_fd = kInvalidFd;
+bool internal_spawn(const char* argv[], const char* envp[], pid_t* pid,
+ fd_t fd_stdin, fd_t fd_stdout) {
+ // NOTE: Caller ensures that fd_stdin and fd_stdout are not 0, 1, or 2, since
+ // this can break communication.
+ //
+ // NOTE: Caller is responsible for closing fd_stdin after the process has
+ // died.
+ int res;
auto fd_closer = at_scope_exit([&] {
- internal_close(primary_fd);
- internal_close(secondary_fd);
+ // NOTE: We intentionally do not close fd_stdin since this can
+ // cause us to receive a fatal SIGPIPE if the process dies.
+ internal_close(fd_stdout);
});
- // 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.
- primary_fd = posix_openpt(O_RDWR);
- if (primary_fd == kInvalidFd)
- return kInvalidFd;
-
- int res = grantpt(primary_fd) || unlockpt(primary_fd);
- if (res != 0) return kInvalidFd;
-
- // Use TIOCPTYGNAME instead of ptsname() to avoid threading problems.
- char secondary_pty_name[128];
- res = ioctl(primary_fd, TIOCPTYGNAME, secondary_pty_name);
- if (res == -1) return kInvalidFd;
-
- secondary_fd = internal_open(secondary_pty_name, O_RDWR);
- if (secondary_fd == kInvalidFd)
- return kInvalidFd;
-
// File descriptor actions
posix_spawn_file_actions_t acts;
res = posix_spawn_file_actions_init(&acts);
- if (res != 0) return kInvalidFd;
+ if (res != 0)
+ return false;
auto acts_cleanup = at_scope_exit([&] {
posix_spawn_file_actions_destroy(&acts);
});
- res = posix_spawn_file_actions_adddup2(&acts, secondary_fd, STDIN_FILENO) ||
- posix_spawn_file_actions_adddup2(&acts, secondary_fd, STDOUT_FILENO) ||
- posix_spawn_file_actions_addclose(&acts, secondary_fd);
- if (res != 0) return kInvalidFd;
+ res = posix_spawn_file_actions_adddup2(&acts, fd_stdin, STDIN_FILENO) ||
+ posix_spawn_file_actions_adddup2(&acts, fd_stdout, STDOUT_FILENO) ||
+ posix_spawn_file_actions_addclose(&acts, fd_stdin) ||
+ posix_spawn_file_actions_addclose(&acts, fd_stdout);
+ if (res != 0)
+ return false;
// Spawn attributes
posix_spawnattr_t attrs;
res = posix_spawnattr_init(&attrs);
- if (res != 0) return kInvalidFd;
+ if (res != 0)
+ return false;
auto attrs_cleanup = at_scope_exit([&] {
posix_spawnattr_destroy(&attrs);
@@ -336,50 +326,17 @@
// In the spawned process, close all file descriptors that are not explicitly
// described by the file actions object. This is Darwin-specific extension.
res = posix_spawnattr_setflags(&attrs, POSIX_SPAWN_CLOEXEC_DEFAULT);
- if (res != 0) return kInvalidFd;
+ if (res != 0)
+ return false;
// posix_spawn
char **argv_casted = const_cast<char **>(argv);
char **envp_casted = const_cast<char **>(envp);
res = posix_spawn(pid, argv[0], &acts, &attrs, argv_casted, envp_casted);
- if (res != 0) return kInvalidFd;
+ if (res != 0)
+ return false;
- // Disable echo in the new terminal, disable CR.
- struct termios termflags;
- tcgetattr(primary_fd, &termflags);
- termflags.c_oflag &= ~ONLCR;
- termflags.c_lflag &= ~ECHO;
- tcsetattr(primary_fd, TCSANOW, &termflags);
-
- // On success, do not close primary_fd on scope exit.
- fd_t fd = primary_fd;
- primary_fd = kInvalidFd;
-
- return fd;
-}
-
-fd_t internal_spawn(const char *argv[], const char *envp[], 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;
- }
-
- fd_t fd = internal_spawn_impl(argv, envp, pid);
-
- for (; count > 0; count--) {
- internal_close(low_fds[count]);
- }
-
- return fd;
+ return true;
}
uptr internal_rename(const char *oldpath, const char *newpath) {
diff --git a/lib/sanitizer_common/sanitizer_posix.h b/lib/sanitizer_common/sanitizer_posix.h
index b5491c5..063408b 100644
--- a/lib/sanitizer_common/sanitizer_posix.h
+++ b/lib/sanitizer_common/sanitizer_posix.h
@@ -67,7 +67,8 @@
uptr internal_waitpid(int pid, int *status, int options);
int internal_fork();
-fd_t internal_spawn(const char *argv[], const char *envp[], pid_t *pid);
+bool internal_spawn(const char* argv[], const char* envp[], pid_t* pid,
+ fd_t stdin, fd_t stdout);
int internal_sysctl(const int *name, unsigned int namelen, void *oldp,
uptr *oldlenp, const void *newp, uptr newlen);
diff --git a/lib/sanitizer_common/sanitizer_symbolizer_internal.h b/lib/sanitizer_common/sanitizer_symbolizer_internal.h
index 2345aee..6442a29 100644
--- a/lib/sanitizer_common/sanitizer_symbolizer_internal.h
+++ b/lib/sanitizer_common/sanitizer_symbolizer_internal.h
@@ -83,7 +83,7 @@
const char *SendCommand(const char *command);
protected:
- ~SymbolizerProcess() {}
+ ~SymbolizerProcess();
/// The maximum number of arguments required to invoke a tool process.
static const unsigned kArgVMax = 16;
@@ -114,6 +114,10 @@
fd_t input_fd_;
fd_t output_fd_;
+ // We hold on to the child's stdin fd (the read end of the pipe)
+ // so that when we write to it, we don't get a SIGPIPE
+ fd_t child_stdin_fd_;
+
InternalMmapVector<char> buffer_;
static const uptr kMaxTimesRestarted = 5;
diff --git a/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp b/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
index 565701c..cc31d3d 100644
--- a/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
+++ b/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
@@ -476,10 +476,11 @@
return symbolizer_process_->SendCommand(buffer_);
}
-SymbolizerProcess::SymbolizerProcess(const char *path, bool use_posix_spawn)
+SymbolizerProcess::SymbolizerProcess(const char* path, bool use_posix_spawn)
: path_(path),
input_fd_(kInvalidFd),
output_fd_(kInvalidFd),
+ child_stdin_fd_(kInvalidFd),
times_restarted_(0),
failed_to_start_(false),
reported_invalid_path_(false),
@@ -488,6 +489,11 @@
CHECK_NE(path_[0], '\0');
}
+SymbolizerProcess::~SymbolizerProcess() {
+ if (child_stdin_fd_ != kInvalidFd)
+ CloseFile(child_stdin_fd_);
+}
+
static bool IsSameModule(const char *path) {
if (const char *ProcessName = GetProcessName()) {
if (const char *SymbolizerName = StripModuleName(path)) {
@@ -533,6 +539,10 @@
CloseFile(input_fd_);
if (output_fd_ != kInvalidFd)
CloseFile(output_fd_);
+ if (child_stdin_fd_ != kInvalidFd) {
+ CloseFile(child_stdin_fd_);
+ child_stdin_fd_ = kInvalidFd; // Don't free in destructor
+ }
return StartSymbolizerSubprocess();
}
diff --git a/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp b/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
index 7eb0c97..29c73e3 100644
--- a/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
+++ b/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
@@ -156,30 +156,30 @@
Printf("\n");
}
+ 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;
+ }
+
if (use_posix_spawn_) {
# if SANITIZER_APPLE
- fd_t fd = internal_spawn(argv, const_cast<const char **>(GetEnvP()), &pid);
- if (fd == kInvalidFd) {
+ bool success = internal_spawn(argv, const_cast<const char**>(GetEnvP()),
+ &pid, outfd[0], infd[1]);
+ if (!success) {
Report("WARNING: failed to spawn external symbolizer (errno: %d)\n",
errno);
+ internal_close(infd[0]);
+ internal_close(outfd[1]);
return false;
}
-
- input_fd_ = fd;
- output_fd_ = fd;
# else // SANITIZER_APPLE
UNIMPLEMENTED();
# endif // SANITIZER_APPLE
} else {
- 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, GetEnvP(), /* stdin */ outfd[0],
/* stdout */ infd[1]);
if (pid < 0) {
@@ -187,11 +187,14 @@
internal_close(outfd[1]);
return false;
}
-
- input_fd_ = infd[0];
- output_fd_ = outfd[1];
}
+ input_fd_ = infd[0];
+ output_fd_ = outfd[1];
+
+ // We intentionally hold on to the read-end so that we don't get a SIGPIPE
+ child_stdin_fd_ = outfd[0];
+
CHECK_GT(pid, 0);
// Check that symbolizer subprocess started successfully.
