From 3f8b0fcc7f15543ad52254027c9828e31c5b78c0 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Mon, 16 Jun 2025 15:35:25 -0700 Subject: [PATCH 1/4] Various assorted improvements to spawnveg Alterations: - Bugfix: Don't set the terminal signals to their defaults in the parent prior to calling spawnveg. This is the primary cause of a lockup in the pty tests which can be reproduced as follows: $ exec ksh $ bin/shtests pty 2>/dev/null ^C ^C ^C (SIGINT doesn't work anymore and may segfault) The correct way to go about dealing with SIGT* is to set those to SIG_DFL in the child process; see _sh_fork() for a working example that doesn't lock up or produce segfaults. In this commit I duplicate _sh_fork()'s behavior in spawnveg for the fork fallback and with POSIX_SPAWN_SETSIGDEF for the posix_spawn version. - Added support for tcsetpgrp to the fork fallback in spawnveg. Some form of this appears to have already been attempted in AT&T olden times, but that old version was broken and needed bugfixes desperately. - If the child needs tcsetpgrp, block the terminal signals in the parent process via sigcritical(). The posix_spawn version doesn't need this because posix_spawn will block signals automatically and therefore doesn't need sigcritical. - Now that the fork fallback for spawnveg works correctly in interactive terminals, prefer that to the sh_fork() codepath on operating systems without posix_spawn tcsetpgrp support. Even though the underlying system call is still ultimately fork, the sh_ntfork() codepath is faster than the traditional sh_fork codepath. Benchmark: $ time /tmp/ksh-devbranch -ic "for i in {1..10000}; do $(whence -p true); done" real 0m03.302s user 0m00.988s sys 0m02.320s $ time /tmp/ksh-newspawnveg -ic "for i in {1..10000}; do $(whence -p true); done" real 0m03.160s user 0m01.187s sys 0m01.977s - To that end, split up the spawnveg versions into spawnveg_fast and spawnveg_slow. Choose the appropriate one when spawnveg is called; this removes the need for the xec.c ifdef hackery. - Removed the ntfork_tcpgrp ifdefs from xec.c; spawnveg can handle it by itself now. - Bugfix: With the spawnveg_fast and spawnveg_slow innovation, spawnveg now always has support for setsid. It'll fallback to fork if POSIX_SPAWN_SETSID isn't available. - Bugfix: For the posix_spawn version of spawnveg, the flags should be of the short type pursuant to the POSIX specification. - Optimization: Use pipe2 in the fork fallback for spawnveg when it's available to avoid two fcntl syscalls. - Updated the spawnveg documentation to reflect the new changes. --- src/cmd/ksh93/sh/xec.c | 54 +++------------- src/lib/libast/Mamfile | 1 + src/lib/libast/comp/spawnveg.c | 112 +++++++++++++++++++++++---------- src/lib/libast/man/spawnveg.3 | 41 ++++++------ 4 files changed, 109 insertions(+), 99 deletions(-) diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index 1f78e3a738f4..02c6c48ae85a 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -44,11 +44,6 @@ # include #endif -#undef _use_ntfork_tcpgrp -#if SHOPT_SPAWN && _lib_posix_spawn > 1 && _lib_posix_spawn_file_actions_addtcsetpgrp_np -#define _use_ntfork_tcpgrp 1 -#endif - #if SHOPT_SPAWN static pid_t sh_ntfork(const Shnode_t*,char*[],int*,int); #endif /* SHOPT_SPAWN */ @@ -1437,11 +1432,7 @@ int sh_exec(const Shnode_t *t, int flags) fifo_save_ppid = sh.current_pid; #endif #if SHOPT_SPAWN -#if _use_ntfork_tcpgrp if(com) -#else - if(com && !job.jobcontrol) -#endif /* _use_ntfork_tcpgrp */ { parent = sh_ntfork(t,com,&jobid,topfd); if(parent<0) @@ -3273,9 +3264,12 @@ static void sigreset(int mode) } /* - * A combined fork/exec for systems with slow fork(). - * Incompatible with job control on interactive shells (job.jobcontrol) if - * the system does not support posix_spawn_file_actions_addtcsetpgrp_np(). + * A combined fork/exec which utilizes libast spawnveg(3) for better performance. + * The spawnveg function will invoke posix_spawn(3) or an equivalent if possible, + * and will fallback to fork(2) if absolutely necessary. For simple command + * execution this codepath is prefered because it's always a bit faster than + * the sh_fork() codepath, even when the underlying system calls it uses wind up + * being the same. */ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int topfd) { @@ -3286,9 +3280,6 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int topfd) char **arge, *path; volatile pid_t grp = 0; Pathcomp_t *pp; -#if _use_ntfork_tcpgrp - volatile int jobwasset=0; -#endif /* _use_ntfork_tcpgrp */ sh_pushcontext(buffp,SH_JMPCMD); errorpush(&buffp->err,ERROR_SILENT); job_lock(); /* errormsg will unlock */ @@ -3340,14 +3331,6 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int topfd) } arge = sh_envgen(); sh.exitval = 0; -#if _use_ntfork_tcpgrp - if(job.jobcontrol) - { - signal(SIGTTIN,SIG_DFL); - signal(SIGTTOU,SIG_DFL); - signal(SIGTSTP,SIG_DFL); - jobwasset++; - } if(sh_isstate(SH_MONITOR) && job.jobcontrol) { if(job.curpgid==0) @@ -3355,7 +3338,6 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int topfd) else grp = job.curpgid; } -#endif /* _use_ntfork_tcpgrp */ sfsync(NULL); sigreset(0); /* set signals to ignore */ @@ -3370,19 +3352,8 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int topfd) job_fork(-2); if(spawnpid == -1) { -#if _use_ntfork_tcpgrp - if(jobwasset) - { - signal(SIGTTIN,SIG_IGN); - signal(SIGTTOU,SIG_IGN); - if(sh_isstate(SH_INTERACTIVE)) - signal(SIGTSTP,SIG_IGN); - else - signal(SIGTSTP,SIG_DFL); - } if(job.jobcontrol) - tcsetpgrp(job.fd,sh.pid); -#endif /* _use_ntfork_tcpgrp */ + tcsetpgrp(job.fd,sh.pid); /* if spawnveg set tcpgrp, we must restore it ourselves */ switch(errno=sh.path_err) { case ENOENT: @@ -3405,17 +3376,6 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int topfd) sh_popcontext(buffp); if(buffp->olist) free_list(buffp->olist); -#if _use_ntfork_tcpgrp - 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 /* _use_ntfork_tcpgrp */ if(sigwasset) sigreset(1); /* restore ignored signals */ if(scope) diff --git a/src/lib/libast/Mamfile b/src/lib/libast/Mamfile index 731c1bf3d800..a729887f4fb0 100644 --- a/src/lib/libast/Mamfile +++ b/src/lib/libast/Mamfile @@ -2404,6 +2404,7 @@ make install virtual make spawnveg.o make comp/spawnveg.c + prev ast_fcntl.h prev ast_tty.h prev sig.h prev include/wait.h diff --git a/src/lib/libast/comp/spawnveg.c b/src/lib/libast/comp/spawnveg.c index f4efe46791cf..29bff9f5d0cf 100644 --- a/src/lib/libast/comp/spawnveg.c +++ b/src/lib/libast/comp/spawnveg.c @@ -28,19 +28,25 @@ */ #include +#include +#include +#include +#include +#include #if _lib_posix_spawn > 1 /* reports underlying exec() errors */ +#define _fast_spawnveg 1 #include -#include -#include -pid_t -spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) +static pid_t +spawnveg_fast(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) { - int err, flags = 0; + int err; + short flags = 0; pid_t pid; posix_spawnattr_t attr; + sigset_t mask, tcmask; #if _lib_posix_spawn_file_actions_addtcsetpgrp_np posix_spawn_file_actions_t actions; #else @@ -49,12 +55,14 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i if (err = posix_spawnattr_init(&attr)) goto nope; -#if POSIX_SPAWN_SETSID +#ifdef POSIX_SPAWN_SETSID if (pgid == -1) flags |= POSIX_SPAWN_SETSID; #endif if (pgid && pgid != -1) flags |= POSIX_SPAWN_SETPGROUP; + if (tcfd >= 0) + flags |= POSIX_SPAWN_SETSIGDEF; if (flags && (err = posix_spawnattr_setflags(&attr, flags))) goto bad; if (pgid && pgid != -1) @@ -67,11 +75,20 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i #if _lib_posix_spawn_file_actions_addtcsetpgrp_np if (tcfd >= 0) { + /* set the terminal signals to SIG_DFL in the child */ + sigemptyset(&tcmask); + sigaddset(&tcmask, SIGTTIN); + sigaddset(&tcmask, SIGTTOU); + sigaddset(&tcmask, SIGTSTP); + if (err = posix_spawnattr_setsigdefault(&attr, &tcmask)) + goto bad; + /* set the child's terminal process group */ if (err = posix_spawn_file_actions_init(&actions)) goto bad; if (err = posix_spawn_file_actions_addtcsetpgrp_np(&actions, tcfd)) goto fail; } + /* spawn the process to run the given command */ if (err = posix_spawn(&pid, path, (tcfd >= 0) ? &actions : NULL, &attr, argv, envv ? envv : environ)) #else if (err = posix_spawn(&pid, path, NULL, &attr, argv, envv ? envv : environ)) @@ -86,6 +103,7 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i #endif posix_spawnattr_destroy(&attr); return pid; + /* cleanup for different fail states */ fail: #if _lib_posix_spawn_file_actions_addtcsetpgrp_np if (tcfd >= 0) @@ -98,9 +116,8 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i return -1; } -#else - -#if _lib_spawn_mode +#elif _lib_spawn_mode +#define _fast_spawnveg 1 #include @@ -111,8 +128,8 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i #define P_DETACH _P_DETACH #endif -pid_t -spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) +static pid_t +spawnveg_fast(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) { NOT_USED(tcfd); #if defined(P_DETACH) @@ -122,9 +139,8 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i #endif } -#else - -#if _lib_spawn && _hdr_spawn && _mem_pgroup_inheritance +#elif _lib_spawn && _hdr_spawn && _mem_pgroup_inheritance +#define _fast_spawnveg 1 #include @@ -132,8 +148,8 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i * MVS OpenEdition / z/OS fork+exec+(setpgid) */ -pid_t -spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) +static pid_t +spawnveg_fast(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) { struct inheritance inherit; @@ -148,11 +164,8 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i } #else - -#include -#include -#include -#include +#define _fast_spawnveg 0 +#endif /* _lib_posix_spawn */ #if _lib_spawnve && _hdr_process #include @@ -161,12 +174,16 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i #endif #endif +#if _lib_pipe2 && O_cloexec +#define pipe(a) pipe2(a,O_cloexec) +#endif + /* * fork+exec+(setsid|setpgid) */ -pid_t -spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) +static pid_t +spawnveg_slow(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) { int n; int m; @@ -174,39 +191,47 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i pid_t rid; int err[2]; - NOT_USED(tcfd); if (!envv) envv = environ; #if _lib_spawnve - if (!pgid) + if (!pgid && tcfd < 0) return spawnve(path, argv, envv); #endif /* _lib_spawnve */ n = errno; if (pipe(err) < 0) err[0] = -1; +#if !(_lib_pipe2 && O_cloexec) else { fcntl(err[0], F_SETFD, FD_CLOEXEC); fcntl(err[1], F_SETFD, FD_CLOEXEC); } - sigcritical(SIG_REG_EXEC|SIG_REG_PROC); +#endif + sigcritical(SIG_REG_EXEC|SIG_REG_PROC|(tcfd>=0?SIG_REG_TERM:0)); pid = fork(); if (pid == -1) n = errno; else if (!pid) { + int ret; sigcritical(0); if (pgid == -1) setsid(); else if (pgid) { - m = 0; - if (pgid == 1 || pgid == -2 && (m = 1)) + if (pgid <= 1) pgid = getpid(); if (setpgid(0, pgid) < 0 && errno == EPERM) setpgid(pgid, 0); - if (m) - tcsetpgrp(2, pgid); + } + if (tcfd >= 0) + { + if(pgid == -1) + pgid = getpid(); + tcsetpgrp(tcfd, pgid); + signal(SIGTTIN,SIG_DFL); + signal(SIGTTOU,SIG_DFL); + signal(SIGTSTP,SIG_DFL); } execve(path, argv, envv); if (err[0] != -1) @@ -214,7 +239,15 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i m = errno; write(err[1], &m, sizeof(m)); } - _exit(errno == ENOENT ? EXIT_NOTFOUND : EXIT_NOEXEC); + if(errno == ENOENT) + ret = EXIT_NOTFOUND; +#ifdef ENAMETOOLONG + else if(errno == ENAMETOOLONG) + ret = EXIT_NOTFOUND; +#endif + else + ret = EXIT_NOEXEC; + _exit(ret); } rid = pid; if (err[0] != -1) @@ -254,8 +287,21 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, i return rid; } -#endif +pid_t +spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd) +{ +#if !_lib_posix_spawn_file_actions_addtcsetpgrp_np + if(tcfd >= 0) + return spawnveg_slow(path, argv, envv, pgid, tcfd); #endif - +#ifndef POSIX_SPAWN_SETSID + if(pgid == -1) + return spawnveg_slow(path, argv, envv, pgid, tcfd); #endif +#if _fast_spawnveg + return spawnveg_fast(path, argv, envv, pgid, tcfd); +#else + return spawnveg_slow(path, argv, envv, pgid, tcfd); +#endif +} diff --git a/src/lib/libast/man/spawnveg.3 b/src/lib/libast/man/spawnveg.3 index 272443bac29f..f11d20ffd270 100644 --- a/src/lib/libast/man/spawnveg.3 +++ b/src/lib/libast/man/spawnveg.3 @@ -81,30 +81,33 @@ The new process becomes a process group leader. The new process joins the process group .IR pgid . .PP -The -.L tcfd -argument is currently only used if the operating system supports the -.I posix_spawn_file_actions_addtcsetpgrp_np -function. When .L tcfd is .LR >=0 , spawnveg will set the controlling terminal for the new process to -.IR tcfd . +.IR tcfd , +and will set the terminal signals +.IR SIGTSTP , +.IR SIGTTIN , +and +.I SIGTTOU +to their defaults in the child process before +.I execve +is called. .SH CAVEATS -If the -.I posix_spawn_file_actions_addtcsetpgrp_np -function is not available, then -.L spawnveg -cannot reliably set the terminal process group. -As a result, it is incompatible with job control when used with terminals. -Additionally, if the -.L POSIX_SPAWN_SETSID -attribute is not supported, then +If +.IR posix_spawn_file_actions_addtcsetpgrp_np (3) +and/or +.I POSIX_SPAWN_SETSID +aren't available, then .L spawnveg -cannot make the new process a session leader when using the -.I posix_spawn -API. +will fall back to using +.I fork +in cases where either +.I setsid +or +.IR tcsetpgrp (3) +is required. .SH "SEE ALSO" -fork(2), posix_spawn(3), exec(2), setpgid(2), setsid(2), spawnve(2) +fork(2), posix_spawn(3), exec(2), setpgid(2), setsid(2), sigaction(2), spawnve(2), tcsetpgrp(3) From 4217c9ec670bba9de5bfbee4709bc5ac8f02d707 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Mon, 16 Jun 2025 16:05:07 -0700 Subject: [PATCH 2/4] rm vestigal extra mask --- src/lib/libast/comp/spawnveg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/libast/comp/spawnveg.c b/src/lib/libast/comp/spawnveg.c index 29bff9f5d0cf..3907df4bc4ec 100644 --- a/src/lib/libast/comp/spawnveg.c +++ b/src/lib/libast/comp/spawnveg.c @@ -46,8 +46,8 @@ spawnveg_fast(const char* path, char* const argv[], char* const envv[], pid_t pg short flags = 0; pid_t pid; posix_spawnattr_t attr; - sigset_t mask, tcmask; #if _lib_posix_spawn_file_actions_addtcsetpgrp_np + sigset_t tcmask; posix_spawn_file_actions_t actions; #else NOT_USED(tcfd); From ba84b5239a522c0af40fc1daacc7578841981f0c Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Mon, 16 Jun 2025 19:59:37 -0700 Subject: [PATCH 3/4] Bump copyright date; add NEWS entry --- NEWS | 5 +++++ src/cmd/ksh93/include/version.h | 2 +- src/lib/libast/comp/spawnveg.c | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 81c063c046d0..3c6b494361a2 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,11 @@ This documents significant changes in the dev branch of ksh 93u+m. For full details, see the git log at: https://github.com/ksh93/ksh Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library. +2025-06-16: + +- Fixed a regression occurring on systems with glibc 2.35+ that could + cause the pty regression tests to crash or lockup. + 2025-06-14: - Fixed a bug occurring on systems with posix_spawn(3), spawnve(2), and diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index b4776a335394..51d98331ba28 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -18,7 +18,7 @@ #include #include "git.h" -#define SH_RELEASE_DATE "2025-06-14" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2025-06-16" /* must be in this format for $((.sh.version)) */ /* * This comment keeps SH_RELEASE_DATE a few lines away from SH_RELEASE_SVER to avoid * merge conflicts when cherry-picking dev branch commits onto a release branch. diff --git a/src/lib/libast/comp/spawnveg.c b/src/lib/libast/comp/spawnveg.c index 3907df4bc4ec..98c893a96ef9 100644 --- a/src/lib/libast/comp/spawnveg.c +++ b/src/lib/libast/comp/spawnveg.c @@ -2,7 +2,7 @@ * * * This software is part of the ast package * * Copyright (c) 1985-2012 AT&T Intellectual Property * -* Copyright (c) 2020-2024 Contributors to ksh 93u+m * +* Copyright (c) 2020-2025 Contributors to ksh 93u+m * * and is licensed under the * * Eclipse Public License, Version 2.0 * * * From 765f7acbdb869ce1ac47900140c83f11b7682b09 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Mon, 16 Jun 2025 20:22:30 -0700 Subject: [PATCH 4/4] Add missing if directive --- src/lib/libast/comp/spawnveg.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lib/libast/comp/spawnveg.c b/src/lib/libast/comp/spawnveg.c index 98c893a96ef9..0fff52058719 100644 --- a/src/lib/libast/comp/spawnveg.c +++ b/src/lib/libast/comp/spawnveg.c @@ -61,8 +61,10 @@ spawnveg_fast(const char* path, char* const argv[], char* const envv[], pid_t pg #endif if (pgid && pgid != -1) flags |= POSIX_SPAWN_SETPGROUP; +#if _lib_posix_spawn_file_actions_addtcsetpgrp_np if (tcfd >= 0) flags |= POSIX_SPAWN_SETSIGDEF; +#endif if (flags && (err = posix_spawnattr_setflags(&attr, flags))) goto bad; if (pgid && pgid != -1)