summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorwm4 <wm4@nowhere>2020-05-15 16:13:28 +0200
committerwm4 <wm4@nowhere>2020-05-15 16:37:41 +0200
commit5309497727debfe1b268f915c5a41bdbe93ad9de (patch)
tree7c93ad237e5e8df5a0634352c7f9d4547eb29ed5
parenta58b2df3f84da9c19e4900e2524036a176c16c7a (diff)
downloadmpv-5309497727debfe1b268f915c5a41bdbe93ad9de.tar.bz2
mpv-5309497727debfe1b268f915c5a41bdbe93ad9de.tar.xz
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).
-rw-r--r--osdep/subprocess-posix.c135
1 files 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 <http://www.gnu.org/licenses/>.
*/
-#include "osdep/posix-spawn.h"
#include <poll.h>
#include <pthread.h>
#include <unistd.h>
@@ -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]);