From 952944197f2744affc3e9abab66f467e2ca5f4e6 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Wed, 20 May 2020 15:50:43 +0100 Subject: [PATCH] Fix bugs in testing if a parameter is set This fixes three related bugs: 1. Expansions like ${var+set} remained static when used within a 'for', 'while' or 'until' loop; the expansions din't change along with the state of the variable, so they could not be used to check whether a variable is set within a loop if the state of that variable changed in the course of the loop. (BUG_ISSETLOOP) 2. ${IFS+s} always yielded 's', and [[ -v IFS ]] always yielded true, even if IFS is unset. (BUG_IFSISSET) 3. IFS was incorrectly exempt from '-u' ('-o nounset') checks. src/cmd/ksh93/sh/macro.c: varsub(): - When getting a node pointer (np) to the parameter to test, special-case IFS by checking if it has a value and not setting the pointer if not. The node to IFS always exists, even after 'unset -v IFS', so before this fix it always followed the code path for a parameter that is set. This fixes BUG_IFSISSET for ${IFS+s} and also fixes set -u (-o nounset) with IFS. - Before using the 'nv_isnull' macro to check if a regular variable is set, call nv_optimize() if needed. This fixes BUG_ISSETLOOP. Idea from Kurtis Rader: https://github.com/att/ast/issues/1090 Of course this only works if SHOPT_OPTIMIZE==1 (the default), but if not, then this bug is not triggered in the first place. - Add some comments for future reference. src/cmd/ksh93/bltins/test.c: test_unop(): - Fix BUG_IFSISSET for [[ -v IFS ]]. The nv_optimize() method doesn't seem to have any effect here, so the only way that I can figure out is to special-case IFS, nv_getval()'ing it to check if IFS has a value in the current scope. src/cmd/ksh93/tests/variables.sh: - Add regression tests for checking if a varariable is set within a loop, within and outside a function with that variable made local (to check if the scope is honoured). Repeat these tests for a regular variable and for IFS, for ${foo+set} and [[ -v foo ]]. (cherry picked from commit a2cf79cb98fa3e47eca85d9049d1d831636c9b16) --- NEWS | 11 ++++++ TODO | 9 ----- src/cmd/ksh93/bltins/test.c | 2 ++ src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/macro.c | 21 +++++++++++- src/cmd/ksh93/tests/variables.sh | 58 ++++++++++++++++++++++++++++++++ 6 files changed, 92 insertions(+), 11 deletions(-) diff --git a/NEWS b/NEWS index 23f8cd90c..d85db4bdb 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,17 @@ For full details, see the git log at: Any uppercase BUG_* names are modernish shell bug IDs. +2020-05-20: + +- Fix BUG_ISSETLOOP. Expansions like ${var+set} remained static when used + within a 'for', 'while' or 'until' loop; the expansions din't change along + with the state of the variable, so they could not be used to check whether a + variable is set within a loop if the state of that variable changed in the + course of the loop. + +- Fix BUG_IFSISSET. ${IFS+s} always yielded 's', and [[ -v IFS ]] always + yielded true, even if IFS is unset. This applied to IFS only. + 2020-05-19: - Fix 'command -p'. The -p option causes the operating system's standard diff --git a/TODO b/TODO index c92aed97b..e7df53400 100644 --- a/TODO +++ b/TODO @@ -65,15 +65,6 @@ https://github.com/modernish/modernish/tree/0.16/lib/modernish/cap/ are erroneously interpreted as wildcards when quoted "$*" is used as the glob pattern. -- BUG_IFSISSET: ${IFS+s} always yields 's' even if IFS is unset. This applies - to IFS only. - -- BUG_ISSETLOOP: Expansions like ${var+set} remain static when used within a - 'for', 'while' or 'until' loop; the expansions don't change along with the - state of the variable, so they cannot be used to check whether a variable - is set within a loop if the state of that variable may change in the - course of the loop. - - BUG_KUNSETIFS: ksh93: Can't unset IFS under very specific circumstances. unset -v IFS is a known POSIX shell idiom to activate default field splitting. With this bug, the unset builtin silently fails to unset IFS diff --git a/src/cmd/ksh93/bltins/test.c b/src/cmd/ksh93/bltins/test.c index e382653c5..bc2f24125 100644 --- a/src/cmd/ksh93/bltins/test.c +++ b/src/cmd/ksh93/bltins/test.c @@ -451,6 +451,8 @@ int test_unop(Shell_t *shp,register int op,register const char *arg) } if(ap = nv_arrayptr(np)) return(nv_arrayisset(np,ap)); + if(*arg=='I' && strcmp(arg,"IFS")==0) + return(nv_getval(np)!=NULL); /* avoid BUG_IFSISSET */ return(!nv_isnull(np) || nv_isattr(np,NV_INTEGER)); } default: diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 70ec07877..f117da57d 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -17,4 +17,4 @@ * David Korn * * * ***********************************************************************/ -#define SH_RELEASE "93u+m 2020-05-19" +#define SH_RELEASE "93u+m 2020-05-20" diff --git a/src/cmd/ksh93/sh/macro.c b/src/cmd/ksh93/sh/macro.c index bb9b1e6b8..26af5691b 100644 --- a/src/cmd/ksh93/sh/macro.c +++ b/src/cmd/ksh93/sh/macro.c @@ -1306,7 +1306,15 @@ retry1: { if(mp->shp->argaddr) flag &= ~NV_NOADD; - np = nv_open(id,mp->shp->var_tree,flag|NV_NOFAIL); + /* + * Get a node pointer (np) to the parameter, if any. + * + * The IFS node always exists, so we must special-case IFS by checking if it has + * a value; otherwise it always follows the code path for a set parameter, so is + * not subject to 'set -u', and may test as set even after 'unset -v IFS'. + */ + if(!(*id=='I' && strcmp(id,"IFS")==0 && nv_getval(sh_scoped(mp->shp,IFSNOD)) == NULL)) + np = nv_open(id,mp->shp->var_tree,flag|NV_NOFAIL); if(!np) { sfprintf(mp->shp->strbuf,"%s%c",id,0); @@ -1393,8 +1401,12 @@ retry1: if((type==M_VNAME||type==M_SUBNAME) && mp->shp->argaddr && strcmp(nv_name(np),id)) mp->shp->argaddr = 0; c = (type>M_BRACE && isastchar(mode)); + /* + * Check if the parameter is set or unset. + */ if(np && (type==M_TREE || !c || !ap)) { + /* The parameter is set. */ char *savptr; c = *((unsigned char*)stkptr(stkp,offset-1)); savptr = stkfreeze(stkp,0); @@ -1430,7 +1442,13 @@ retry1: if(ap) v = nv_arrayisset(np,ap)?(char*)"x":0; else + { +#if SHOPT_OPTIMIZE + if(mp->shp->argaddr) + nv_optimize(np); /* avoid BUG_ISSETLOOP */ +#endif /* SHOPT_OPTIMIZE */ v = nv_isnull(np)?0:(char*)"x"; + } } else v = nv_getval(np); @@ -1446,6 +1464,7 @@ retry1: } else { + /* The parameter is unset. */ if(sh_isoption(SH_NOUNSET) && !isastchar(mode) && (type==M_VNAME || type==M_SIZE)) errormsg(SH_DICT,ERROR_exit(1),e_notset,id); v = 0; diff --git a/src/cmd/ksh93/tests/variables.sh b/src/cmd/ksh93/tests/variables.sh index debe8f575..590e55ca4 100755 --- a/src/cmd/ksh93/tests/variables.sh +++ b/src/cmd/ksh93/tests/variables.sh @@ -680,4 +680,62 @@ level=$($SHELL -c $'$SHELL -c \'print -r "$SHLVL"\'') $SHELL -c 'unset .sh' 2> /dev/null [[ $? == 1 ]] || err_exit 'unset .sh should return 1' +#' + +# ====== +# ${var+set} within a loop. +_test_isset() { eval " + $1=initial_value + function _$1_test { + typeset $1 # make local and initially unset + for i in 1 2 3 4 5; do + case \${$1+s} in + ( s ) print -n 'S'; unset -v $1 ;; + ( '' ) print -n 'U'; $1='' ;; + esac + done + } + _$1_test + [[ -n \${$1+s} && \${$1} == initial_value ]] || exit + for i in 1 2 3 4 5; do + case \${$1+s} in + ( s ) print -n 's'; unset -v $1 ;; + ( '' ) print -n 'u'; $1='' ;; + esac + done +"; } +expect='USUSUsusus' +actual=$(_test_isset var) +[[ "$actual" = "$expect" ]] || err_exit "\${var+s} expansion fails in loops (expected '$expect', got '$actual')" +actual=$(_test_isset IFS) +[[ "$actual" = "$expect" ]] || err_exit "\${IFS+s} expansion fails in loops (expected '$expect', got '$actual')" + +# [[ -v var ]] within a loop. +_test_v() { eval " + $1=initial_value + function _$1_test { + typeset $1 # make local and initially unset + for i in 1 2 3 4 5; do + if [[ -v $1 ]] + then print -n 'S'; unset -v $1 + else print -n 'U'; $1='' + fi + done + } + _$1_test + [[ -v $1 && \${$1} == initial_value ]] || exit + for i in 1 2 3 4 5; do + if [[ -v $1 ]] + then print -n 's'; unset -v $1 + else print -n 'u'; $1='' + fi + done +"; } +expect='USUSUsusus' +actual=$(_test_v var) +[[ "$actual" = "$expect" ]] || err_exit "[[ -v var ]] expansion fails in loops (expected '$expect', got '$actual')" +actual=$(_test_v IFS) +[[ "$actual" = "$expect" ]] || err_exit "[[ -v IFS ]] expansion fails in loops (expected '$expect', got '$actual')" + +# ====== exit $((Errors<125?Errors:125))