From 5309497727debfe1b268f915c5a41bdbe93ad9de Mon Sep 17 00:00:00 2001 From: wm4 Date: Fri, 15 May 2020 16:13:28 +0200 Subject: [PATCH] subprocess: replace posix_spawnp() with fork() This code runs posix_spawnp() within a fork() in some cases, in order to "disown" processes which are meant as being started detached. But posix_spawnp() is not marked as async-signal-safe, so what we do is not allowed. It could for example cause deadlocks, depending on implementation and luck at runtime. Turns out posix_spawnp() is useless crap. Replace it with "classic" fork() to ensure correctness. We could probably use another mechanism to start a process "disowned" than doing a double-fork(). The only problem with "disowning" a process is calling setsid() (which posix_spawnp() didn't support, but maybe will in newer revisions), and removing as as parent from the child process (the double-fork() will make PID 1 the parent). But there is no good way to either remove us as parent, or to "reap" the PID in a way that is safe and less of a mess than the current code. This is because POSIX/UNIX is a miserable heap of shit. (Less shit than "alternatives" like win32, no doubt.) Because POSIX/UNIX is a miserable heap of shit, execvp() is also not specified as async-signal-safe. It's funny how you can run a full fledged HTTP server in an async-signal-safe context, but not start a shitty damn process. Unix is really, really, really extremely bad at this process management stuff. So we reimplement execvp() in an async-signal-safe way. The new code assumes that CLOEXEC is a thing. Since POSIX/UNIX is such a heap of shit, O_CLOEXEC and FD_CLOEXEC were (probably) added at different times, but both must be present. io.h defines them to 0 if they don't exist, and in this case the code will error out at runtime. Surely we could do without CLOEXEC via fallback, but I'll do that only if at least 1 bug is reported wrt. this issue. The idea how to report exec() failure or success is from musl. The way as_execvpe() is also inspired by musl (for example, the list of error codes that should make it fail is the same as in musl's code). --- osdep/subprocess-posix.c | 135 ++++++++++++++++++++++++++++++++++----- 1 file changed, 118 insertions(+), 17 deletions(-) diff --git a/osdep/subprocess-posix.c b/osdep/subprocess-posix.c index fb11179618..df3c9600ac 100644 --- a/osdep/subprocess-posix.c +++ b/osdep/subprocess-posix.c @@ -15,7 +15,6 @@ * License along with mpv. If not, see . */ -#include "osdep/posix-spawn.h" #include #include #include @@ -36,18 +35,128 @@ extern char **environ; #define SAFE_CLOSE(fd) do { if ((fd) >= 0) close((fd)); (fd) = -1; } while (0) +// Async-signal-safe execvpe(). POSIX does not list it as async-signal-safe +// (POSIX is such a joke), so do it manually. While in theory the searching is +// apparently implementation dependent and not exposed (because POSIX is a +// joke?), the expected rules are still relatively simple. +// Doesn't set errno correctly. +// Somewhat inspired by musl's src/process/execvp.c. +static int as_execvpe(const char *path, const char *file, char *const argv[], + char *const envp[]) +{ + if (strchr(file, '/') || !file[0]) + return execve(file, argv, envp); + + size_t flen = strlen(file); + while (path && path[0]) { + size_t plen = strcspn(path, ":"); + // Ignore paths that are too long. + char fn[PATH_MAX]; + if (plen + 1 + flen + 1 < sizeof(fn)) { + memcpy(fn, path, plen); + fn[plen] = '/'; + memcpy(fn + plen + 1, file, flen + 1); + execve(fn, argv, envp); + if (errno != EACCES && errno != ENOENT && errno != ENOTDIR) + break; + } + path += plen + (path[plen] == ':' ? 1 : 0); + } + return -1; +} + +// Returns 0 on any error, valid PID on success. +// This function must be async-signal-safe, as it may be called from a fork(). +static pid_t spawn_process(const char *path, struct mp_subprocess_opts *opts, + int src_fds[]) +{ + int p[2] = {-1, -1}; + pid_t fres = 0; + sigset_t sigmask, oldmask; + sigfillset(&sigmask); + pthread_sigmask(SIG_BLOCK, &sigmask, &oldmask); + + // We setup a communication pipe to signal failure. Since the child calls + // exec() and becomes the calling process, we don't know if or when the + // child process successfully ran exec() just from the PID. + // Use a CLOEXEC pipe to detect whether exec() was used. Obviously it will + // be closed if exec() succeeds, and an error is written if not. + // There are also some things further below in the code that need CLOEXEC. + if (mp_make_cloexec_pipe(p) < 0) + goto done; + // Check whether CLOEXEC is really set. Important for correct operation. + int p_flags = fcntl(p[0], F_GETFD); + if (p_flags == -1 || !FD_CLOEXEC || !(p_flags & FD_CLOEXEC)) + goto done; // require CLOEXEC; unknown if fallback would be worth it + + fres = fork(); + if (fres < 0) { + fres = 0; + goto done; + } + if (fres == 0) { + // child + + for (int n = 0; n < opts->num_fds; n++) { + if (src_fds[n] == opts->fds[n].fd) { + int flags = fcntl(opts->fds[n].fd, F_GETFD); + if (flags == -1) + goto child_failed; + flags &= ~(unsigned)FD_CLOEXEC; + if (fcntl(opts->fds[n].fd, F_SETFD, flags) == -1) + goto child_failed; + } else if (dup2(src_fds[n], opts->fds[n].fd) < 0) { + goto child_failed; + } + } + + as_execvpe(path, opts->exe, opts->args, opts->env ? opts->env : environ); + + child_failed: + write(p[1], &(char){1}, 1); // shouldn't be able to fail + _exit(1); + } + + SAFE_CLOSE(p[1]); + + int r; + do { + r = read(p[0], &(char){0}, 1); + } while (r < 0 && errno == EINTR); + + // If exec()ing child failed, collect it immediately. + if (r != 0) { + while (waitpid(fres, &(int){0}, 0) < 0 && errno == EINTR) {} + fres = 0; + } + +done: + pthread_sigmask(SIG_SETMASK, &oldmask, NULL); + SAFE_CLOSE(p[0]); + SAFE_CLOSE(p[1]); + + return fres; +} + void mp_subprocess2(struct mp_subprocess_opts *opts, struct mp_subprocess_result *res) { - posix_spawn_file_actions_t fa; - bool fa_destroy = false; int status = -1; int comm_pipe[MP_SUBPROCESS_MAX_FDS][2]; + int src_fds[MP_SUBPROCESS_MAX_FDS]; int devnull = -1; pid_t pid = 0; bool spawned = false; bool killed_by_us = false; int cancel_fd = -1; + char *path = getenv("PATH"); + char path_storage[PATH_MAX]; + if (!path) { + path = path_storage; + size_t r = confstr(_CS_PATH, path, sizeof(path_storage)); + if (r == 0 || r >= sizeof(path_storage)) + path[0] = '\0'; // failure, who cares + } *res = (struct mp_subprocess_result){0}; @@ -69,10 +178,6 @@ void mp_subprocess2(struct mp_subprocess_opts *opts, if (devnull < 0) goto done; - if (posix_spawn_file_actions_init(&fa)) - goto done; - fa_destroy = true; - // redirect FDs for (int n = 0; n < opts->num_fds; n++) { int src_fd = devnull; @@ -80,12 +185,9 @@ void mp_subprocess2(struct mp_subprocess_opts *opts, src_fd = comm_pipe[n][1]; if (opts->fds[n].src_fd >= 0) src_fd = opts->fds[n].src_fd; - if (posix_spawn_file_actions_adddup2(&fa, src_fd, opts->fds[n].fd)) - goto done; + src_fds[n] = src_fd; } - char **env = opts->env ? opts->env : environ; - if (opts->detach) { // If we run it detached, we fork a child to start the process; then // it exits immediately, letting PID 1 inherit it. So we don't need @@ -99,7 +201,7 @@ void mp_subprocess2(struct mp_subprocess_opts *opts, if (fres == 0) { // child setsid(); - if (posix_spawnp(&pid, opts->exe, &fa, NULL, opts->args, env)) + if (!spawn_process(path, opts, src_fds)) _exit(1); _exit(0); } @@ -108,13 +210,14 @@ void mp_subprocess2(struct mp_subprocess_opts *opts, while (waitpid(fres, &child_status, 0) < 0 && errno == EINTR) {} if (!WIFEXITED(child_status) || WEXITSTATUS(child_status) != 0) goto done; - spawned = true; } else { - if (posix_spawnp(&pid, opts->exe, &fa, NULL, opts->args, env)) + pid = spawn_process(path, opts, src_fds); + if (!pid) goto done; - spawned = true; } + spawned = true; + for (int n = 0; n < opts->num_fds; n++) SAFE_CLOSE(comm_pipe[n][1]); SAFE_CLOSE(devnull); @@ -174,8 +277,6 @@ void mp_subprocess2(struct mp_subprocess_opts *opts, while (waitpid(pid, &status, 0) < 0 && errno == EINTR) {} done: - if (fa_destroy) - posix_spawn_file_actions_destroy(&fa); for (int n = 0; n < opts->num_fds; n++) { SAFE_CLOSE(comm_pipe[n][0]); SAFE_CLOSE(comm_pipe[n][1]);