From c928046aa9abfe4287acad5d6abb8fbed4c75096 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sat, 27 Feb 2021 01:01:30 +0000 Subject: [PATCH] Fix ${.sh.fun} leaking out of DEBUG trap The value of the ${.sh.fun} variable, which is supposed to contain the name of the function currently being executed, leaks out of the DEBUG trap if it executes a function. Reproducer: $ fn() { echo "executing the function"; } $ trap fn DEBUG $ trap - DEBUG executing the function $ echo ${.sh.fun} fn ${.sh.fun} should be empty outside the function. Annalysis: The sh_debug() function in xec.c, which executes the DEBUG trap action, contains these lines, which are part of restoring the state after running the trap action with sh_trap(): nv_putval(SH_PATHNAMENOD,shp->st.filename,NV_NOFREE); nv_putval(SH_FUNNAMENOD,shp->st.funname,NV_NOFREE); shp->st = savst; First the SH_PATHNAMENOD (${.sh.file}) and SH_FUNNAMENOD (${.sh.fun}) variables get restored from the values in the shell's scoped information struct (shp->st), but that is done *before* restoring the parent scope with 'shp->st = savst;'. It should be done after. Fixing the order is sufficient to fix the bug. However, I am not convinced that these nv_putval() calls are good for anything at all. Setting, unsetting, restoring, etc. the ${.sh.fun} and ${.sh.file} variables is already being handled perfectly well elsewhere in the code for executing functions and sourcing dot scripts. The DEBUG trap is neither here nor there. There's no reason for it to get involved with these variables. I was unable to break anything after simply removing those two lines. So I strongly suspect this is another case, out of many now, where a bug in ksh93 is properly fixed by removing some code. I couldn't get ${.sh.file} to leak similarly -- I think this is because SH_PATHNAMENOD (and not SH_FUNNOD) is set explicitly in exfile() in main.c, masking this incorrect restore. It is the only place where SH_PATHNAMENOD and SH_FUNNOD are not both set. src/cmd/ksh93/sh/xec.c: - Remove these two spurious nv_putval() calls. src/cmd/ksh93/tests/variables.sh: - Add regression test for leaking ${.sh.fun}. --- NEWS | 4 ++++ src/cmd/ksh93/sh/xec.c | 2 -- src/cmd/ksh93/tests/variables.sh | 6 ++++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 71067751e..862a7b6ea 100644 --- a/NEWS +++ b/NEWS @@ -23,6 +23,10 @@ Any uppercase BUG_* names are modernish shell bug IDs. part uniquely identified it so the menu only showed one item. Details: https://www.mail-archive.com/ast-users@lists.research.att.com/msg00436.html +- A bug with ${.sh.fun} in combination with the DEBUG trap has been fixed. + The ${.sh.fun} variable wrongly continued to contain the name of the last + function executed by the DEBUG trap after the trap action completed. + 2021-02-21: - Fixed: The way that SIGWINCH was handled (i.e. the signal emitted when the diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index ac23276fa..3f045ddb7 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -697,8 +697,6 @@ int sh_debug(Shell_t *shp, const char *trap, const char *name, const char *subsc shp->indebug = 0; if(shp->st.cmdname) error_info.id = shp->st.cmdname; - nv_putval(SH_PATHNAMENOD,shp->st.filename,NV_NOFREE); - nv_putval(SH_FUNNAMENOD,shp->st.funname,NV_NOFREE); shp->st = savst; if(sav != stkptr(stkp,0)) stkset(stkp,sav,offset); diff --git a/src/cmd/ksh93/tests/variables.sh b/src/cmd/ksh93/tests/variables.sh index 531461315..24aa4e1ed 100755 --- a/src/cmd/ksh93/tests/variables.sh +++ b/src/cmd/ksh93/tests/variables.sh @@ -1192,5 +1192,11 @@ got=$(FPATH=$tmp "$SHELL" ./lineno_autoload 2>&1) [[ $got == "$exp" ]] || err_exit 'Regression in $LINENO and/or error messages.' \ $'Diff follows:\n'"$(diff -u <(print -r -- "$exp") <(print -r -- "$got") | sed $'s/^/\t| /')" +# ====== +# Before 2021-02-26, the DEBUG trap corrupted ${.sh.fun} +unset .sh.fun +got=$(some_func() { :; }; trap some_func DEBUG; trap - DEBUG; print -r "${.sh.fun}") +[[ -z $got ]] || err_exit "\${.sh.fun} leaks out of DEBUG trap (got $(printf %q "$got"))" + # ====== exit $((Errors<125?Errors:125))