From fac59b12be63890da9c40c0d5f95da6b7c1e1e81 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Wed, 26 Jan 2022 01:10:34 -0800 Subject: [PATCH] Implement support for POSIX_SPAWN_TCSETPGROUP & fix spawnveg bugs Note that this is still a WIP. Current TODOs: - Error out if posix_spawnattr_tcsetpgrp_np is detected but posix_spawn is not. - Document the changes to spawnveg in the relevant man page. - Maybe: Use tcsetpgrp in the spawnveg fork fallback (if this is implemented, vfork will not be allowed to use that function since the behavior of tcsetpgrp after vfork is unpredictable and not portable). This commit implements support for the glibc POSIX_SPAWN_TCSETPGROUP and posix_spawnattr_tcsetpgrp_np extension [1]. This was done with the intention of improving performance in interactive shells without reintroducing previous race conditions[2][3]. These changes were tested with glibc commit 342cc934. src/cmd/ksh93/sh/path.c: - Tell spawnveg to set the terminal process group when launching a child process in an interactive shell. src/cmd/ksh93/sh/xec.c: - Allow posix_spawn when posix_spawnattr_tcsetpgrp_np is available. - Reimplement most of the sh_ntfork() SIGTSTP handling code removed in commit 66c37202. src/lib/libast/comp/spawnveg.c, src/lib/libast/misc/procopen.c, src/lib/libast/features/sys: - Add support for POSIX_SPAWN_TCSETPGROUP as implemented in glibc. This code (with a few changes) might also work on QNX, but I've avoided changing anything on that OS since I don't have a QNX system to test with. - Bugfix 1: If pgid == -1, use POSIX_SPAWN_SETSID when it's available (spawnveg uses setsid(2) after fork/vfork, so it should do the same thing when posix_spawn is used instead. src/lib/libast/features/lib: - Bugfix 2: Fix syntax errors in the posix_spawn feature test that caused ksh to fall back to vfork instead. - Detect the existence of posix_spawnattr_tcsetpgrp_np(). [1]: https://sourceware.org/git/?p=glibc.git;a=commit;h=342cc934a3bf74ac618e2318d738f22ac93257ba [2]: https://github.com/ksh93/ksh/issues/79 [3]: https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html --- src/cmd/ksh93/sh/path.c | 2 +- src/cmd/ksh93/sh/xec.c | 34 +++++++++++++++++++++++++++++++- src/lib/libast/comp/spawnveg.c | 36 ++++++++++++++++++++++++++++------ src/lib/libast/features/lib | 7 ++++--- src/lib/libast/features/sys | 2 +- src/lib/libast/misc/procopen.c | 4 ++-- 6 files changed, 71 insertions(+), 14 deletions(-) diff --git a/src/cmd/ksh93/sh/path.c b/src/cmd/ksh93/sh/path.c index b16744b6676a..164891cee6e9 100644 --- a/src/cmd/ksh93/sh/path.c +++ b/src/cmd/ksh93/sh/path.c @@ -140,7 +140,7 @@ static pid_t _spawnveg(const char *path, char* const argv[], char* const envp[], while(1) { sh_stats(STAT_SPAWN); - pid = spawnveg(path,argv,envp,pgid); + pid = spawnveg(path,argv,envp,pgid,job.jobcontrol?job.fd:-1); if(pid>=0 || errno!=EAGAIN) break; } diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index 31329f53b2ba..52afcd3c3e7d 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -1623,7 +1623,11 @@ int sh_exec(register const Shnode_t *t, int flags) fifo_save_ppid = sh.current_pid; #endif #if SHOPT_SPAWN +#if !_lib_posix_spawnattr_tcsetpgrp_np if(com && !job.jobcontrol) +#else + if(com) +#endif /* !_lib_posix_spawnattr_tcsetpgrp_np */ { parent = sh_ntfork(t,com,&jobid,ntflag); if(parent<0) @@ -3462,7 +3466,8 @@ static void sigreset(int mode) /* * A combined fork/exec for systems with slow fork(). - * Incompatible with job control on interactive shells (job.jobcontrol). + * Incompatible with job control on interactive shells (job.jobcontrol) if + * the system does not support the POSIX_SPAWN_TCSETPGROUP flag. */ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int flag) { @@ -3474,6 +3479,9 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int flag) char **arge, *path; volatile pid_t grp = 0; Pathcomp_t *pp; +#if _lib_posix_spawnattr_tcsetpgrp_np + volatile int jobwasset=0; +#endif /* _lib_posix_spawnattr_tcsetpgrp_np */ if(flag) { otype = savetype; @@ -3538,8 +3546,21 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int flag) } arge = sh_envgen(); sh.exitval = 0; +#if _lib_posix_spawnattr_tcsetpgrp_np + if(job.jobcontrol) + { + signal(SIGTTIN,SIG_DFL); + signal(SIGTTOU,SIG_DFL); + signal(SIGTSTP,SIG_DFL); + jobwasset++; + } +#endif /* _lib_posix_spawnattr_tcsetpgrp_np */ #ifdef JOBS +#if _lib_posix_spawnattr_tcsetpgrp_np + if(sh_isstate(SH_MONITOR) && (job.jobcontrol || (otype&FAMP))) +#else if(sh_isstate(SH_MONITOR) && (otype&FAMP)) +#endif /* _lib_posix_spawnattr_tcsetpgrp_np */ { if((otype&FAMP) || job.curpgid==0) grp = 1; @@ -3600,6 +3621,17 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int flag) sh_popcontext(buffp); if(buffp->olist) free_list(buffp->olist); +#if _lib_posix_spawnattr_tcsetpgrp_np + if(jobwasset) + { + signal(SIGTTIN,SIG_IGN); + signal(SIGTTOU,SIG_IGN); + if(sh_isstate(SH_INTERACTIVE)) + signal(SIGTSTP,SIG_IGN); + else + signal(SIGTSTP,SIG_DFL); + } +#endif /* _lib_posix_spawnattr_tcsetpgrp_np */ if(sigwasset) sigreset(1); /* restore ignored signals */ if(scope) diff --git a/src/lib/libast/comp/spawnveg.c b/src/lib/libast/comp/spawnveg.c index bb0db2a8a73c..bf03cfdf2f4d 100644 --- a/src/lib/libast/comp/spawnveg.c +++ b/src/lib/libast/comp/spawnveg.c @@ -44,10 +44,18 @@ NoN(spawnveg) #include #include +#if !_lib_posix_spawn +#error "DEBUG ERROR 1" +#elif !_lib_posix_spawnattr_tcsetpgrp_np +#error "DEBUG ERROR 2" +#elif !POSIX_SPAWN_TCSETPGROUP +#error "DEBUG ERROR 3" +#endif + pid_t -spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) +spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) { - int err; + int err, flags = 0; pid_t pid; posix_spawnattr_t attr; @@ -55,12 +63,25 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) goto nope; if (pgid) { + flags |= POSIX_SPAWN_SETPGROUP; +#if _lib_posix_spawnattr_tcsetpgrp_np + if (tcfd >= 0) + flags |= POSIX_SPAWN_TCSETPGROUP; +#endif +#if POSIX_SPAWN_SETSID + if (pgid == -1) + flags |= POSIX_SPAWN_SETSID; +#endif + if (err = posix_spawnattr_setflags(&attr, flags)) + goto bad; if (pgid <= 1) pgid = 0; if (err = posix_spawnattr_setpgroup(&attr, pgid)) goto bad; - if (err = posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETPGROUP)) +#if _lib_posix_spawnattr_tcsetpgrp_np + if (tcfd >= 0 && (err = posix_spawnattr_tcsetpgrp_np(&attr, tcfd))) goto bad; +#endif } if (err = posix_spawn(&pid, path, NiL, &attr, argv, envv ? envv : environ)) { @@ -99,8 +120,9 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) #endif pid_t -spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) +spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) { + NOT_USED(tcfd); #if defined(P_DETACH) return spawnve(pgid ? P_DETACH : P_NOWAIT, path, argv, envv ? envv : environ); #else @@ -119,10 +141,11 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) */ pid_t -spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) +spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) { struct inheritance inherit; + NOT_USED(tcfd); inherit.flags = 0; if (pgid) { @@ -156,7 +179,7 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) */ pid_t -spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) +spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) { #if _lib_fork || _lib_vfork int n; @@ -171,6 +194,7 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid) #endif #endif + NOT_USED(tcfd); if (!envv) envv = environ; #if _lib_spawnve diff --git a/src/lib/libast/features/lib b/src/lib/libast/features/lib index 6a8295f6b1b7..484fdf804885 100644 --- a/src/lib/libast/features/lib +++ b/src/lib/libast/features/lib @@ -240,9 +240,9 @@ tst lib_posix_spawn unistd.h stdlib.h spawn.h -Dfork=______fork note{ posix_spaw #include /* if it uses fork() why bother? */ #undef fork - pid_t fork(void)) { NOTE("uses fork()"; return -1; } - pid_t _fork(void)) { NOTE("uses _fork()"; return -1; } - pid_t __fork(void)) { NOTE("uses __fork()"; return -1; } + pid_t fork(void) { NOTE("uses fork()"); return -1; } + pid_t _fork(void) { NOTE("uses _fork()"); return -1; } + pid_t __fork(void) { NOTE("uses __fork()"); return -1; } int main(argc, argv) int argc; @@ -343,6 +343,7 @@ tst lib_posix_spawn unistd.h stdlib.h spawn.h -Dfork=______fork note{ posix_spaw _exit(n); } }end +lib posix_spawnattr_tcsetpgrp_np tst lib_spawn_mode unistd.h stdlib.h note{ first spawn arg is mode and it works }end execute{ #include diff --git a/src/lib/libast/features/sys b/src/lib/libast/features/sys index 6bab48802be6..a07334037cdb 100644 --- a/src/lib/libast/features/sys +++ b/src/lib/libast/features/sys @@ -195,7 +195,7 @@ extern setpgid int (pid_t, pid_t) extern setsid pid_t (void) extern setuid int (uid_t) extern sleep unsigned (unsigned int) -extern spawnveg pid_t (const char*, char* const[], char* const[], pid_t) +extern spawnveg pid_t (const char*, char* const[], char* const[], pid_t, int) extern srand void (unsigned int) extern strcasecmp int (const char*, const char*) extern strcat char* (char*, const char*) diff --git a/src/lib/libast/misc/procopen.c b/src/lib/libast/misc/procopen.c index 2bced4de35de..c63a15d8d1e2 100644 --- a/src/lib/libast/misc/procopen.c +++ b/src/lib/libast/misc/procopen.c @@ -744,7 +744,7 @@ sfsync(sfstderr); if (forked || (flags & PROC_OVERLAY)) execve(path, p, environ); #if _use_spawnveg - else if ((proc->pid = spawnveg(path, p, environ, proc->pgrp)) != -1) + else if ((proc->pid = spawnveg(path, p, environ, proc->pgrp, -1)) != -1) goto cleanup; #endif if (errno != ENOEXEC) @@ -773,7 +773,7 @@ sfsync(sfstderr); execve(env + 2, p, environ); #if _use_spawnveg else - proc->pid = spawnveg(env + 2, p, environ, proc->pgrp); + proc->pid = spawnveg(env + 2, p, environ, proc->pgrp, -1); #endif cleanup: if (forked)