From af9c34e9ec0ec82fe887078d67a54ad5155a24d4 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Sun, 19 Jul 2020 21:19:05 -0700 Subject: [PATCH] Use vfork instead of posix_spawn WARNING: This is an experimental fix for the race conditions reported at ksh93/ksh#79 and att/ast#468. There may be bugs that have not yet been caught. This patch replaces usage of posix_spawn with vfork, since the former is prone to race conditions. src/lib/libast/features/lib, src/lib/libast/comp/spawnveg.c: - Remove the posix_spawn implementation of spawnveg. - Set the terminal process group between fork/vfork and execve to fix a race condition. src/cmd/ksh93/sh/xec.c: - Set the terminal process group on error to fix a vfork(2) lockup and crash with fork(2); both occured after the spawnveg fix. --- src/cmd/ksh93/sh/xec.c | 14 ++-- src/lib/libast/comp/spawnveg.c | 50 ++------------ src/lib/libast/features/lib | 116 +-------------------------------- 3 files changed, 14 insertions(+), 166 deletions(-) diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index 4f99919bf1b8..ad374c93192a 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -3724,12 +3724,16 @@ static pid_t sh_ntfork(Shell_t *shp,const Shnode_t *t,char *argv[],int *jobid,in fail: if(jobfork && spawnpid<0) job_fork(0); - if(spawnpid < 0) switch(errno=shp->path_err) + if(spawnpid < 0) { - case ENOENT: - errormsg(SH_DICT,ERROR_system(ERROR_NOENT),e_found+4); - default: - errormsg(SH_DICT,ERROR_system(ERROR_NOEXEC),e_exec+4); + tcsetpgrp(2,job.curpgid); + switch(errno=shp->path_err) + { + case ENOENT: + errormsg(SH_DICT,ERROR_system(ERROR_NOENT),e_found+4); + default: + errormsg(SH_DICT,ERROR_system(ERROR_NOEXEC),e_exec+4); + } } } else diff --git a/src/lib/libast/comp/spawnveg.c b/src/lib/libast/comp/spawnveg.c index 6117e3ad1bf2..feee6ee16d07 100644 --- a/src/lib/libast/comp/spawnveg.c +++ b/src/lib/libast/comp/spawnveg.c @@ -38,51 +38,6 @@ NoN(spawnveg) #else -#if _lib_posix_spawn > 1 /* reports underlying exec() errors */ - -#include -#include -#include - -pid_t -spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) -{ - int err; - pid_t pid; - posix_spawnattr_t attr; - - if (err = posix_spawnattr_init(&attr)) - goto nope; - if (pgid) - { - if (pgid <= 1) - pgid = 0; - if (err = posix_spawnattr_setpgroup(&attr, pgid)) - goto bad; - if (err = posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETPGROUP)) - goto bad; - } - if (err = posix_spawn(&pid, path, NiL, &attr, argv, envv ? envv : environ)) - goto bad; - posix_spawnattr_destroy(&attr); -#if _lib_posix_spawn < 2 - if (waitpid(pid, &err, WNOHANG|WNOWAIT) == pid && EXIT_STATUS(err) == 127) - { - while (waitpid(pid, NiL, 0) == -1 && errno == EINTR); - if (!access(path, X_OK)) - errno = ENOEXEC; - pid = -1; - } -#endif - return pid; - bad: - posix_spawnattr_destroy(&attr); - nope: - errno = err; - return -1; -} - -#else #if _lib_spawn_mode @@ -211,7 +166,11 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) { m = 0; if (pgid == 1 || pgid == -2 && (m = 1)) + { pgid = getpid(); + if (!m) + tcsetpgrp(2, pgid); + } if (setpgid(0, pgid) < 0 && errno == EPERM) setpgid(pgid, 0); #if _lib_tcgetpgrp @@ -291,6 +250,5 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) #endif -#endif #endif diff --git a/src/lib/libast/features/lib b/src/lib/libast/features/lib index d5d44bebba2a..9a68041a9313 100644 --- a/src/lib/libast/features/lib +++ b/src/lib/libast/features/lib @@ -241,120 +241,6 @@ tst real_vfork note{ vfork child shares data with parent }end execute{ } }end -tst lib_posix_spawn unistd.h stdlib.h spawn.h -Dfork=______fork note{ posix_spawn exists and it works and its worth using }end status{ - #include - #include - #include - #include - #include - #include - #include - /* if it uses fork() why bother? */ - #undef fork - pid_t fork _ARG_((void)) { NOTE("uses fork()"); return -1; } - pid_t _fork _ARG_((void)) { NOTE("uses _fork()"); return -1; } - pid_t __fork _ARG_((void)) { NOTE("uses __fork()"); return -1; } - int - main(argc, argv) - int argc; - char** argv; - { - char* s; - pid_t pid; - posix_spawnattr_t attr; - int n; - int status; - char* cmd[3]; - char tmp[1024]; - if (argv[1]) - _exit(signal(SIGHUP, SIG_DFL) != SIG_IGN); - signal(SIGHUP, SIG_IGN); - if (posix_spawnattr_init(&attr)) - { - NOTE("posix_spawnattr_init() FAILED"); - _exit(0); - } - if (posix_spawnattr_setpgroup(&attr, 0)) - { - NOTE("posix_spawnattr_setpgroup() FAILED"); - _exit(0); - } - if (posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETPGROUP)) - { - NOTE("posix_spawnattr_setflags() FAILED"); - _exit(0); - } - /* first try an a.out and verify that SIGHUP is ignored */ - cmd[0] = argv[0]; - cmd[1] = "test"; - cmd[2] = 0; - if (posix_spawn(&pid, cmd[0], 0, &attr, cmd, 0)) - { - NOTE("posix_spawn() FAILED"); - _exit(0); - } - status = 1; - if (wait(&status) < 0) - { - NOTE("wait() FAILED"); - _exit(0); - } - if (status != 0) - { - NOTE("SIGHUP ignored in parent not ignored in child"); - _exit(0); - } - /* must return exec-type errors or its useless to us *unless* there is no [v]fork() */ - n = strlen(cmd[0]); - if (n >= (sizeof(tmp) - 3)) - { - NOTE("test executable path too long"); - _exit(0); - } - strcpy(tmp, cmd[0]); - tmp[n] = '.'; - tmp[n+1] = 's'; - tmp[n+2] = 'h'; - tmp[n+3] = 0; - if ((n = open(tmp, O_CREAT|O_WRONLY, S_IRWXU|S_IRWXG|S_IRWXO)) < 0 || - chmod(tmp, S_IRWXU|S_IRWXG|S_IRWXO) < 0 || - write(n, "exit 99\n", 8) != 8 || - close(n) < 0) - { - NOTE("test script create FAILED"); - _exit(0); - } - cmd[0] = tmp; - n = 0; /* 0 means reject */ - pid = -1; - if (posix_spawn(&pid, cmd[0], 0, &attr, cmd, 0)) - { - n = 2; - NOTE("ENOEXEC produces posix_spawn() error (BEST)"); - } - else if (pid == -1) - NOTE("ENOEXEC returns pid == -1"); - else if (wait(&status) != pid) - NOTE("ENOEXEC produces no child process"); - else if (!WIFEXITED(status)) - NOTE("ENOEXEC produces signal exit"); - else - { - status = WEXITSTATUS(status); - if (status == 127) - { - n = 1; - NOTE("ENOEXEC produces exit status 127 (GOOD)"); - } - else if (status == 99) - NOTE("ENOEXEC invokes sh"); - else if (status == 0) - NOTE("ENOEXEC reports no error"); - } - _exit(n); - } -}end - tst lib_spawn_mode unistd.h stdlib.h note{ first spawn arg is mode and it works }end execute{ #include #include @@ -689,7 +575,7 @@ tst - output{ printf("\n"); #endif - #if _lib_spawnveg || _lib_posix_spawn || _lib_spawn_mode || _lib_spawn && _hdr_spawn && _mem_pgroup_inheritance || _lib_vfork && _real_vfork + #if _lib_spawnveg || _lib_spawn_mode || _lib_spawn && _hdr_spawn && _mem_pgroup_inheritance || _lib_vfork && _real_vfork printf("#if !_AST_no_spawnveg\n"); printf("#define _use_spawnveg 1\n"); printf("#endif\n");