From 29983114b48b6f81b9bce1d0081a22820d0f0435 Mon Sep 17 00:00:00 2001 From: Joshua Hudson Date: Tue, 6 Nov 2018 18:24:11 -0800 Subject: [PATCH 01/10] Use vfork() to start child processes where this yields a performance improvement due to getting rid of page faults. --- src/Native/Unix/Common/pal_config.h.in | 1 + src/Native/Unix/System.Native/pal_process.c | 76 +++++++++++++++++---- src/Native/Unix/configure.cmake | 9 +++ 3 files changed, 72 insertions(+), 14 deletions(-) diff --git a/src/Native/Unix/Common/pal_config.h.in b/src/Native/Unix/Common/pal_config.h.in index b65878873740..b0d3412634aa 100644 --- a/src/Native/Unix/Common/pal_config.h.in +++ b/src/Native/Unix/Common/pal_config.h.in @@ -12,6 +12,7 @@ #cmakedefine01 HAVE_UTSNAME_DOMAINNAME #cmakedefine01 HAVE_STAT64 #cmakedefine01 HAVE_PIPE2 +#cmakedefine01 HAVE_VFORK_SHM #cmakedefine01 HAVE_STAT_BIRTHTIME #cmakedefine01 HAVE_STAT_TIMESPEC #cmakedefine01 HAVE_STAT_TIM diff --git a/src/Native/Unix/System.Native/pal_process.c b/src/Native/Unix/System.Native/pal_process.c index 577f8ddd8c3d..da492433a170 100644 --- a/src/Native/Unix/System.Native/pal_process.c +++ b/src/Native/Unix/System.Native/pal_process.c @@ -94,6 +94,7 @@ static int Dup2WithInterruptedRetry(int oldfd, int newfd) return result; } +#if !HAVE_VFORK_SHM static ssize_t WriteSize(int fd, const void* buffer, size_t count) { ssize_t rv = 0; @@ -145,6 +146,7 @@ static void ExitChild(int pipeToParent, int error) } _exit(error != 0 ? error : EXIT_FAILURE); } +#endif int32_t SystemNative_ForkAndExecProcess(const char* filename, char* const argv[], @@ -165,8 +167,17 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, bool haveProcessCreateLock = false; #endif bool success = true; - int stdinFds[2] = {-1, -1}, stdoutFds[2] = {-1, -1}, stderrFds[2] = {-1, -1}, waitForChildToExecPipe[2] = {-1, -1}; - int processId = -1; + int stdinFds[2] = {-1, -1}, stdoutFds[2] = {-1, -1}, stderrFds[2] = {-1, -1}; + pid_t processId = -1; + +#if HAVE_VFORK_SHM + // Gets written to by child; initial value is -1 because it really can't happen while 0 is merely a bug elsewhere + volatile int captureErrno = -1; +#define ExitChildOnError(err) (void)_exit(captureErrno = err) /* this can't be a function */ +#else + int waitForChildToExecPipe[2] = {-1, -1}; +#define ExitChildOnError(err) ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], err) +#endif // Validate arguments if (NULL == filename || NULL == argv || NULL == envp || NULL == stdinFd || NULL == stdoutFd || @@ -221,6 +232,34 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, goto done; } +#if HAVE_VFORK_SHM + // This platform has the shared-memory implementation of vfork(). For a one gigabyte process + // the expected performance gain of using 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. Eliding the pipe for sending the error from execve() back to the parent is pure + // gravy. + + // 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, changing signals 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 // We create a pipe purely for the benefit of knowing when the child process has called exec. // We can use that to block waiting on the pipe to be closed, which lets us block the parent // from returning until the child process is actually transitioned to the target program. This @@ -233,14 +272,8 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, SystemNative_Pipe(waitForChildToExecPipe, PAL_O_CLOEXEC); #endif - // Fork the child process - if ((processId = fork()) == -1) - { - success = false; - goto done; - } - - if (processId == 0) // processId == 0 if this is child process + if ((processId = fork()) == 0) // processId == 0 if this is child process +#endif { // 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. @@ -248,14 +281,14 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, (redirectStdout && Dup2WithInterruptedRetry(stdoutFds[WRITE_END_OF_PIPE], STDOUT_FILENO) == -1) || (redirectStderr && Dup2WithInterruptedRetry(stderrFds[WRITE_END_OF_PIPE], STDERR_FILENO) == -1)) { - ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); + ExitChildOnError(errno); } if (setCredentials) { if (setgid(groupId) == -1 || setuid(userId) == -1) { - ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); + ExitChildOnError(errno); } } @@ -266,14 +299,20 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, while (CheckInterrupted(result = chdir(cwd))); if (result == -1) { - ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); + ExitChildOnError(errno); } } // Finally, execute the new process. execve will not return if it's successful. execve(filename, argv, envp); - ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); // execve failed + ExitChildOnError(errno); // execve failed + } + else if (processId < 0) + { + success = false; + goto done; } +#undef ExitChildOnError // This is the parent process. processId == pid of the child *childPid = processId; @@ -297,6 +336,14 @@ done:; CloseIfOpen(stdoutFds[WRITE_END_OF_PIPE]); CloseIfOpen(stderrFds[WRITE_END_OF_PIPE]); +#if HAVE_VFORK_SHM + int childError = captureErrno; // Don't read volatile more than necessary + if (childError != -1) + { + success = false; + priorErrno = childError; + } +#else // Also close the write end of the exec waiting pipe, and wait for the pipe to be closed // by trying to read from it (the read will wake up when the pipe is closed and broken). // Ignore any errors... this is a best-effort attempt. @@ -315,6 +362,7 @@ done:; } CloseIfOpen(waitForChildToExecPipe[READ_END_OF_PIPE]); } +#endif // If we failed, close everything else and give back error values in all out arguments. if (!success) diff --git a/src/Native/Unix/configure.cmake b/src/Native/Unix/configure.cmake index 9ac5041097c1..ab4af1e6721c 100644 --- a/src/Native/Unix/configure.cmake +++ b/src/Native/Unix/configure.cmake @@ -327,6 +327,15 @@ check_c_source_compiles( " HAVE_SENDFILE_6) +check_c_source_runs( + " + #include + #include + #include + int main(void) { volatile int shm = 1; int status; pid_t pid; if ((pid = vfork()) == 0) { shm = 0; _exit(0); } if (pid > 0) waitpid(pid, &status, 0); return shm; } + " + HAVE_VFORK_SHM) + check_symbol_exists( fcopyfile copyfile.h From 9dc1bbbae7daef1524fcef9981b57660f18df187 Mon Sep 17 00:00:00 2001 From: Joshua Hudson Date: Mon, 12 Nov 2018 18:58:55 -0800 Subject: [PATCH 02/10] Remove specific dependency on shared memory vfork so that cross compiles work again. --- src/Native/Unix/Common/pal_config.h.in | 2 +- src/Native/Unix/System.Native/pal_process.c | 68 +++++++-------------- src/Native/Unix/configure.cmake | 14 ++--- 3 files changed, 29 insertions(+), 55 deletions(-) diff --git a/src/Native/Unix/Common/pal_config.h.in b/src/Native/Unix/Common/pal_config.h.in index b0d3412634aa..6195a57f2d0e 100644 --- a/src/Native/Unix/Common/pal_config.h.in +++ b/src/Native/Unix/Common/pal_config.h.in @@ -11,8 +11,8 @@ #cmakedefine01 HAVE_GETIFADDRS #cmakedefine01 HAVE_UTSNAME_DOMAINNAME #cmakedefine01 HAVE_STAT64 +#cmakedefine01 HAVE_VFORK #cmakedefine01 HAVE_PIPE2 -#cmakedefine01 HAVE_VFORK_SHM #cmakedefine01 HAVE_STAT_BIRTHTIME #cmakedefine01 HAVE_STAT_TIMESPEC #cmakedefine01 HAVE_STAT_TIM diff --git a/src/Native/Unix/System.Native/pal_process.c b/src/Native/Unix/System.Native/pal_process.c index da492433a170..71693ed0898a 100644 --- a/src/Native/Unix/System.Native/pal_process.c +++ b/src/Native/Unix/System.Native/pal_process.c @@ -94,7 +94,6 @@ static int Dup2WithInterruptedRetry(int oldfd, int newfd) return result; } -#if !HAVE_VFORK_SHM static ssize_t WriteSize(int fd, const void* buffer, size_t count) { ssize_t rv = 0; @@ -146,7 +145,6 @@ static void ExitChild(int pipeToParent, int error) } _exit(error != 0 ? error : EXIT_FAILURE); } -#endif int32_t SystemNative_ForkAndExecProcess(const char* filename, char* const argv[], @@ -167,18 +165,9 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, bool haveProcessCreateLock = false; #endif bool success = true; - int stdinFds[2] = {-1, -1}, stdoutFds[2] = {-1, -1}, stderrFds[2] = {-1, -1}; + int stdinFds[2] = {-1, -1}, stdoutFds[2] = {-1, -1}, stderrFds[2] = {-1, -1}, waitForChildToExecPipe[2] = {-1, 1}; pid_t processId = -1; -#if HAVE_VFORK_SHM - // Gets written to by child; initial value is -1 because it really can't happen while 0 is merely a bug elsewhere - volatile int captureErrno = -1; -#define ExitChildOnError(err) (void)_exit(captureErrno = err) /* this can't be a function */ -#else - int waitForChildToExecPipe[2] = {-1, -1}; -#define ExitChildOnError(err) ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], err) -#endif - // Validate arguments if (NULL == filename || NULL == argv || NULL == envp || NULL == stdinFd || NULL == stdoutFd || NULL == stderrFd || NULL == childPid) @@ -232,13 +221,24 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, goto done; } -#if HAVE_VFORK_SHM - // This platform has the shared-memory implementation of vfork(). For a one gigabyte process - // the expected performance gain of using 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. Eliding the pipe for sending the error from execve() back to the parent is pure - // gravy. + // We create a pipe purely for the benefit of knowing when the child process has called exec. + // We can use that to block waiting on the pipe to be closed, which lets us block the parent + // from returning until the child process is actually transitioned to the target program. This + // avoids problems where the parent process uses members of Process, like ProcessName, when the + // Process is still the clone of this one. This is a best-effort attempt, so ignore any errors. + // If the child fails to exec we use the pipe to pass the errno to the parent process. +#if HAVE_PIPE2 + pipe2(waitForChildToExecPipe, O_CLOEXEC); +#else + SystemNative_Pipe(waitForChildToExecPipe, PAL_O_CLOEXEC); +#endif + +#if HAVE_VFORK + // 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: @@ -260,18 +260,6 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, if ((processId = vfork()) == 0) // processId == 0 if this is child process #else - // We create a pipe purely for the benefit of knowing when the child process has called exec. - // We can use that to block waiting on the pipe to be closed, which lets us block the parent - // from returning until the child process is actually transitioned to the target program. This - // avoids problems where the parent process uses members of Process, like ProcessName, when the - // Process is still the clone of this one. This is a best-effort attempt, so ignore any errors. - // If the child fails to exec we use the pipe to pass the errno to the parent process. -#if HAVE_PIPE2 - pipe2(waitForChildToExecPipe, O_CLOEXEC); -#else - SystemNative_Pipe(waitForChildToExecPipe, PAL_O_CLOEXEC); -#endif - if ((processId = fork()) == 0) // processId == 0 if this is child process #endif { @@ -281,14 +269,14 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, (redirectStdout && Dup2WithInterruptedRetry(stdoutFds[WRITE_END_OF_PIPE], STDOUT_FILENO) == -1) || (redirectStderr && Dup2WithInterruptedRetry(stderrFds[WRITE_END_OF_PIPE], STDERR_FILENO) == -1)) { - ExitChildOnError(errno); + ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); } if (setCredentials) { if (setgid(groupId) == -1 || setuid(userId) == -1) { - ExitChildOnError(errno); + ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); } } @@ -299,20 +287,19 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, while (CheckInterrupted(result = chdir(cwd))); if (result == -1) { - ExitChildOnError(errno); + ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); } } // Finally, execute the new process. execve will not return if it's successful. execve(filename, argv, envp); - ExitChildOnError(errno); // execve failed + ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); } else if (processId < 0) { success = false; goto done; } -#undef ExitChildOnError // This is the parent process. processId == pid of the child *childPid = processId; @@ -336,14 +323,6 @@ done:; CloseIfOpen(stdoutFds[WRITE_END_OF_PIPE]); CloseIfOpen(stderrFds[WRITE_END_OF_PIPE]); -#if HAVE_VFORK_SHM - int childError = captureErrno; // Don't read volatile more than necessary - if (childError != -1) - { - success = false; - priorErrno = childError; - } -#else // Also close the write end of the exec waiting pipe, and wait for the pipe to be closed // by trying to read from it (the read will wake up when the pipe is closed and broken). // Ignore any errors... this is a best-effort attempt. @@ -362,7 +341,6 @@ done:; } CloseIfOpen(waitForChildToExecPipe[READ_END_OF_PIPE]); } -#endif // If we failed, close everything else and give back error values in all out arguments. if (!success) diff --git a/src/Native/Unix/configure.cmake b/src/Native/Unix/configure.cmake index ab4af1e6721c..45e02f3bc0db 100644 --- a/src/Native/Unix/configure.cmake +++ b/src/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 @@ -327,15 +332,6 @@ check_c_source_compiles( " HAVE_SENDFILE_6) -check_c_source_runs( - " - #include - #include - #include - int main(void) { volatile int shm = 1; int status; pid_t pid; if ((pid = vfork()) == 0) { shm = 0; _exit(0); } if (pid > 0) waitpid(pid, &status, 0); return shm; } - " - HAVE_VFORK_SHM) - check_symbol_exists( fcopyfile copyfile.h From 5e9b8e3f37db617d7aabf9a2d75ed8604c5a5bde Mon Sep 17 00:00:00 2001 From: Joshua Hudson Date: Thu, 13 Dec 2018 20:48:12 -0800 Subject: [PATCH 03/10] 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 --- src/Native/Unix/System.Native/pal_process.c | 55 +++++++++++++++++++-- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_process.c b/src/Native/Unix/System.Native/pal_process.c index 71693ed0898a..b4b202f08246 100644 --- a/src/Native/Unix/System.Native/pal_process.c +++ b/src/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 @@ -167,6 +165,12 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, bool success = true; int stdinFds[2] = {-1, -1}, stdoutFds[2] = {-1, -1}, stderrFds[2] = {-1, -1}, waitForChildToExecPipe[2] = {-1, 1}; pid_t processId = -1; + int thread_cancel_state; + sigset_t signal_set; + sigset_t old_signal_set; + + // None of this code can be cancelled 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,6 +237,11 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, SystemNative_Pipe(waitForChildToExecPipe, PAL_O_CLOEXEC); #endif + // 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 // 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 @@ -263,6 +272,35 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, if ((processId = fork()) == 0) // processId == 0 if this is child process #endif { + // 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. + sigset_t junk_signal_set; + struct sigaction sa_default; + struct sigaction sa_old; + struct sigaction sa_trash; + 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) + { + if (sigaction(sig, &sa_default, &sa_old)) + { + if (sig != SIGKILL && sig != SIGSTOP) + break; // No more signals + } else { + if ((sa_old.sa_flags & SA_SIGINFO) + ? ((void (*)(int))sa_old.sa_sigaction == SIG_IGN || (void (*)(int))sa_old.sa_sigaction == SIG_DFL) + : (sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)) + { + // It has a pre-defined handler -- put it back + sigaction(sig, &sa_old, &sa_trash); + } + } + } + pthread_sigmask(SIG_SETMASK, &old_signal_set, &junk_signal_set); // Not all architectures allow NULL here + // 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) || @@ -295,8 +333,13 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, execve(filename, argv, envp); ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); } - else if (processId < 0) + + // 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; } @@ -362,10 +405,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) From 9fd4ccea40c1792dae613b0fa1a549172f794568 Mon Sep 17 00:00:00 2001 From: Joshua Hudson Date: Sun, 23 Dec 2018 13:17:11 -0800 Subject: [PATCH 04/10] Fix issues from vfork() pull request review --- src/Native/Unix/System.Native/pal_process.c | 29 ++++++++++++--------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_process.c b/src/Native/Unix/System.Native/pal_process.c index b4b202f08246..7180f398a8bb 100644 --- a/src/Native/Unix/System.Native/pal_process.c +++ b/src/Native/Unix/System.Native/pal_process.c @@ -277,6 +277,9 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, // 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; @@ -285,18 +288,20 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, sa_default.sa_handler = SIG_DFL; for (int sig = 1; ; ++sig) { + if (sig == SIGKILL || sig == SIGSTOP) + { + continue; + } if (sigaction(sig, &sa_default, &sa_old)) { - if (sig != SIGKILL && sig != SIGSTOP) - break; // No more signals - } else { - if ((sa_old.sa_flags & SA_SIGINFO) - ? ((void (*)(int))sa_old.sa_sigaction == SIG_IGN || (void (*)(int))sa_old.sa_sigaction == SIG_DFL) - : (sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)) - { - // It has a pre-defined handler -- put it back - sigaction(sig, &sa_old, &sa_trash); - } + break; // No more signals + } + if ((sa_old.sa_flags & SA_SIGINFO) + ? ((void (*)(int))sa_old.sa_sigaction == SIG_IGN || (void (*)(int))sa_old.sa_sigaction == SIG_DFL) + : (sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)) + { + // It has a pre-defined handler -- put it back -- the flags value may be significant + sigaction(sig, &sa_old, &sa_trash); } } pthread_sigmask(SIG_SETMASK, &old_signal_set, &junk_signal_set); // Not all architectures allow NULL here @@ -331,7 +336,7 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, // Finally, execute the new process. execve will not return if it's successful. execve(filename, argv, envp); - ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); + ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno); // execve failed } // Restore signal mask in the parent process immediately after fork() or vfork() call @@ -408,7 +413,7 @@ done:; } // Restore thread cancel state - //pthread_setcancelstate(thread_cancel_state, &thread_cancel_state); + pthread_setcancelstate(thread_cancel_state, &thread_cancel_state); return success ? 0 : -1; } From 2504a81d20da7c32831d1db0d5756a3027162f8e Mon Sep 17 00:00:00 2001 From: Joshua Hudson Date: Sat, 5 Jan 2019 17:15:10 -0800 Subject: [PATCH 05/10] Check handler before replacing it --- src/Native/Unix/System.Native/pal_process.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_process.c b/src/Native/Unix/System.Native/pal_process.c index 7180f398a8bb..69985ac8b89e 100644 --- a/src/Native/Unix/System.Native/pal_process.c +++ b/src/Native/Unix/System.Native/pal_process.c @@ -283,7 +283,6 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, sigset_t junk_signal_set; struct sigaction sa_default; struct sigaction sa_old; - struct sigaction sa_trash; 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) @@ -292,16 +291,17 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, { continue; } - if (sigaction(sig, &sa_default, &sa_old)) + if (sigaction(sig, NULL, &sa_old)) { break; // No more signals } if ((sa_old.sa_flags & SA_SIGINFO) - ? ((void (*)(int))sa_old.sa_sigaction == SIG_IGN || (void (*)(int))sa_old.sa_sigaction == SIG_DFL) - : (sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)) + ? ((void (*)(int))sa_old.sa_sigaction != SIG_IGN && (void (*)(int))sa_old.sa_sigaction != SIG_DFL) + : (sa_old.sa_handler != SIG_IGN && sa_old.sa_handler != SIG_DFL)) { - // It has a pre-defined handler -- put it back -- the flags value may be significant - sigaction(sig, &sa_old, &sa_trash); + // It has a custom handler, put the default handler back. + // We check first th 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 From 6713107a57ae27bafdb77d0b07f0a315590b0814 Mon Sep 17 00:00:00 2001 From: Joshua Hudson Date: Mon, 7 Jan 2019 20:35:30 -0800 Subject: [PATCH 06/10] Improve readability of signal handler removing --- src/Native/Unix/System.Native/pal_process.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_process.c b/src/Native/Unix/System.Native/pal_process.c index 69985ac8b89e..7b05eff3909b 100644 --- a/src/Native/Unix/System.Native/pal_process.c +++ b/src/Native/Unix/System.Native/pal_process.c @@ -258,7 +258,7 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, // + 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, changing signals in the vfork() child is a bad idea. We don't + // 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 @@ -295,9 +295,8 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, { break; // No more signals } - if ((sa_old.sa_flags & SA_SIGINFO) - ? ((void (*)(int))sa_old.sa_sigaction != SIG_IGN && (void (*)(int))sa_old.sa_sigaction != SIG_DFL) - : (sa_old.sa_handler != SIG_IGN && sa_old.sa_handler != SIG_DFL)) + 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 th preserve flags on default handlers. From 0328815a46b8033a77fc92848149f47a661669cd Mon Sep 17 00:00:00 2001 From: Joshua Hudson Date: Wed, 9 Jan 2019 18:28:21 -0800 Subject: [PATCH 07/10] Convert tabs to spaces --- src/Native/Unix/System.Native/pal_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_process.c b/src/Native/Unix/System.Native/pal_process.c index 7b05eff3909b..ab50eb47f72e 100644 --- a/src/Native/Unix/System.Native/pal_process.c +++ b/src/Native/Unix/System.Native/pal_process.c @@ -272,7 +272,7 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, if ((processId = fork()) == 0) // processId == 0 if this is child process #endif { - // It turns out that child processes depend on their sigmask being set to something sane rather than mask all. + // 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. @@ -295,11 +295,11 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, { break; // No more signals } - 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) + 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 th preserve flags on default handlers. + // We check first th preserve flags on default handlers. sigaction(sig, &sa_default, NULL); } } From 7186c01d62bd0e8c9ac2577ef5a0b56131a1f710 Mon Sep 17 00:00:00 2001 From: Joshua Hudson Date: Sun, 27 Jan 2019 19:28:47 -0800 Subject: [PATCH 08/10] use NSIG instead of dynamic probing because glibc punches a hole in the middle of the signal list --- src/Native/Unix/System.Native/pal_process.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_process.c b/src/Native/Unix/System.Native/pal_process.c index ab50eb47f72e..6d524636e3f4 100644 --- a/src/Native/Unix/System.Native/pal_process.c +++ b/src/Native/Unix/System.Native/pal_process.c @@ -285,22 +285,21 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, 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) + for (int sig = 1; sig < NSIG; ++sig) { if (sig == SIGKILL || sig == SIGSTOP) { continue; } - if (sigaction(sig, NULL, &sa_old)) + if (!sigaction(sig, NULL, &sa_old)) { - break; // No more signals - } - 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 th preserve flags on default handlers. - sigaction(sig, &sa_default, NULL); + 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 th 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 From 0cb3196b9d623edaa4bb26c1ba19d1366e1b6f78 Mon Sep 17 00:00:00 2001 From: Joshua Hudson Date: Sun, 10 Feb 2019 11:40:57 -0800 Subject: [PATCH 09/10] Exclude Mac OSX from vfork() because we don't quite trust it. --- src/Native/Unix/System.Native/pal_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_process.c b/src/Native/Unix/System.Native/pal_process.c index 6d524636e3f4..048024a1249e 100644 --- a/src/Native/Unix/System.Native/pal_process.c +++ b/src/Native/Unix/System.Native/pal_process.c @@ -242,9 +242,9 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, sigfillset(&signal_set); pthread_sigmask(SIG_SETMASK, &signal_set, &old_signal_set); -#if HAVE_VFORK +#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 + // 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. From 40860e0fe55a1d64fd89481c7268ee9350d9a4c4 Mon Sep 17 00:00:00 2001 From: Joshua Hudson Date: Mon, 11 Feb 2019 18:22:00 -0800 Subject: [PATCH 10/10] Fix one last batch of typos --- src/Native/Unix/System.Native/pal_process.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_process.c b/src/Native/Unix/System.Native/pal_process.c index 048024a1249e..b568050bfd99 100644 --- a/src/Native/Unix/System.Native/pal_process.c +++ b/src/Native/Unix/System.Native/pal_process.c @@ -163,13 +163,13 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, bool haveProcessCreateLock = false; #endif bool success = true; - int stdinFds[2] = {-1, -1}, stdoutFds[2] = {-1, -1}, stderrFds[2] = {-1, -1}, waitForChildToExecPipe[2] = {-1, 1}; + int stdinFds[2] = {-1, -1}, stdoutFds[2] = {-1, -1}, stderrFds[2] = {-1, -1}, waitForChildToExecPipe[2] = {-1, -1}; pid_t processId = -1; int thread_cancel_state; sigset_t signal_set; sigset_t old_signal_set; - // None of this code can be cancelled without leaking handles, so just don't allow it + // 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 @@ -297,7 +297,7 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, if (oldhandler != SIG_IGN && oldhandler != SIG_DFL) { // It has a custom handler, put the default handler back. - // We check first th preserve flags on default handlers. + // We check first to preserve flags on default handlers. sigaction(sig, &sa_default, NULL); } }