From 7003aba487dd61a098464f746a4d2adcdacf4d08 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sat, 30 May 2020 15:21:30 +0100 Subject: [PATCH] Fix 'test'/'[' exit status >1 on error in arithmetic expression Fix BUG_TESTERR1A: POSIX non-compliance of 'test'/'[' exit status on error. The command now returns status 2 instead of 1 when given an invalid number or arithmetic expression, e.g.: [ 123 -eq 123x ] The problem was that the test builtin (b_test()) calls the generic arithmetic evaluation subsystem (sh/arith.c, sh/streval.c) which has no awareness of the test builtin. A simple solution would be to always make the arithmetic subsystem use an exit status > 1 for arithmetic errors, but globally changing this may cause backwards compatibility issues. So it's best to change the behaviour of the 'test' builtin only. This requires the arithmetic subsystem to be aware of whether it was called from the 'test' builtin or not. To that end, this commit adds a global flag and overrides the ERROR_exit macro where needed. src/cmd/ksh93/include/defs.h, src/cmd/ksh93/sh/defs.c: - Declare and initialise a global sh_in_test_builtin flag. - Declare internal function for ERROR_exit override in test.c. src/cmd/ksh93/bltins/test.c: - Add override for ERROR_exit macro using a function that checks if the exit status is at least 2 if the error occurred while running the test builtin. - b_test(): Set sh_in_test_builtin flag while running test builtin. src/cmd/ksh93/sh/arith.c, src/cmd/ksh93/sh/streval.c: - Override ERROR_exit macro using function from test.c. src/cmd/ksh93/tests/bracket.sh: - Add regression test verifying status > 1 on arith error in test. (cherry picked from commit 5eeae5eb9fd5ed961a5096764ad11ab870a223a9) --- NEWS | 8 +++++ TODO | 3 -- src/cmd/ksh93/bltins/test.c | 59 ++++++++++++++++++++++++++++----- src/cmd/ksh93/include/defs.h | 2 ++ src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/arith.c | 3 ++ src/cmd/ksh93/sh/defs.c | 1 + src/cmd/ksh93/sh/streval.c | 3 ++ src/cmd/ksh93/tests/bracket.sh | 5 +++ 9 files changed, 73 insertions(+), 13 deletions(-) diff --git a/NEWS b/NEWS index b658dd7ca..b69a00bd4 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,14 @@ For full details, see the git log at: Any uppercase BUG_* names are modernish shell bug IDs. +2020-05-30: + +- Fix POSIX compliance of 'test'/'[' exit status on error. The command now + returns status 2 instead of 1 when given an invalid number or arithmetic + expression, e.g.: + [ 123 -eq 123x ]; echo $? + now outputs 2 instead of 1. + 2020-05-29: - Fix BUG_FNSUBSH: functions can now be correctly redefined and unset in diff --git a/TODO b/TODO index c8dd5a371..85e37474b 100644 --- a/TODO +++ b/TODO @@ -77,6 +77,3 @@ https://github.com/modernish/modernish/tree/0.16/lib/modernish/cap/ multi-byte characters as IFS field delimiters still doesn't work. For example, "$*" joins positional parameters on the first byte of IFS instead of the first character. - -- BUG_TESTERR1A: test/[ exits with a non-error false status (1) if an - invalid argument is given to an operator. diff --git a/src/cmd/ksh93/bltins/test.c b/src/cmd/ksh93/bltins/test.c index bc2f24125..45aa3df92 100644 --- a/src/cmd/ksh93/bltins/test.c +++ b/src/cmd/ksh93/bltins/test.c @@ -82,6 +82,23 @@ static char *nxtarg(struct test*,int); static int expr(struct test*,int); static int e3(struct test*); +/* + * POSIX requires error status > 1 for test builtin. + * Since ksh 'test' can parse arithmetic expressions, the #define + * override is also needed in sh/arith.c and sh/streval.c + */ +int _ERROR_exit_b_test(int exitval) +{ + if(sh_in_test_builtin) + { + sh_in_test_builtin = 0; + if(exitval < 2) + exitval = 2; + } + return(ERROR_exit(exitval)); +} +#define ERROR_exit(n) _ERROR_exit_b_test(n) + static int test_strmatch(Shell_t *shp,const char *str, const char *pat) { regoff_t match[2*(MATCH_MAX+1)],n; @@ -113,6 +130,9 @@ int b_test(int argc, char *argv[],Shbltin_t *context) struct test tdata; register char *cp = argv[0]; register int not; + int exitval; + + sh_in_test_builtin = 1; tdata.sh = context->shp; tdata.av = argv; tdata.ap = 1; @@ -123,7 +143,10 @@ int b_test(int argc, char *argv[],Shbltin_t *context) errormsg(SH_DICT,ERROR_exit(2),e_missing,"']'"); } if(argc <= 1) - return(1); + { + exitval = 1; + goto done; + } cp = argv[1]; if(c_eq(cp,'(') && argc<=6 && c_eq(argv[argc-1],')')) { @@ -153,18 +176,31 @@ int b_test(int argc, char *argv[],Shbltin_t *context) if(argc==5) break; if(not && cp[0]=='-' && cp[2]==0) - return(test_unop(tdata.sh,cp[1],argv[3])!=0); + { + exitval = (test_unop(tdata.sh,cp[1],argv[3])!=0); + goto done; + } else if(argv[1][0]=='-' && argv[1][2]==0) - return(!test_unop(tdata.sh,argv[1][1],cp)); + { + exitval = (!test_unop(tdata.sh,argv[1][1],cp)); + goto done; + } else if(not && c_eq(argv[2],'!')) - return(*argv[3]==0); + { + exitval = (*argv[3]==0); + goto done; + } errormsg(SH_DICT,ERROR_exit(2),e_badop,cp); } - return(test_binop(tdata.sh,op,argv[1],argv[3])^(argc!=5)); + exitval = (test_binop(tdata.sh,op,argv[1],argv[3])^(argc!=5)); + goto done; } case 3: if(not) - return(*argv[2]!=0); + { + exitval = (*argv[2]!=0); + goto done; + } if(cp[0] != '-' || cp[2] || cp[1]=='?') { if(cp[0]=='-' && (cp[1]=='-' || cp[1]=='?') && @@ -180,12 +216,17 @@ int b_test(int argc, char *argv[],Shbltin_t *context) } break; } - return(!test_unop(tdata.sh,cp[1],argv[2])); + exitval = (!test_unop(tdata.sh,cp[1],argv[2])); + goto done; case 2: - return(*cp==0); + exitval = (*cp==0); + goto done; } tdata.ac = argc; - return(!expr(&tdata,0)); + exitval = (!expr(&tdata,0)); +done: + sh_in_test_builtin = 0; + return(exitval); } /* diff --git a/src/cmd/ksh93/include/defs.h b/src/cmd/ksh93/include/defs.h index 049ddc593..d883bdddf 100644 --- a/src/cmd/ksh93/include/defs.h +++ b/src/cmd/ksh93/include/defs.h @@ -505,5 +505,7 @@ extern const char e_dict[]; # define sh_stats(x) #endif /* SHOPT_STATS */ +extern int sh_in_test_builtin; +extern int _ERROR_exit_b_test(int); #endif diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index c03d5ee3e..abaf5ea16 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-29" +#define SH_RELEASE "93u+m 2020-05-30" diff --git a/src/cmd/ksh93/sh/arith.c b/src/cmd/ksh93/sh/arith.c index 6361431b7..901f33ea6 100644 --- a/src/cmd/ksh93/sh/arith.c +++ b/src/cmd/ksh93/sh/arith.c @@ -31,6 +31,9 @@ #include "variables.h" #include "builtins.h" +/* POSIX requires error status > 1 if called from test builtin */ +#define ERROR_exit(n) _ERROR_exit_b_test(n) + #ifndef LLONG_MAX #define LLONG_MAX LONG_MAX #endif diff --git a/src/cmd/ksh93/sh/defs.c b/src/cmd/ksh93/sh/defs.c index 308ce22fe..d505af120 100644 --- a/src/cmd/ksh93/sh/defs.c +++ b/src/cmd/ksh93/sh/defs.c @@ -46,3 +46,4 @@ char *sh_lexstates[ST_NONE] = {0}; struct jobs job = {0}; int32_t sh_mailchk = 600; +int sh_in_test_builtin = 0; diff --git a/src/cmd/ksh93/sh/streval.c b/src/cmd/ksh93/sh/streval.c index 8f4db66c8..1928ef88c 100644 --- a/src/cmd/ksh93/sh/streval.c +++ b/src/cmd/ksh93/sh/streval.c @@ -36,6 +36,9 @@ #include "FEATURE/externs" #include "defs.h" /* for sh.decomma */ +/* POSIX requires error status > 1 if called from test builtin */ +#define ERROR_exit(n) _ERROR_exit_b_test(n) + #ifndef ERROR_dictionary # define ERROR_dictionary(s) (s) #endif diff --git a/src/cmd/ksh93/tests/bracket.sh b/src/cmd/ksh93/tests/bracket.sh index 8be30ed77..fdabb0e6d 100755 --- a/src/cmd/ksh93/tests/bracket.sh +++ b/src/cmd/ksh93/tests/bracket.sh @@ -370,5 +370,10 @@ var=$(echo begin; exec >/dev/tty; [ -t 1 ] && test -t 1 && [[ -t 1 ]]) \ && [[ $var == $'begin' ]] \ || err_exit "test -t 1 in comsub with exec >/dev/tty fails" +# ====== +# POSIX specifies that on error, test builtin should always return status > 1 +test 123 -eq 123x 2>/dev/null +[[ $? -ge 2 ]] || err_exit 'test builtin should return value greater than 1 on error' + # ====== exit $((Errors<125?Errors:125))