Skip to content

Commit

Permalink
Fix crash upon running many subshells (#113)
Browse files Browse the repository at this point in the history
Co-authored-by: Martijn Dekker <martijn@inlv.org>

An intermittent crash occurred after running many thousands of
virtual/non-forked subshells. One reproducer is a crash in the
shbench fibonacci.ksh test, as documented here:
https://github.com/ksh-community/shbench/blob/f3d9e134/bench/fibonacci.ksh#L4-L10

The apparent cause was the signed and insufficiently large 'short'
data type of 'curenv' and related variables which wrapped around to
a negative number when overflowing. These IDs are necessary for the
'wait' builtin to obtain the exit status from a background job.

This fix is inspired by a patch based on ksh 93v-:
https://build.opensuse.org/package/view_file/shells/ksh/ksh93-longenv.dif?expand=1
https://src.fedoraproject.org/rpms/ksh/blob/f24/f/ksh-20130628-longer.patch

However, we change the type to 'unsigned int' instead of 'long'. On
all remotely modern systems, ints are 32-bit values, and using this
type avoids a performance degradation on 32-bit sytems. Making them
unsigned prevents an overflow to negative values.

src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/include/jobs.h,
src/cmd/ksh93/include/nval.h,
src/cmd/ksh93/include/shell.h:
- Change the types of the static global 'subenv' and the subshell
  structure members 'curenv', 'jobenv', 'subenv', 'p_env' and
  'subshell' to one consistent type, unsigned int.

src/cmd/ksh93/sh/jobs.c,
src/cmd/ksh93/sh/macro.c:
src/cmd/ksh93/sh/name.c:
src/cmd/ksh93/sh/nvtype.c,
src/cmd/ksh93/sh/subshell.c:
- Updates to match new variable types.

src/cmd/ksh93/tests/subshell.sh:
- Show wrong exit status in message on failure of 'wait' builtin.
  • Loading branch information
JohnoKing committed Aug 12, 2020
1 parent f485fe0 commit 05ac1db
Show file tree
Hide file tree
Showing 13 changed files with 25 additions and 17 deletions.
4 changes: 4 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ For full details, see the git log at: https://github.com/ksh93/ksh

Any uppercase BUG_* names are modernish shell bug IDs.

2020-08-11:

- Fixed an intermittent crash upon running a large number of subshells.

2020-08-10:

- A number of fixes have been applied to the printf formatting directives
Expand Down
4 changes: 2 additions & 2 deletions src/cmd/ksh93/include/defs.h
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,8 @@ struct shared
Namval_t *prev_table; /* previous table used in nv_open */ \
Sfio_t *outpool; /* output stream pool */ \
long timeout; /* read timeout */ \
short curenv; /* current subshell number */ \
short jobenv; /* subshell number for jobs */ \
unsigned int curenv; /* current subshell number */ \
unsigned int jobenv; /* subshell number for jobs */ \
int infd; /* input file descriptor */ \
short nextprompt; /* next prompt is PS<nextprompt> */ \
short poolfiles; \
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/jobs.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ struct process
unsigned short p_exit; /* exit value or signal number */
unsigned short p_exitmin; /* minimum exit value for xargs */
unsigned short p_flag; /* flags - see below */
int p_env; /* subshell environment number */
unsigned int p_env; /* subshell environment number */
#ifdef JOBS
off_t p_name; /* history file offset for command */
struct termios p_stty; /* terminal state for job */
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/nval.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ struct Namfun
{
const Namdisc_t *disc;
char nofree;
unsigned char subshell;
unsigned int subshell;
uint32_t dsize;
Namfun_t *next;
char *last;
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/shell.h
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ struct Shell_s
int exitval; /* most recent exit value */
unsigned char trapnote; /* set when trap/signal is pending */
char shcomp; /* set when running shcomp */
short subshell; /* set for virtual subshell */
unsigned int subshell; /* set for virtual subshell */
#ifdef _SH_PRIVATE
_SH_PRIVATE
#endif /* _SH_PRIVATE */
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 @@ -17,4 +17,4 @@
* David Korn <dgk@research.att.com> *
* *
***********************************************************************/
#define SH_RELEASE "93u+m 2020-08-10"
#define SH_RELEASE "93u+m 2020-08-11"
3 changes: 2 additions & 1 deletion src/cmd/ksh93/sh/init.c
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,8 @@ void sh_setmatch(Shell_t *shp,const char *v, int vsize, int nmatch, regoff_t mat
{
struct match *mp = &ip->SH_MATCH_init;
Namval_t *np = nv_namptr(mp->node,0);
register int i,n,x, savesub=shp->subshell;
register int i,n,x;
unsigned int savesub = shp->subshell;
Namarr_t *ap = nv_arrayptr(SH_MATCHNOD);
shp->subshell = 0;
#ifndef SHOPT_2DMATCH
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/sh/jobs.c
Original file line number Diff line number Diff line change
Expand Up @@ -1647,7 +1647,7 @@ static struct process *job_unpost(register struct process *pwtop,int notify)
register struct process *pw;
/* make sure all processes are done */
#ifdef DEBUG
sfprintf(sfstderr,"ksh: job line %4d: drop pid=%d critical=%d pid=%d env=%d\n",__LINE__,getpid(),job.in_critical,pwtop->p_pid,pwtop->p_env);
sfprintf(sfstderr,"ksh: job line %4d: drop pid=%d critical=%d pid=%d env=%u\n",__LINE__,getpid(),job.in_critical,pwtop->p_pid,pwtop->p_env);
sfsync(sfstderr);
#endif /* DEBUG */
pwtop = pw = job_byjid((int)pwtop->p_job);
Expand Down
5 changes: 3 additions & 2 deletions src/cmd/ksh93/sh/macro.c
Original file line number Diff line number Diff line change
Expand Up @@ -2716,6 +2716,7 @@ static char *sh_tilde(Shell_t *shp,register const char *string)
register int c;
register struct passwd *pw;
register Namval_t *np=0;
unsigned int save;
static Dt_t *logins_tree;
if(*string++!='~')
return(NIL(char*));
Expand Down Expand Up @@ -2771,10 +2772,10 @@ static char *sh_tilde(Shell_t *shp,register const char *string)
logins_tree = dtopen(&_Nvdisc,Dtbag);
if(np=nv_search(string,logins_tree,NV_ADD))
{
c = shp->subshell;
save = shp->subshell;
shp->subshell = 0;
nv_putval(np, pw->pw_dir,0);
shp->subshell = c;
shp->subshell = save;
}
return(pw->pw_dir);
}
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/sh/name.c
Original file line number Diff line number Diff line change
Expand Up @@ -2464,7 +2464,7 @@ static void table_unset(Shell_t *shp, register Dt_t *root, int flags, Dt_t *oroo
{
if(nv_cover(nq))
{
int subshell = shp->subshell;
unsigned int subshell = shp->subshell;
shp->subshell = 0;
if(nv_isattr(nq, NV_INTEGER))
{
Expand Down
3 changes: 2 additions & 1 deletion src/cmd/ksh93/sh/nvtype.c
Original file line number Diff line number Diff line change
Expand Up @@ -1318,7 +1318,8 @@ int nv_settype(Namval_t* np, Namval_t *tp, int flags)
char *val=0;
Namarr_t *ap=0;
Shell_t *shp = sh_getinterp();
int nelem=0,subshell=shp->subshell;
int nelem = 0;
unsigned int subshell = shp->subshell;
#if SHOPT_TYPEDEF
Namval_t *tq;
if(nv_type(np)==tp)
Expand Down
8 changes: 4 additions & 4 deletions src/cmd/ksh93/sh/subshell.c
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ static struct subshell
char pwdclose;
} *subshell_data;

static int subenv;
static unsigned int subenv;


/*
Expand Down Expand Up @@ -176,7 +176,7 @@ void sh_subfork(void)
{
register struct subshell *sp = subshell_data;
Shell_t *shp = sp->shp;
int curenv = shp->curenv;
unsigned int curenv = shp->curenv;
pid_t pid;
char *trap = shp->st.trapcom[0];
if(trap)
Expand Down Expand Up @@ -251,7 +251,7 @@ Namval_t *sh_assignok(register Namval_t *np,int add)
Dt_t *dp= shp->var_tree;
Namval_t *mpnext;
Namarr_t *ap;
int save;
unsigned int save;
/* don't bother to save if in a ${ subshare; } */
if(sp->subshare)
return(np);
Expand Down Expand Up @@ -456,7 +456,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
struct subshell sub_data;
register struct subshell *sp = &sub_data;
int jmpval,nsig=0,duped=0;
int savecurenv = shp->curenv;
unsigned int savecurenv = shp->curenv;
int savejobpgid = job.curpgid;
int *saveexitval = job.exitval;
char *savsig;
Expand Down
3 changes: 2 additions & 1 deletion src/cmd/ksh93/tests/subshell.sh
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,8 @@ err() { return $1; }
( err 12 ) & pid=$!
: $( $date)
wait $pid
[[ $? == 12 ]] || err_exit 'exit status from subshells not being preserved'
exit_status=$?
[[ $exit_status == 12 ]] || err_exit "exit status from subshells not being preserved (expected 12, got $exit_status)"

if cat /dev/fd/3 3</dev/null >/dev/null 2>&1 || whence mkfifo > /dev/null
then x="$(sed 's/^/Hello /' <(print "Fred" | sort))"
Expand Down

0 comments on commit 05ac1db

Please sign in to comment.