From d93ac7cafb1ea17c3aa7c3304801c8b4bf142c9e Mon Sep 17 00:00:00 2001 From: joshudson Date: Fri, 15 Feb 2019 08:50:46 -0800 Subject: [PATCH] Use vfork() improve performance when starting processes. (dotnet/corefx#33289) * Use vfork() to start child processes where this yields a performance improvement due to getting rid of page faults. * Remove specific dependency on shared memory vfork so that cross compiles work again. * Added signal mask code so that a child process can't confuse the parent; also took care of pthread cancellation mask in case third-party native code tries to use it * Fix issues from vfork() pull request review * Check handler before replacing it * Improve readability of signal handler removing * Convert tabs to spaces * use NSIG instead of dynamic probing because glibc punches a hole in the middle of the signal list * Exclude Mac OSX from vfork() because we don't quite trust it. * Fix one last batch of typos Commit migrated from https://github.com/dotnet/corefx/commit/0a561e3c71ec9b98f07b90d0c1f20f8956adafaa --- .../Native/Unix/Common/pal_config.h.in | 1 + .../Native/Unix/System.Native/pal_process.c | 98 ++++++++++++++++--- src/libraries/Native/Unix/configure.cmake | 5 + 3 files changed, 92 insertions(+), 12 deletions(-) diff --git a/src/libraries/Native/Unix/Common/pal_config.h.in b/src/libraries/Native/Unix/Common/pal_config.h.in index d372ed825d3e72..cfc43a632cc0c7 100644 --- a/src/libraries/Native/Unix/Common/pal_config.h.in +++ b/src/libraries/Native/Unix/Common/pal_config.h.in @@ -11,6 +11,7 @@ #cmakedefine01 HAVE_GETIFADDRS #cmakedefine01 HAVE_UTSNAME_DOMAINNAME #cmakedefine01 HAVE_STAT64 +#cmakedefine01 HAVE_VFORK #cmakedefine01 HAVE_PIPE2 #cmakedefine01 HAVE_STAT_BIRTHTIME #cmakedefine01 HAVE_STAT_TIMESPEC diff --git a/src/libraries/Native/Unix/System.Native/pal_process.c b/src/libraries/Native/Unix/System.Native/pal_process.c index 169ca838980ab6..77171f1e5f60cd 100644 --- a/src/libraries/Native/Unix/System.Native/pal_process.c +++ b/src/libraries/Native/Unix/System.Native/pal_process.c @@ -23,9 +23,7 @@ #if HAVE_PIPE2 #include #endif -#if !HAVE_PIPE2 #include -#endif #if HAVE_SCHED_SETAFFINITY || HAVE_SCHED_GETAFFINITY #include @@ -166,7 +164,13 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, #endif bool success = true; int stdinFds[2] = {-1, -1}, stdoutFds[2] = {-1, -1}, stderrFds[2] = {-1, -1}, waitForChildToExecPipe[2] = {-1, -1}; - int processId = -1; + pid_t processId = -1; + int thread_cancel_state; + sigset_t signal_set; + sigset_t old_signal_set; + + // None of this code can be canceled without leaking handles, so just don't allow it + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &thread_cancel_state); // Validate arguments if (NULL == filename || NULL == argv || NULL == envp || NULL == stdinFd || NULL == stdoutFd || @@ -233,15 +237,73 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, SystemNative_Pipe(waitForChildToExecPipe, PAL_O_CLOEXEC); #endif - // Fork the child process - if ((processId = fork()) == -1) + // The fork child must not be signalled until it calls exec(): our signal handlers do not + // handle being raised in the child process correctly + sigfillset(&signal_set); + pthread_sigmask(SIG_SETMASK, &signal_set, &old_signal_set); + +#if HAVE_VFORK && !(defined(__APPLE__)) // We don't trust vfork on OS X right now. + // This platform has vfork(). vfork() is either a synonym for fork or provides shared memory + // semantics. For a one gigabyte process, the expected performance gain of using shared memory + // vfork() rather than fork() is 99.5% merely due to avoiding page faults as the kernel does not + // need to set all writable pages in the parent process to copy-on-write because the child process + // is allowed to write to the parent process memory pages. + + // The thing to remember about shared memory vfork() is the documentation is way out of date. + // It does the following things: + // * creates a new process in the memory space of the calling process. + // * blocks the calling thread (not process!) in an uninterruptable sleep + // * sets up the process records so the following happen: + // + execve() replaces the memory space in the child and unblocks the parent + // + process exit by any means unblocks the parent + // + ptrace() makes a security demand against the parent process + // + accessing the terminal with read() or write() fail in system-dependent ways + // Due to lack of documentation, setting signal handlers in the vfork() child is a bad idea. We don't + // do this, but it's worth pointing out. + + // All platforms that provide shared memory vfork() check the parent process's context when + // ptrace() is used on the child, thus making setuid() safe to use after vfork(). The fabled vfork() + // security hole is the other way around; if a multithreaded host were to execute setuid() + // on another thread while a vfork() child is still pending, bad things are possible; however we + // do not do that. + + if ((processId = vfork()) == 0) // processId == 0 if this is child process +#else + if ((processId = fork()) == 0) // processId == 0 if this is child process +#endif { - success = false; - goto done; - } + // It turns out that child processes depend on their sigmask being set to something sane rather than mask all. + // On the other hand, we have to mask all to avoid our own signal handlers running in the child process, writing + // to the pipe, and waking up the handling thread in the parent process. This also avoids third-party code getting + // equally confused. + // Remove all signals, then restore signal mask. + // Since we are in a vfork() child, the only safe signal values are SIG_DFL and SIG_IGN. See man 3 libthr on BSD. + // "The implementation interposes the user-installed signal(3) handlers....to pospone signal delivery to threads + // which entered (libthr-internal) critical sections..." We want to pass SIG_DFL anyway. + sigset_t junk_signal_set; + struct sigaction sa_default; + struct sigaction sa_old; + memset(&sa_default, 0, sizeof(sa_default)); // On some architectures, sa_mask is a struct so assigning zero to it doesn't compile + sa_default.sa_handler = SIG_DFL; + for (int sig = 1; sig < NSIG; ++sig) + { + if (sig == SIGKILL || sig == SIGSTOP) + { + continue; + } + if (!sigaction(sig, NULL, &sa_old)) + { + void (*oldhandler)(int) = (sa_old.sa_flags & SA_SIGINFO) ? (void (*)(int))sa_old.sa_sigaction : sa_old.sa_handler; + if (oldhandler != SIG_IGN && oldhandler != SIG_DFL) + { + // It has a custom handler, put the default handler back. + // We check first to preserve flags on default handlers. + sigaction(sig, &sa_default, NULL); + } + } + } + pthread_sigmask(SIG_SETMASK, &old_signal_set, &junk_signal_set); // Not all architectures allow NULL here - if (processId == 0) // processId == 0 if this is child process - { // For any redirections that should happen, dup the pipe descriptors onto stdin/out/err. // We don't need to explicitly close out the old pipe descriptors as they will be closed on the 'execve' call. if ((redirectStdin && Dup2WithInterruptedRetry(stdinFds[READ_END_OF_PIPE], STDIN_FILENO) == -1) || @@ -275,6 +337,16 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); // execve failed } + // Restore signal mask in the parent process immediately after fork() or vfork() call + pthread_sigmask(SIG_SETMASK, &old_signal_set, &signal_set); + + if (processId < 0) + { + // failed + success = false; + goto done; + } + // This is the parent process. processId == pid of the child *childPid = processId; *stdinFd = stdinFds[WRITE_END_OF_PIPE]; @@ -336,10 +408,12 @@ done:; *childPid = -1; errno = priorErrno; - return -1; } - return 0; + // Restore thread cancel state + pthread_setcancelstate(thread_cancel_state, &thread_cancel_state); + + return success ? 0 : -1; } FILE* SystemNative_POpen(const char* command, const char* type) diff --git a/src/libraries/Native/Unix/configure.cmake b/src/libraries/Native/Unix/configure.cmake index a76aef06bd2e93..428063518993cf 100644 --- a/src/libraries/Native/Unix/configure.cmake +++ b/src/libraries/Native/Unix/configure.cmake @@ -118,6 +118,11 @@ check_symbol_exists( sys/stat.h HAVE_STAT64) +check_symbol_exists( + vfork + unistd.h + HAVE_VFORK) + check_symbol_exists( pipe2 unistd.h