Skip to content

Commit

Permalink
Do not call unset disc. upon subshell cleanup or running a script
Browse files Browse the repository at this point in the history
Reproducer:

    $ ksh -c 'x.unset() { echo UNSET; }; (x=1); echo end'
    UNSET
    end

The unset disciplien function is called while cleaning up the 'x'
variable that was set in a subshell.

Yet, if we cause the subshell to run in a separate process, that
doesn't happen (note: 'ulimit' forces a virtual subshell to fork):

    $ ksh -c 'x.unset() { echo UNSET; }; (ulimit -c 0; x=1); echo end'
    end

...and that is the correct behaviour. `x` was never explicitly unset,
it's just that the subshell terminated. So it's bogus that the
unset discipline function got called. Virtual and real subshells
should act identically (and, in this case, identically to main
shells as well).

Cleanup of subshell variables upon termination of a virtual
subshell is done by nv_restore(). It sets a subshell_noscope flag
to stop sh_assignok() from creating a virtual subshell scope during
cleanup. This should stop assign() in nvdisc.c from executing an
unset discipline as well.

More bogosity (run the reproducer from this source tree):

    $ x.unset() { echo UNSET; }
    $ bin/package
    UNSET
    darwin.arm64-64

This happens when running a script without a #! path, which
involves forking and then calling sh_reinit(), which unsets all the
variables. Unset disciplines should be blocked there as well.

src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/sh/subshell.c:
- Add sh.nv_restore flag, to be set while nv_restore() is running.
  This now replaces subshell_noscope.

src/cmd/ksh93/sh/nvdisc.c: assign():
- Refuse to run an unset discipline (i.e., assign() is called with
  val==NULL) if sh.nv_restore is set, or if the SH_INIT state flag
  is set (i.e. if sh_reinit() is running).
  • Loading branch information
McDutchie committed Dec 5, 2024
1 parent a1fcad4 commit fcfb686
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 8 deletions.
4 changes: 4 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.
could not be recalled from the history (e.g. with arrow up). Bug introduced
on 2022-10-31.

- Fixed an issue that caused .unset discipline functions (if set) to be
incorrectly called during cleanup upon terminating a virtual subshell or
before running a #!-less script.

2024-11-26:

- The -v/--verbose option of the cp, mv and ln built-in commands (which are
Expand Down
1 change: 1 addition & 0 deletions src/cmd/ksh93/include/shell.h
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ struct Shell_s
* Programs using libshell should not rely on them as they may change. */
int subshell; /* set for virtual subshell */
int realsubshell; /* ${.sh.subshell}, actual subshell level (including virtual and forked) */
char nv_restore; /* set while restoring variables upon terminating a virtual subshell */
int32_t shlvl; /* $SHLVL, non-subshell child shell level */
char shcomp; /* set when running shcomp */
unsigned char trapnote; /* set when trap/signal is pending */
Expand Down
6 changes: 5 additions & 1 deletion src/cmd/ksh93/sh/nvdisc.c
Original file line number Diff line number Diff line change
Expand Up @@ -241,11 +241,15 @@ static void assign(Namval_t *np,const char* val,int flags,Namfun_t *handle)
int type = (flags&NV_APPEND)?APPEND:ASSIGN;
struct vardisc *vp = (struct vardisc*)handle;
Namval_t *nq = vp->disc[type];
struct blocked block, *bp = block_info(np, &block);
struct blocked block, *bp;
Namval_t node;
void *saveval = np->nvalue;
Namval_t *tp, *nr; /* for 'typeset -T' types */
int jmpval = 0;
/* No unset discipline during virtual subshell cleanup or shell reinit */
if(!val && (sh.nv_restore || sh_isstate(SH_INIT)))
return;
bp = block_info(np, &block);
if(val && (tp=nv_type(np)) && (nr=nv_open(val,sh.var_tree,NV_VARNAME|NV_ARRAY|NV_NOADD|NV_NOFAIL)) && tp==nv_type(nr))
{
char *sub = nv_getsub(np);
Expand Down
10 changes: 4 additions & 6 deletions src/cmd/ksh93/sh/subshell.c
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,6 @@ static struct subshell
#endif /* _lib_fchdir */
} *subshell_data;

static char subshell_noscope; /* for temporarily disabling all virtual subshell scope creation */

static unsigned int subenv;


Expand Down Expand Up @@ -264,10 +262,10 @@ void sh_assignok(Namval_t *np,int add)
Namarr_t *ap;
unsigned int save;
/*
* Don't create a scope if told not to (see nv_restore()) or if this is a subshare.
* Don't create a scope during virtual subshell cleanup (see nv_restore()) or if this is a subshare.
* Also, ${.sh.level} (SH_LEVELNOD) is handled specially and is not scoped in virtual subshells.
*/
if(subshell_noscope || sh.subshare || np==SH_LEVELNOD)
if(sh.nv_restore || sh.subshare || np==SH_LEVELNOD)
return;
if((ap=nv_arrayptr(np)) && (mp=nv_opensub(np)))
{
Expand Down Expand Up @@ -330,7 +328,7 @@ static void nv_restore(struct subshell *sp)
Namval_t *mp, *np;
Namval_t *mpnext;
int flags,nofree;
subshell_noscope = 1;
sh.nv_restore = 1;
for(lp=sp->svar; lp; lp=lq)
{
np = (Namval_t*)&lp->dict;
Expand Down Expand Up @@ -388,7 +386,7 @@ static void nv_restore(struct subshell *sp)
free(lp);
sp->svar = lq;
}
subshell_noscope = 0;
sh.nv_restore = 0;
}

/*
Expand Down
25 changes: 24 additions & 1 deletion src/cmd/ksh93/tests/variables.sh
Original file line number Diff line number Diff line change
Expand Up @@ -689,14 +689,37 @@ function dave.unset
unset dave
[[ $(typeset +f) == *dave.* ]] && err_exit 'unset discipline not removed'

# ======

x=$(
dave=dave
function dave.unset
{
print dave.unset
}
)
[[ $x == dave.unset ]] || err_exit 'unset discipline not called with subset completion'
[[ -n $x ]] && err_exit "unset discipline wrongly called upon subshell completion (got '$x')"

x=$(
v="still set"
v.unset()
{
print UNSET
}
ulimit -c 0
print "$v"
)
[[ $x == 'still set' ]] || err_exit "incorrect behaviour of unset discipline in forked subshell (got $(printf %q "$x"))"

echo 'echo ok' >script
v.unset() { echo UNSET; }
v=1
./script >out
{ unset v; } >/dev/null
got=$(<out)
[[ $got = ok ]] || err_exit "incorrect behaviour of unset discipline when running #!-less script (got $(printf %q "$got"))"

# ======

print 'print ${VAR}' > $tmp/script
unset VAR
Expand Down

0 comments on commit fcfb686

Please sign in to comment.