From 55f0f8ce52ec39a797a745031a60816f998ce01a Mon Sep 17 00:00:00 2001
From: Martijn Dekker <martijn@inlv.org>
Date: Tue, 1 Sep 2020 20:24:44 +0100
Subject: [PATCH] -o posix: disable '[ -t ]' == '[ -t 1 ]' hack

On ksh93, 'test -t' is equivalent to 'test -t 1' (and of course
"[ -t ]" is equivalent to "[ -t 1 ]").

This is purely for compatibility with ancient Bourne shell
breakage. No other shell supports this. ksh93 should probably keep
it for backwards compatibility, but it should definitely be
disabled in POSIX mode as it is a violation of the standard; 'test
-t' is an instance of 'test "$string"', which tests if the string
is empty, so it should test if the string '-t' is empty (quod non).

This also replaces the fix for 'test -t 1' in a command
substitution with a better one that avoids forking (re: cafe33f0).

src/cmd/ksh93/sh/parse.c:
- qscan(): If the posix option is active, disable the parser-based
  hack that converts a simple "[ -t ]" to "[ -t 1 ]".

src/cmd/ksh93/bltins/test.c:
- e3(): If the posix option is active, disable the part of the
  compatibility hack that was used for compound expressions
  that end in '-t', e.g. "[ -t 2 -o -t ]".
- test_unop(): Remove the forking fix for "[ -t 1 ]".

src/cmd/ksh93/edit/edit.c:
- tty_check(): This function is used by "[ -t 1 ]" and in other
  contexts as well, so a fix here is more comprehensive. Forking
  here would cause a segfault, but we don't actually need to. This
  adds a fix that simply returns false if we're in a virtual
  subshell that is also a command substitution. Since command
  substitutions always fork upon redirecting standard output within
  them (making them no longer virtual), it is safe to do this.

src/cmd/ksh93/tests/bracket.sh
- Add comprehensive regression tests for test/[/[[ -t variants in
  command substitutions, in simple and compound expressions, with
  and without redirecting stdout to /dev/tty within the comsub.
- Add tests verifying that -o posix disables the old hack.
- Tweak other tests, including one that globally disabled xtrace.
---
 NEWS                           |  1 +
 src/cmd/ksh93/bltins/test.c    | 12 +++----
 src/cmd/ksh93/edit/edit.c      |  6 ++++
 src/cmd/ksh93/sh.1             |  1 +
 src/cmd/ksh93/sh/parse.c       | 17 ++++++---
 src/cmd/ksh93/tests/bracket.sh | 63 ++++++++++++++++++++++++++--------
 6 files changed, 75 insertions(+), 25 deletions(-)

diff --git a/NEWS b/NEWS
index 81f04f079..6899dc038 100644
--- a/NEWS
+++ b/NEWS
@@ -20,6 +20,7 @@ Any uppercase BUG_* names are modernish shell bug IDs.
   * makes the <> redirection operator default to stdin instead of stdout
     (this keeps the 2020-05-13 BUG_REDIRIO fix for the POSIX mode while
     restoring traditional ksh93 behaviour for backwards compatibility)
+  * disables a noncompliant 'test -t' == 'test -t 1' compatibility hack
 
 2020-08-19:
 
diff --git a/src/cmd/ksh93/bltins/test.c b/src/cmd/ksh93/bltins/test.c
index 569844959..3fe2772b2 100644
--- a/src/cmd/ksh93/bltins/test.c
+++ b/src/cmd/ksh93/bltins/test.c
@@ -314,8 +314,12 @@ static int e3(struct test *tp)
 	cp = nxtarg(tp,1);
 	if(cp!=0 && (c_eq(cp,'=') || c2_eq(cp,'!','=')))
 		goto skip;
-	if(c2_eq(arg,'-','t'))
-	{
+	if(!sh_isoption(SH_POSIX) && c2_eq(arg,'-','t'))
+	{	/*
+		 * Ancient compatibility hack supporting test -t with no arguments == test -t 1.
+		 * This is only reached when doing a compound expression like: test 1 -eq 1 -a -t
+		 * (for simple 'test -t' this is handled in the parser, see qscan() in sh/parse.c).
+		 */
 		if(cp)
 		{
 			op = strtol(cp,&binop, 10);
@@ -323,7 +327,6 @@ static int e3(struct test *tp)
 		}
 		else
 		{
-		/* test -t with no arguments */
 			tp->ap--;
 			return(tty_check(1));
 		}
@@ -455,9 +458,6 @@ int test_unop(Shell_t *shp,register int op,register const char *arg)
 	    {
 		char *last;
 		op = strtol(arg,&last, 10);
-		/* To make '-t 1' work in a $(comsub), fork. https://github.com/att/ast/issues/1079 */
-		if (op == 1 && shp->subshell && shp->comsub && !shp->subshare)
-			sh_subfork();
 		return(*last?0:tty_check(op));
 	    }
 	    case 'v':
diff --git a/src/cmd/ksh93/edit/edit.c b/src/cmd/ksh93/edit/edit.c
index fe522667b..c800f06d6 100644
--- a/src/cmd/ksh93/edit/edit.c
+++ b/src/cmd/ksh93/edit/edit.c
@@ -162,6 +162,12 @@ int tty_check(int fd)
 {
 	register Edit_t *ep = (Edit_t*)(shgd->ed_context);
 	struct termios tty;
+	/*
+	 * The tty_get check below does not work on 1 (stdout) in command substitutions. But comsubs fork upon redirecting 1,
+	 * and forking resets sh.subshell to 0, so we can safely return false when in a virtual subshell that is a comsub.
+	 */
+	if(fd==1 && sh.subshell && sh.comsub)
+		return(0);
 	ep->e_savefd = -1;
 	return(tty_get(fd,&tty)==0);
 }
diff --git a/src/cmd/ksh93/sh.1 b/src/cmd/ksh93/sh.1
index 31e1f1406..6ec8a99ec 100644
--- a/src/cmd/ksh93/sh.1
+++ b/src/cmd/ksh93/sh.1
@@ -7042,6 +7042,7 @@ Enable POSIX standard compatibility mode. This option
 is on by default if ksh is invoked as \fBsh\fR. It
 causes file descriptors > 2 to be left open when invoking another program,
 makes the \fB<>\fR redirection operator default to standard input,
+disables a hack that makes \fBtest -t\fR (\fB[ -t ]\fR) equivalent to \fBtest -t 1\fR (\fB[ -t 1 ]\fR),
 enables octal numbers in \fBlet\fR shell arithmetic (see \fBletoctal\fR), and
 disables the \fB&>\fR redirection shorthand.
 .TP 8
diff --git a/src/cmd/ksh93/sh/parse.c b/src/cmd/ksh93/sh/parse.c
index ac91de258..921b455ca 100644
--- a/src/cmd/ksh93/sh/parse.c
+++ b/src/cmd/ksh93/sh/parse.c
@@ -1800,11 +1800,18 @@ static struct argnod *qscan(struct comnod *ac,int argn)
 	register struct argnod *ap;
 	register struct dolnod* dp;
 	register int special=0;
-	/* special hack for test -t compatibility */
-	if((Namval_t*)ac->comnamp==SYSTEST)
-		special = 2;
-	else if(*(ac->comarg->argval)=='[' && ac->comarg->argval[1]==0)
-		special = 3;
+	/*
+	 * The 'special' variable flags a parser hack for ancient 'test -t' compatibility.
+	 * As this is done at parse time, it only affects literal '-t', not 'foo=-t; test "$foo"'.
+	 * It only works for a simple '-t'; a compound expression ending in '-t' is hacked in bltins/test.c.
+	 */
+	if(!sh_isoption(SH_POSIX))
+	{
+		if((Namval_t*)ac->comnamp==SYSTEST)
+			special = 2;	/* convert "test -t" to "test -t 1" */
+		else if(*(ac->comarg->argval)=='[' && ac->comarg->argval[1]==0)
+			special = 3;	/* convert "[ -t ]" to "[ -t 1 ]" */
+	}
 	if(special)
 	{
 		ap = ac->comarg->argnxt.ap;
diff --git a/src/cmd/ksh93/tests/bracket.sh b/src/cmd/ksh93/tests/bracket.sh
index 0ddd0d394..994654627 100755
--- a/src/cmd/ksh93/tests/bracket.sh
+++ b/src/cmd/ksh93/tests/bracket.sh
@@ -202,11 +202,12 @@ exec 4>&-
 if	[[ 011 -ne 11 ]]
 then	err_exit "leading zeros in arithmetic compares not ignored"
 fi
-{
-	set -x
-	[[ foo > bar ]]
-} 2> /dev/null || { set +x; err_exit "foo<bar with -x enabled" ;}
-set +x
+(
+	{
+		set -x
+		[[ foo > bar ]]
+	} 2> /dev/null
+) || err_exit "foo<bar with -x enabled"
 (
 	eval "[[ (a) ]]"
 ) 2> /dev/null || err_exit "[[ (a) ]] not working"
@@ -367,24 +368,58 @@ test ! ! '' 2> /dev/null && err_exit 'test ! ! "" should return non-zero'
 
 # This is the simple case that doesn't do any redirection of stdout within the command
 # substitution. Thus the [ -t 1 ] test should be false.
-var=$(echo begin; { [ -t 1 ] || test -t 1 || [[ -t 1 ]]; } && echo -t 1 is true; echo end)
-[[ $var == $'begin\nend' ]] || err_exit "test -t 1 in comsub fails"
+expect=$'begin\nend'
+actual=$(echo begin; [ -t 1 ] || test -t 1 || [[ -t 1 ]] && echo -t 1 is true; echo end)
+[[ $actual == "$expect" ]] || err_exit 'test -t 1 in comsub fails' \
+	"(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
+actual=$(echo begin; [ -n X -a -t 1 ] || test -n X -a -t 1 || [[ -n X && -t 1 ]] && echo -t 1 is true; echo end)
+[[ $actual == "$expect" ]] || err_exit 'test -t 1 in comsub fails (compound expression)' \
+	"(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
+# Same for the ancient compatibility hack for 'test -t' with no arguments.
+actual=$(echo begin; [ -t ] || test -t && echo -t is true; echo end)
+[[ $actual == "$expect" ]] || err_exit 'test -t in comsub fails' \
+	"(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
+actual=$(echo begin; [ -n X -a -t ] || test -n X -a -t && echo -t is true; echo end)
+[[ $actual == "$expect" ]] || err_exit 'test -t in comsub fails (compound expression)' \
+	"(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
 
 # This is the more complex case that does redirect stdout within the command substitution to the
 # actual tty. Thus the [ -t 1 ] test should be true.
-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"
+actual=$(echo begin; exec >/dev/tty; [ -t 1 ] && test -t 1 && [[ -t 1 ]]) \
+|| err_exit 'test -t 1 in comsub with exec >/dev/tty fails'
+actual=$(echo begin; exec >/dev/tty; [ -n X -a -t 1 ] && test -n X -a -t 1 && [[ -n X && -t 1 ]]) \
+|| err_exit 'test -t 1 in comsub with exec >/dev/tty fails (compound expression)'
+# Same for the ancient compatibility hack for 'test -t' with no arguments.
+actual=$(echo begin; exec >/dev/tty; [ -t ] && test -t) \
+|| err_exit 'test -t in comsub with exec >/dev/tty fails'
+actual=$(echo begin; exec >/dev/tty; [ -n X -a -t ] && test -n X -a -t) \
+|| err_exit 'test -t in comsub with exec >/dev/tty fails (compound expression)'
+
+# The POSIX mode should disable the ancient 'test -t' compatibility hack.
+if	[[ -o ?posix ]]
+then	# need 'eval' in first test, as it's a parser hack, and ksh normally parses ahead of execution
+	(set -o posix; eval '[ -t ] && test -t') >/dev/null \
+	|| err_exit "posix mode fails to disable 'test -t' compat hack"
+	# the compound command variant of the hack is in the 'test' builtin itself; no 'eval' needed
+	expect=$'*: argument expected\n*: argument expected'
+	actual=$(set -o posix; [ -n X -a -t ] 2>&1; test -n X -a -t 2>&1)
+	[[ $actual == $expect ]] || err_exit "posix mode fails to disable 'test -t' compat hack (compound expression)" \
+		"(expected output matching $(printf %q "$expect"), got $(printf %q "$actual"))"
+fi
 
 # ======
 # 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'
+expect=$': test: 123x: arithmetic syntax error\nExit status: 2'
+actual=$(test 123 -eq 123x 2>&1; echo "Exit status: $?")
+[[ $actual == *"$expect" ]] || err_exit 'test builtin does not error out with status 2' \
+	"(expected *$(printf %q "$expect"), got $(printf %q "$actual"))"
 
 # ======
 # The '=~' operator should work with curly brackets
-$SHELL -c '[[ AATAAT =~ (AAT){2} ]]' || err_exit '[[ AATAAT =~ (AAT){2} ]] does not match'
-$SHELL -c '[[ AATAATCCCAATAAT =~ (AAT){2}CCC(AAT){2} ]]' || err_exit '[[ AATAATCCCAATAAT =~ (AAT){2}CCC(AAT){2} ]] does not match'
+error=$(set +x; "$SHELL" -c '[[ AATAAT =~ (AAT){2} ]]' 2>&1) \
+|| err_exit "[[ AATAAT =~ (AAT){2} ]] does not match${error:+ (got $(printf %q "$error"))}"
+error=$(set +x; "$SHELL" -c '[[ AATAATCCCAATAAT =~ (AAT){2}CCC(AAT){2} ]]' 2>&1) \
+|| err_exit "[[ AATAATCCCAATAAT =~ (AAT){2}CCC(AAT){2} ]] does not match${error:+ (got $(printf %q "$error"))}"
 
 # ======
 exit $((Errors<125?Errors:125))