From a197b0427afbc29a0a3637342f03dba6b7a7a35a Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Wed, 5 May 2021 02:42:45 +0100 Subject: [PATCH] Fix two more 'command' bugs BUG 1: Though 'command' is specified/documented as a regular builtin, preceding assignments survive the invocation (as with special or declaration builtins) if 'command' has no command arguments in these cases: $ foo=wrong1 command; echo $foo wrong1 $ foo=wrong2 command -p; echo $foo wrong2 $ foo=wrong3 command -x; echo $foo wrong3 Analysis: sh_exec(), case TCOM (simple command), contains the following loop that skips over 'command' prefixes, preparsing any options and remembering the offset in the 'command' variable: src/cmd/ksh93/sh/xec.c 1059 while(np==SYSCOMMAND || !np && com0 && nv_search(com0,shp->fun_tree,0)==SYSCOMMAND) 1060 { 1061 register int n = b_command(0,com,&shp->bltindata); 1062 if(n==0) 1063 break; 1064 command += n; 1065 np = 0; 1066 if(!(com0= *(com+=n))) 1067 break; 1068 np = nv_bfsearch(com0, shp->bltin_tree, &nq, &cp); 1069 } This skipping is not done if the preliminary b_command() call on line 1061 (with argc==0) returns zero. This is currently the case for command -v/-V, so that 'command' is treated as a plain and regular builtin for those options. The cause of the bug is that this skipping is even done if 'command' has no arguments. So something like 'foo=bar command' is treated as simply 'foo=bar', which of course survives. So the fix is for b_command() to return zero if there are no arguments. Then b_command() itself needs changing to not error out on the second/main b_command() call if there are no arguments. src/cmd/ksh93/bltins/whence.c: b_command(): - When called with argc==0, return a zero offset not just for -v (X_FLAG) or -V (V_FLAG), but also if there are no arguments left (!*argv) after parsing options. - When called with argc>0, do not issue a usage error if there are no arguments, but instead return status 0 (or, if -v/-V was given, status 2 which was the status of the previous usage message). This way, 'command -v $emptyvar' now also works as you'd expect. BUG 2: 'command -p' sometimes failed after executing certain loops. src/cmd/ksh93/sh/path.c: defpath_init(): - astconf() returns a pointer to memory that may be overwritten later, so duplicate the string returned. Backported from ksh2020. (re: f485fe0f, aa4669ad, ) src/cmd/ksh93/tests/builtins.sh: - Update the test for BUG_CMDSPASGN to check every variant of 'command' (all options and none; invoking/querying all kinds of command and none) with a preceding assignment. (re: fae8862c) This also covers bug 2 as 'command -p' was failing on macOS prior to the fix due to a loop executed earlier in another test. --- NEWS | 9 +++++++++ src/cmd/ksh93/bltins/whence.c | 10 ++++++---- src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/path.c | 8 ++++++-- src/cmd/ksh93/tests/builtins.sh | 20 ++++++++++++++++++-- 5 files changed, 40 insertions(+), 9 deletions(-) diff --git a/NEWS b/NEWS index 487c048eb..2a8e8024b 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,15 @@ For full details, see the git log at: https://github.com/ksh93/ksh Any uppercase BUG_* names are modernish shell bug IDs. +2021-05-05: + +- Fixed: a preceding variable assignment like foo=bar in 'foo=bar command' + (with no command arguments after 'command') incorrectly survived the + 'command' regular built-in command invocation. + +- Fixed: 'command -p some_utility' intermittently failed to find the utility + under certain conditions due to a memory corruption issue. + 2021-05-03: - Subshells (even if non-forked) now keep a properly separated state of the diff --git a/src/cmd/ksh93/bltins/whence.c b/src/cmd/ksh93/bltins/whence.c index 5bf898bdb..94d7ae4c3 100644 --- a/src/cmd/ksh93/bltins/whence.c +++ b/src/cmd/ksh93/bltins/whence.c @@ -85,20 +85,22 @@ int b_command(register int argc,char *argv[],Shbltin_t *context) errormsg(SH_DICT,ERROR_usage(2), "%s", opt_info.arg); UNREACHABLE(); } + argv += opt_info.index; if(argc==0) { - if(flags & (X_FLAG|V_FLAG)) - return(0); /* return no offset now; sh_exec() will treat command -v/-V as normal builtin */ + if((flags & (X_FLAG|V_FLAG)) || !*argv) + return(0); /* return no offset now; sh_exec() will treat command -v/-V/(null) as normal builtin */ if(flags & P_FLAG) sh_onstate(SH_XARG); return(opt_info.index); /* offset for sh_exec() to remove 'command' prefix + options */ } - argv += opt_info.index; - if(error_info.errors || !*argv) + if(error_info.errors) { errormsg(SH_DICT,ERROR_usage(2),"%s", optusage((char*)0)); UNREACHABLE(); } + if(!*argv) + return((flags & (X_FLAG|V_FLAG)) != 0 ? 2 : 0); return(whence(shp,argv, flags)); } diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index c12566b2a..bc47b64fe 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -21,7 +21,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.0.0-alpha" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2021-05-03" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2021-05-05" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2021 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/sh/path.c b/src/cmd/ksh93/sh/path.c index 312b812db..eb18d8cf8 100644 --- a/src/cmd/ksh93/sh/path.c +++ b/src/cmd/ksh93/sh/path.c @@ -431,8 +431,12 @@ Pathcomp_t *path_nextcomp(Shell_t *shp,register Pathcomp_t *pp, const char *name static Pathcomp_t* defpath_init(Shell_t *shp) { - if(!std_path && !(std_path=astconf("PATH",NIL(char*),NIL(char*)))) - abort(); + if(!std_path) + { + if(!(std_path = astconf("PATH",NIL(char*),NIL(char*)))) + abort(); + std_path = sh_strdup(std_path); /* the value returned by astconf() is short-lived */ + } Pathcomp_t *pp = (void*)path_addpath(shp,(Pathcomp_t*)0,(std_path),PATH_PATH); return(pp); } diff --git a/src/cmd/ksh93/tests/builtins.sh b/src/cmd/ksh93/tests/builtins.sh index 0b9cc64db..f78bb001c 100755 --- a/src/cmd/ksh93/tests/builtins.sh +++ b/src/cmd/ksh93/tests/builtins.sh @@ -768,8 +768,24 @@ printf '\\\000' | read -r -d '' # ====== # BUG_CMDSPASGN: Preceding a special builtin with 'command' should disable its special properties. -foo=BUG command eval ':' -[[ $foo == BUG ]] && err_exit "'command' fails to disable the special properties of special builtins" +# Test that assignments preceding 'command' are local. +command -p ls +for arg in '' -v -V -p -x +do + for cmd in '' : true ls eval 'eval :' 'eval true' 'eval ls' + do + [[ $arg == -x ]] && ! command -xv "${cmd% *}" >/dev/null && continue + unset foo + eval "foo=BUG command $arg $cmd" >/dev/null 2>&1 + got=$? + case $arg,$cmd in + -v, | -V, ) exp=2 ;; + *) exp=0 ;; + esac + [[ $got == "$exp" ]] || err_exit "exit status of 'command $arg $cmd' is $got, expected $exp" + [[ -v foo ]] && err_exit "preceding assignment survives 'command $arg $cmd'" + done +done # Regression that occurred after fixing the bug above: the += operator didn't work correctly. # https://www.mail-archive.com/ast-users@lists.research.att.com/msg00369.html