Skip to content

Overhaul spawnveg to use clone(2) and fix many related spawning bugs #888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,18 @@ 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-07-03:

- Fixed a regression introduced on 02-02-2022 occurring on systems with
glibc 2.35+ that could cause the pty regression tests to crash or lockup.

- Fixed a regression introduced on 02-02-2022 that could cause ksh to crash
after a command fails with E2BIG.

- Fixed a bug that caused a file descriptor leak when a command run
with the 'command' builtin couldn't be executed due to an error
(e.g. ENOENT or E2BIG).

2025-06-14:

- Fixed a bug occurring on systems with posix_spawn(3), spawnve(2), and
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#include <ast_release.h>
#include "git.h"

#define SH_RELEASE_DATE "2025-06-22" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2025-07-03" /* 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.
Expand Down
36 changes: 18 additions & 18 deletions src/cmd/ksh93/sh/path.c
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ static pid_t _spawnveg(const char *path, char* const argv[], char* const envp[],
if(pid>=0 || errno!=EAGAIN)
break;
}
if(pid<0 && job.jobcontrol)
tcsetpgrp(job.fd,sh.pid); /* if spawnveg set tcpgrp, we must restore it ourselves */
return pid;
}

Expand Down Expand Up @@ -993,15 +995,20 @@ noreturn void path_exec(const char *arg0,char *argv[],struct argnod *local)
if(sh.subshell)
sh_subtmpfile();
spawnpid = path_spawn(opath,argv,envp,libpath,0);
if(spawnpid==-1 && sh.path_err!=ENOENT)
if(spawnpid==-1)
{
/*
* A command was found but it couldn't be executed.
* POSIX specifies that the shell should continue to search for the
* command in PATH and return 126 only when it can't find an executable
* file in other elements of PATH.
*/
not_executable = sh.path_err;
if(sh.path_err == E2BIG)
break;
else if(sh.path_err!=ENOENT)
{
/*
* A command was found but it couldn't be executed.
* POSIX specifies that the shell should continue to search for the
* command in PATH and return 126 only when it can't find an executable
* file in other elements of PATH.
*/
not_executable = sh.path_err;
}
}
while(pp && (pp->flags&PATH_FPATH))
pp = path_nextcomp(pp,arg0,0);
Expand Down Expand Up @@ -1258,13 +1265,6 @@ pid_t path_spawn(const char *opath,char **argv, char **envp, Pathcomp_t *libpath
case EPERM:
sh.path_err = errno;
return -1;
case ENOTDIR:
case ENOENT:
case EINTR:
#ifdef EMLINK
case EMLINK:
#endif /* EMLINK */
return -1;
case E2BIG:
if(sh_isstate(SH_XARG))
{
Expand All @@ -1282,10 +1282,10 @@ pid_t path_spawn(const char *opath,char **argv, char **envp, Pathcomp_t *libpath
}
/* FALLTHROUGH */
default:
errormsg(SH_DICT,ERROR_system(ERROR_NOEXEC),e_exec,path);
UNREACHABLE();
sh.path_err = errno;
return -1;
}
return 0;
UNREACHABLE();
}

/*
Expand Down
79 changes: 17 additions & 62 deletions src/cmd/ksh93/sh/xec.c
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,6 @@
# include <sys/resource.h>
#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 */
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
{
Expand All @@ -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 */
Expand Down Expand Up @@ -3340,22 +3331,13 @@ 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)
grp = 1;
else
grp = job.curpgid;
}
#endif /* _use_ntfork_tcpgrp */

sfsync(NULL);
sigreset(0); /* set signals to ignore */
Expand All @@ -3368,35 +3350,19 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int topfd)
fail:
if(jobfork && spawnpid<0)
job_fork(-2);
if(spawnpid == -1)
if(spawnpid==-1) switch(errno=sh.path_err)
{
#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 */
switch(errno=sh.path_err)
{
case ENOENT:
errormsg(SH_DICT,ERROR_exit(ERROR_NOENT),e_found+4);
UNREACHABLE();
case ENOENT:
errormsg(SH_DICT,ERROR_exit(ERROR_NOENT),e_found+4);
UNREACHABLE();
#ifdef ENAMETOOLONG
case ENAMETOOLONG:
errormsg(SH_DICT,ERROR_exit(ERROR_NOENT),e_toolong+4);
UNREACHABLE();
case ENAMETOOLONG:
errormsg(SH_DICT,ERROR_exit(ERROR_NOENT),e_toolong+4);
UNREACHABLE();
#endif
default:
errormsg(SH_DICT,ERROR_system(ERROR_NOEXEC),e_exec+4);
UNREACHABLE();
}
default:
errormsg(SH_DICT,ERROR_system(ERROR_NOEXEC),e_exec+4);
UNREACHABLE();
}
job_unlock();
}
Expand All @@ -3405,17 +3371,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)
Expand All @@ -3425,7 +3380,7 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int topfd)
if(jmpval==SH_JMPSCRIPT)
nv_setlist(t->com.comset,NV_EXPORT|NV_IDENT|NV_ASSIGN,0);
}
if(t->com.comio && (jmpval || spawnpid<=0) && sh.topfd > topfd)
if((t->com.comio || spawnpid<0) && jmpval && sh.topfd > topfd)
sh_iorestore(topfd,jmpval);
if(jmpval>SH_JMPCMD)
siglongjmp(*sh.jmplist,jmpval);
Expand Down
61 changes: 55 additions & 6 deletions src/cmd/ksh93/tests/io.sh
Original file line number Diff line number Diff line change
Expand Up @@ -696,16 +696,65 @@ err=$(
"(got $(printf %q "$err"))"

# File descriptor leak after 'command not found' with process substitution as argument
err=$(
ulimit -n 25 || exit 0
exp=127
expout=$(
set +x
PATH=/dev/null
for ((i=1; i<10; i++))
do notfound <(:) >(:) 2> /dev/null
LINENO=1
for ((i=1; i<20; i++))
do notfound
done 2>&1
exit 0
) || err_exit "Process substitution leaks file descriptors when used as argument to nonexistent command" \
"(got $(printf %q "$err"))"
)
gotout=$(
ulimit -n 18 || exit 0
set +x
PATH=/dev/null
LINENO=1
for ((i=1; i<20; i++))
do notfound <(:) >(:)
done 2>&1
exit
)
got=$?
((got==exp)) || err_exit "Process substitution leaks file descriptors when used as argument to nonexistent command" \
"(expected exit status $exp, got status $got)"
[[ $expout == "$gotout" ]] || err_exit "Process substitution leaks file descriptors when used as argument to nonexistent command" \
$'Diff follows:\n'"$(diff -u <(print -r -- "$expout") <(print -r -- "$gotout"))"

# Same as above, but also test commands executed with command(1)
gotout=$(
ulimit -n 18 || exit 0
set +x
PATH=/dev/null
LINENO=1
for ((i=1; i<20; i++))
do command notfound <(:) >(:)
done 2>&1
exit
)
got=$?
((got==exp)) || err_exit "Process substitution leaks file descriptors when used as argument to nonexistent command run with command(1)" \
"(expected exit status $exp, got status $got)"
[[ $expout == "$gotout" ]] || err_exit "Process substitution leaks file descriptors when used as argument to nonexistent command run with command(1)" \
$'Diff follows:\n'"$(diff -u <(print -r -- "$expout") <(print -r -- "$gotout"))"

# Now test with command -x
gotout=$(
ulimit -n 18 || exit 0
set +x
PATH=/dev/null
LINENO=1
for ((i=1; i<20; i++))
do command -x notfound <(:) >(:)
done 2>&1
exit
)
got=$?
((got==exp)) || err_exit "Process substitution leaks file descriptors when used as argument to nonexistent command run with 'command -x'" \
"(expected exit status $exp, got status $got)"
[[ $expout == "$gotout" ]] || err_exit "Process substitution leaks file descriptors when used as argument to nonexistent command run with 'command -x'" \
$'Diff follows:\n'"$(diff -u <(print -r -- "$expout") <(print -r -- "$gotout"))"

got=$(command -x cat <(command -x echo foo) 2>&1) || err_exit "process substitution doesn't work with 'command'" \
"(got $(printf %q "$got"))"
Expand Down
Loading