[compiler-rt] r324846 - [compiler-rt] Replace forkpty with posix_spawn
Kuba Mracek via llvm-commits
llvm-commits at lists.llvm.org
Sun Feb 11 11:23:42 PST 2018
Author: kuba.brecka
Date: Sun Feb 11 11:23:42 2018
New Revision: 324846
URL: http://llvm.org/viewvc/llvm-project?rev=324846&view=rev
Log:
[compiler-rt] Replace forkpty with posix_spawn
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.
Differential Revision: https://reviews.llvm.org/D40032
Modified:
compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc
compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h
compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h
compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc
compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cc
compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc?rev=324846&r1=324845&r2=324846&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc Sun Feb 11 11:23:42 2018
@@ -63,7 +63,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>
@@ -205,27 +207,62 @@ int internal_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) {
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h?rev=324846&r1=324845&r2=324846&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h Sun Feb 11 11:23:42 2018
@@ -57,7 +57,7 @@ uptr internal_ptrace(int request, int pi
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.
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h?rev=324846&r1=324845&r2=324846&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h Sun Feb 11 11:23:42 2018
@@ -72,7 +72,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:
@@ -109,7 +109,7 @@ class SymbolizerProcess {
uptr times_restarted_;
bool failed_to_start_;
bool reported_invalid_path_;
- bool use_forkpty_;
+ bool use_posix_spawn_;
};
class LLVMSymbolizerProcess;
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc?rev=324846&r1=324845&r2=324846&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc Sun Feb 11 11:23:42 2018
@@ -395,14 +395,14 @@ const char *LLVMSymbolizer::FormatAndSen
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');
}
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cc?rev=324846&r1=324845&r2=324846&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cc (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_mac.cc Sun Feb 11 11:23:42 2018
@@ -51,7 +51,7 @@ bool DlAddrSymbolizer::SymbolizeData(upt
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);
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc?rev=324846&r1=324845&r2=324846&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc Sun Feb 11 11:23:42 2018
@@ -34,10 +34,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.
@@ -152,80 +148,36 @@ bool SymbolizerProcess::StartSymbolizerS
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) {
More information about the llvm-commits
mailing list