From af6a32d14f3a9c732bf494bd411f205c5142f634 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Mon, 3 May 2021 04:03:46 +0100 Subject: [PATCH] Fix $RANDOM to act consistently in subshells (#294) This fixes the following: 1. Using $RANDOM in a virtual/non-forked subshell no longer influences the reproducible $RANDOM sequence in the parent environment. 2. When invoking a subshell $RANDOM is now re-seeded (as mksh and bash do) so that invocations in repeated subshells (including forked subshells) longer produce identical sequences by default. 3. Program flow corruption that occurred in scripts on executing ( ( simple_command & ) ). src/cmd/ksh93/include/variables.h: - Move 'struct rand' here as it will be needed in subshell.c. Add rand_seed member to save the pseudorandom generator seed. Remove the pointer to the shell state as it's redundant. src/cmd/ksh93/sh/init.c: - put_rand(): Store given seed in rand_seed while calling srand(). No longer pointlessly limit the number of possible seeds with the RANDMASK bitmask (that mask is to limit the values to 0-32767, it should not limit the number of possible sequences to 32768). - nget_rand(): Instead of using rand(), use rand_r() to update the random_seed value. This makes it possible to save/restore the current seed of the pseudorandom generator. - Add sh_reseed_rand() function that reseeds the pseudorandom generator by calling srand() with a bitwise-xor combination of the current PID, the current time with a granularity of 1/10000 seconds, and a sequence number that is increased on each invocation. - nv_init(): Set the initial seed using sh_reseed_rand() here instead of in sh_main(), as this is where the other struct rand members are initialised. src/cmd/ksh93/sh/main.c: sh_main(): - Remove the srand() call that was replaced by the sh_reseed_rand() call in init.c. src/cmd/ksh93/sh/subshell.c: sh_subshell(): - Upon entering a virtual subshell, save the current $RANDOM seed and state, then reseed $RANDOM for the subshell. - Upon exiting a virtual subshell, restore $RANDOM seed and state and reseed the generator using srand() with the restored seed. src/cmd/ksh93/sh/xec.c: sh_exec(): - When optimizing out a subshell that is the last command, still act like a subshell: reseed $RANDOM and increase ${.sh.subshell}. - Fix a separate bug discovered while implementing this. Do not optimize '( simple_command & )' when in a virtual subshell; doing this causes program flow corruption. - When optimizing '( simple_command & )', also reseed $RANDOM and increment ${.sh.subshell}. src/cmd/ksh93/tests/subshell.sh, src/cmd/ksh93/tests/variables.sh: - Add various tests for all of the above. Co-authored-by: Johnothan King Resolves: https://github.com/ksh93/ksh/issues/285 --- NEWS | 9 ++++ src/cmd/ksh93/COMPATIBILITY | 6 +++ src/cmd/ksh93/Mamfile | 1 + src/cmd/ksh93/include/variables.h | 10 +++++ src/cmd/ksh93/sh/init.c | 29 +++++++------ src/cmd/ksh93/sh/main.c | 2 - src/cmd/ksh93/sh/subshell.c | 12 ++++++ src/cmd/ksh93/sh/xec.c | 9 +++- src/cmd/ksh93/tests/subshell.sh | 9 ++++ src/cmd/ksh93/tests/variables.sh | 69 ++++++++++++++++++++++++++++++- 10 files changed, 139 insertions(+), 17 deletions(-) diff --git a/NEWS b/NEWS index 9d21d6a4daf8..047b3c3a34df 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,15 @@ Any uppercase BUG_* names are modernish shell bug IDs. 2021-05-03: +- Subshells (even if non-forked) now keep a properly separated state of the + pseudorandom generator used for $RANDOM, so that using $RANDOM in a + non-forked subshell no longer influences a reproducible $RANDOM sequence in + the parent environment. In addition, upon invoking a subshell, $RANDOM is now + reseeded (as mksh and bash do). + +- Fixed program flow corruption that occurred in scripts on executing a + background job in a nested subshell, as in ( ( simple_command & ) ). + - Completed the 2021-04-30 fix for ${var'{}'} where is '-', '+', ':-' or ':+' by fixing a bug that caused an extra '}' to be output. diff --git a/src/cmd/ksh93/COMPATIBILITY b/src/cmd/ksh93/COMPATIBILITY index a4e1c7705adf..e1c180f1c1d4 100644 --- a/src/cmd/ksh93/COMPATIBILITY +++ b/src/cmd/ksh93/COMPATIBILITY @@ -132,6 +132,12 @@ For more details, see the NEWS file and for complete details, see the git log. 24. The readonly attribute of ksh variables is no longer imported from or exported to other ksh shell instances through the environment. +25. Subshells (even if non-forked) now keep a properly separated state + of the pseudorandom generator used for $RANDOM, so that using + $RANDOM in a non-forked subshell no longer influences a reproducible + $RANDOM sequence in the parent environment. In addition, upon + invoking a subshell, $RANDOM is now reseeded (as mksh and bash do). + ____________________________________________________________________________ KSH-93 VS. KSH-88 diff --git a/src/cmd/ksh93/Mamfile b/src/cmd/ksh93/Mamfile index c8207ff4c3e7..5490c6b59ab4 100644 --- a/src/cmd/ksh93/Mamfile +++ b/src/cmd/ksh93/Mamfile @@ -311,6 +311,7 @@ make install prev FEATURE/dynamic implicit prev FEATURE/options implicit prev ${PACKAGE_ast_INCLUDE}/option.h implicit + prev include/nval.h implicit done include/variables.h prev ${PACKAGE_ast_INCLUDE}/error.h implicit prev ${PACKAGE_ast_INCLUDE}/stak.h implicit diff --git a/src/cmd/ksh93/include/variables.h b/src/cmd/ksh93/include/variables.h index f045d3518618..3c55d426039a 100644 --- a/src/cmd/ksh93/include/variables.h +++ b/src/cmd/ksh93/include/variables.h @@ -25,6 +25,16 @@ #include #include "FEATURE/options" #include "FEATURE/dynamic" +#include + +/* used for RANDNOD ($RANDOM) */ +struct rand +{ + Namfun_t hdr; + unsigned int rand_seed; + int32_t rand_last; +}; +extern void sh_reseed_rand(struct rand *); /* The following defines must be kept synchronous with shtab_variables[] in data/variables.c */ diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c index 1c450eaa71ee..5b7dcdcff05d 100644 --- a/src/cmd/ksh93/sh/init.c +++ b/src/cmd/ksh93/sh/init.c @@ -34,6 +34,7 @@ #include #include #include +#include #include "variables.h" #include "path.h" #include "fault.h" @@ -142,13 +143,6 @@ struct seconds Shell_t *sh; }; -struct rand -{ - Namfun_t hdr; - Shell_t *sh; - int32_t rand_last; -}; - struct ifs { Namfun_t hdr; @@ -658,8 +652,8 @@ static void put_rand(register Namval_t* np,const char *val,int flags,Namfun_t *f if(flags&NV_INTEGER) n = *(double*)val; else - n = sh_arith(rp->sh,val); - srand((int)(n&RANDMASK)); + n = sh_arith(&sh,val); + srand(rp->rand_seed = (unsigned int)n); rp->rand_last = -1; if(!np->nvalue.lp) np->nvalue.lp = &rp->rand_last; @@ -671,10 +665,11 @@ static void put_rand(register Namval_t* np,const char *val,int flags,Namfun_t *f */ static Sfdouble_t nget_rand(register Namval_t* np, Namfun_t *fp) { + struct rand *rp = (struct rand*)fp; register long cur, last= *np->nvalue.lp; NOT_USED(fp); do - cur = (rand()>>rand_shift)&RANDMASK; + cur = (rand_r(&rp->rand_seed)>>rand_shift)&RANDMASK; while(cur==last); *np->nvalue.lp = cur; return((Sfdouble_t)cur); @@ -686,6 +681,17 @@ static char* get_rand(register Namval_t* np, Namfun_t *fp) return(fmtbase(n, 10, 0)); } +void sh_reseed_rand(struct rand *rp) +{ + struct tms tp; + unsigned int time; + static unsigned int seq; + timeofday(&tp); + time = (unsigned int)remainder(dtime(&tp) * 10000.0, (double)UINT_MAX); + srand(rp->rand_seed = shgd->current_pid ^ time ^ ++seq); + rp->rand_last = -1; +} + /* * These three routines are for LINENO */ @@ -1748,7 +1754,6 @@ static Init_t *nv_init(Shell_t *shp) ip->SECONDS_init.hdr.nofree = 1; ip->RAND_init.hdr.disc = &RAND_disc; ip->RAND_init.hdr.nofree = 1; - ip->RAND_init.sh = shp; ip->SH_MATCH_init.hdr.disc = &SH_MATCH_disc; ip->SH_MATCH_init.hdr.nofree = 1; ip->SH_MATH_init.disc = &SH_MATH_disc; @@ -1793,8 +1798,8 @@ static Init_t *nv_init(Shell_t *shp) nv_stack(L_ARGNOD, &ip->L_ARG_init); nv_putval(SECONDS, (char*)&d, NV_DOUBLE); nv_stack(RANDNOD, &ip->RAND_init.hdr); - d = (shp->gd->pid&RANDMASK); nv_putval(RANDNOD, (char*)&d, NV_DOUBLE); + sh_reseed_rand((struct rand *)RANDNOD->nvfun); nv_stack(LINENO, &ip->LINENO_init); SH_MATCHNOD->nvfun = &ip->SH_MATCH_init.hdr; nv_putsub(SH_MATCHNOD,(char*)0,10); diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c index c24379795ec2..c23b7fdbe0cb 100644 --- a/src/cmd/ksh93/sh/main.c +++ b/src/cmd/ksh93/sh/main.c @@ -149,8 +149,6 @@ int sh_main(int ac, char *av[], Shinit_f userinit) } shp->fn_depth = shp->dot_depth = 0; command = error_info.id; - /* set pidname '$$' */ - srand(shp->gd->pid&0x7fff); if(nv_isnull(PS4NOD)) nv_putval(PS4NOD,e_traceprompt,NV_RDONLY); path_pwd(shp,1); diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c index c3196251fbb3..1cc96d388951 100644 --- a/src/cmd/ksh93/sh/subshell.c +++ b/src/cmd/ksh93/sh/subshell.c @@ -488,6 +488,9 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) struct sh_scoped savst; struct dolnod *argsav=0; int argcnt; + struct rand *rp; /* current $RANDOM discipline function data */ + unsigned int save_rand_seed; /* parent shell $RANDOM seed */ + int save_rand_last; /* last random number from $RANDOM in parent shell */ memset((char*)sp, 0, sizeof(*sp)); sfsync(shp->outpool); sh_sigcheck(shp); @@ -601,6 +604,11 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) sp->cpipe = shp->cpipe[1]; shp->cpid = 0; sh_sigreset(0); + /* save the current $RANDOM seed and state; reseed $RANDOM */ + rp = (struct rand*)RANDNOD->nvfun; + save_rand_seed = rp->rand_seed; + save_rand_last = rp->rand_last; + sh_reseed_rand(rp); } jmpval = sigsetjmp(buff.buff,0); if(jmpval==0) @@ -856,6 +864,10 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) shp->cpid = sp->cpid; shp->cpipe[1] = sp->cpipe; shp->coutpipe = sp->coutpipe; + /* restore $RANDOM seed and state */ + rp = (struct rand*)RANDNOD->nvfun; + srand(rp->rand_seed = save_rand_seed); + rp->rand_last = save_rand_last; } shp->subshare = sp->subshare; shp->subdup = sp->subdup; diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index 1c0ebfca8930..e8913d81fb2c 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -1937,6 +1937,7 @@ int sh_exec(register const Shnode_t *t, int flags) flags &= ~OPTIMIZE_FLAG; if(!shp->subshell && !shp->st.trapcom[0] && !shp->st.trap[SH_ERRTRAP] && (flags&sh_state(SH_NOFORK))) { + /* This is the last command, so avoid creating a subshell */ char *savsig; int nsig,jmpval; struct checkpt *buffp = (struct checkpt*)stkalloc(shp->stk,sizeof(struct checkpt)); @@ -1948,6 +1949,9 @@ int sh_exec(register const Shnode_t *t, int flags) memcpy(savsig,(char*)&shp->st.trapcom[0],nsig); shp->st.otrapcom = (char**)savsig; } + /* Still act like a subshell: reseed $RANDOM and increment ${.sh.subshell} */ + sh_reseed_rand((struct rand*)RANDNOD->nvfun); + shgd->realsubshell++; sh_sigreset(0); sh_pushcontext(shp,buffp,SH_JMPEXIT); jmpval = sigsetjmp(buffp->buff,0); @@ -1961,7 +1965,7 @@ int sh_exec(register const Shnode_t *t, int flags) sh_done(shp,0); } else if(((type=t->par.partre->tre.tretyp)&FAMP) && ((type&COMMSK)==TFORK) - && !sh_isoption(SH_INTERACTIVE)) + && !job.jobcontrol && !shp->subshell) { /* Optimize '( simple_command & )' */ pid_t pid; @@ -1970,7 +1974,8 @@ int sh_exec(register const Shnode_t *t, int flags) _sh_fork(shp,pid,0,0); if(pid==0) { - shgd->current_pid = getpid(); + sh_reseed_rand((struct rand*)RANDNOD->nvfun); + shgd->realsubshell++; sh_exec(t->par.partre,flags); shp->st.trapcom[0]=0; sh_done(shp,0); diff --git a/src/cmd/ksh93/tests/subshell.sh b/src/cmd/ksh93/tests/subshell.sh index 85ea3e928e17..edba4a8ebbf3 100755 --- a/src/cmd/ksh93/tests/subshell.sh +++ b/src/cmd/ksh93/tests/subshell.sh @@ -1034,5 +1034,14 @@ got=$(_AST_FEATURES="TEST_TMP_VAR - $$" "$SHELL" -c '(d=${ builtin getconf;}); g got=$(ulimit -t unlimited 2>/dev/null; (dummy=${ exec true; }); echo ok) [[ $got == ok ]] || err_exit "'exec' command run in subshare disregards parent virtual subshell" +# ====== +# https://github.com/ksh93/ksh/pull/294#discussion_r624627501 +exp='this should be run once' +$SHELL -c '( ( : & ) ); echo "this should be run once"' >r624627501.out +sleep .01 +got=$( /dev/null + print $RANDOM +} +integer rand1=$(rand_print) +integer rand2=$(rand_print) +(( rand1 == rand2 )) && err_exit "Test 1: \$RANDOM seed in subshell doesn't change" \ + "(both results are $rand1)" +# Make sure we're actually using a different pseudorandom seed +integer rand1=$( + ulimit -t unlimited 2> /dev/null + test $RANDOM + print $RANDOM +) +integer rand2=${ print $RANDOM ;} +(( rand1 == rand2 )) && err_exit "Test 2: \$RANDOM seed in subshell doesn't change" \ + "(both results are $rand1)" +# $RANDOM should be reseeded when the final command is inside of a subshell +rand1=$($SHELL -c 'RANDOM=1; (echo $RANDOM)') +rand2=$($SHELL -c 'RANDOM=1; (echo $RANDOM)') +(( rand1 == rand2 )) && err_exit "Test 3: \$RANDOM seed in subshell doesn't change" \ + "(both results are $rand1)" +# $RANDOM should be reseeded for the ( simple_command & ) optimization +( echo $RANDOM & ) >r1 +( echo $RANDOM & ) >r2 +sleep .01 +(( $(128)) && print -n / && kill -l "$e"))" +# ... unset followed by launching a forked subshell +$SHELL -c ' + errors=0 + unset -v "$@" || let errors++ + ( + ulimit -t unlimited 2>/dev/null + for var do + [[ $var == _ ]] && continue # only makes sense that $_ is immediately set again + [[ -v $var ]] && let errors++ + done + exit $((errors + 1)) + ) + exit $? +' unset_to_fork_test "$@" +(((e = $?) == 1)) || err_exit "Failure in unsetting one or more special variables followed by launching forked subshell" \ + "(exit status $e$( ((e>128)) && print -n / && kill -l "$e"))" + # ====== # ${.sh.pid} should be the forked subshell's PID (