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

@@ -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

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.

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);
 }
 
 /*

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

src/cmd/ksh93/include/version.h

@@ -17,4 +17,4 @@
 *                  David Korn <dgk@research.att.com>                   *
 *                                                                      *
 ***********************************************************************/
-#define SH_RELEASE	"93u+m 2020-05-29"
+#define SH_RELEASE	"93u+m 2020-05-30"

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

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;

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

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))