From 8e1e405e41d8ab075f46bd519de94211a0999d8b Mon Sep 17 00:00:00 2001
From: Kurtis Rader <krader@skepticism.us>
Date: Mon, 17 Dec 2018 19:01:22 -0800
Subject: [PATCH] Fix [ -t1 ] inside a command substitution

Another non-forking subshell bug involving `[ -t 1 ]` being true when it
should be false.

Fixes #1079
---
 CHANGELOG.md                         |  2 ++
 src/cmd/ksh93/bltins/test.c          | 31 ++++++++++++----------
 src/cmd/ksh93/tests/basic.sh         | 39 ++++++++++++++++------------
 src/cmd/ksh93/tests/test.exp         | 22 ++++++++++++++++
 src/cmd/ksh93/tests/test.exp.out     |  2 ++
 src/cmd/ksh93/tests/util/preamble.sh | 13 ++++++++++
 6 files changed, 79 insertions(+), 30 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 77022d66f675..33b159b6f25b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -43,6 +43,8 @@ None at this time.
 
 ## Notable fixes and improvements
 
+- Doing `[ -t1 ]` inside a command substitution behaves correctly
+  (issue #1079).
 - The project now passes its unit tests when built with malloc debugging
   enabled (i.e., `meson test --setup=malloc`).
 - Changes to the project are now validated by running unit tests on the Travis
diff --git a/src/cmd/ksh93/bltins/test.c b/src/cmd/ksh93/bltins/test.c
index 208e231b705a..94a186d859f7 100644
--- a/src/cmd/ksh93/bltins/test.c
+++ b/src/cmd/ksh93/bltins/test.c
@@ -92,8 +92,8 @@ struct test {
 };
 
 static_fn char *nxtarg(struct test *, int);
-static_fn int eval_expr(struct test *, int);
-static_fn int eval_e3(struct test *);
+static_fn int eval_expr(Shell_t *shp, struct test *, int);
+static_fn int eval_e3(Shell_t *shp, struct test *);
 
 static_fn int test_strmatch(Shell_t *shp, const char *str, const char *pat) {
     int match[2 * (MATCH_MAX + 1)], n;
@@ -223,7 +223,7 @@ int b_test(int argc, char *argv[], Shbltin_t *context) {
         default: { break; }
     }
     tdata.ac = argc;
-    result = !eval_expr(&tdata, 0);
+    result = !eval_expr(shp, &tdata, 0);
 
 done:
     sh_popcontext(shp, &buff);
@@ -236,11 +236,11 @@ int b_test(int argc, char *argv[], Shbltin_t *context) {
 // Flag is 1 when in parenthesis.
 // Flag is 2 when evaluating -a.
 //
-static_fn int eval_expr(struct test *tp, int flag) {
+static_fn int eval_expr(Shell_t *shp, struct test *tp, int flag) {
     int r;
     char *p;
 
-    r = eval_e3(tp);
+    r = eval_e3(shp, tp);
     while (tp->ap < tp->ac) {
         p = nxtarg(tp, 0);
         // Check for -o and -a.
@@ -254,10 +254,10 @@ static_fn int eval_expr(struct test *tp, int flag) {
                     tp->ap--;
                     break;
                 }
-                r |= eval_expr(tp, 3);
+                r |= eval_expr(shp, tp, 3);
                 continue;
             } else if (*p == 'a') {
-                r &= eval_expr(tp, 2);
+                r &= eval_expr(shp, tp, 2);
                 continue;
             }
         }
@@ -280,15 +280,15 @@ static_fn char *nxtarg(struct test *tp, int mt) {
     return tp->av[tp->ap++];
 }
 
-static_fn int eval_e3(struct test *tp) {
+static_fn int eval_e3(Shell_t *shp, struct test *tp) {
     char *arg, *cp;
     int op;
     char *binop;
 
     arg = nxtarg(tp, 0);
-    if (c_eq(arg, '!')) return !eval_e3(tp);
+    if (c_eq(arg, '!')) return !eval_e3(shp, tp);
     if (c_eq(arg, '(')) {
-        op = eval_expr(tp, 1);
+        op = eval_expr(shp, tp, 1);
         cp = nxtarg(tp, 0);
         if (!cp || !c_eq(cp, ')')) {
             errormsg(SH_DICT, ERROR_exit(2), e_missing, "')'");
@@ -301,11 +301,14 @@ static_fn int eval_e3(struct test *tp) {
     if (c2_eq(arg, '-', 't')) {
         if (cp) {
             op = strtol(cp, &binop, 10);
-            return *binop ? 0 : tty_check(op);
+            if (*binop) return 0;
+            if (shp->subshell && op == STDOUT_FILENO) return 0;
+            return tty_check(op);
         }
         // Test -t with no arguments.
         tp->ap--;
-        return tty_check(1);
+        if (shp->subshell) return 0;
+        return tty_check(STDOUT_FILENO);
     }
     if (*arg == '-' && arg[2] == 0) {
         op = arg[1];
@@ -441,7 +444,9 @@ int test_unop(Shell_t *shp, int op, const char *arg) {
         case 't': {
             char *last;
             op = strtol(arg, &last, 10);
-            return *last ? 0 : tty_check(op);
+            if (*last) return 0;
+            if (shp->subshell && op == STDOUT_FILENO) return 0;
+            return tty_check(op);
         }
         case 'v':
         case 'R': {
diff --git a/src/cmd/ksh93/tests/basic.sh b/src/cmd/ksh93/tests/basic.sh
index 32bc877d8c44..9426dc060c28 100755
--- a/src/cmd/ksh93/tests/basic.sh
+++ b/src/cmd/ksh93/tests/basic.sh
@@ -421,28 +421,33 @@ expect=$'x\ny\nz'
 for tee in "$(whence tee)" $bin_tee
 do
     print xxx > $TEST_DIR/file
-    $tee  >(sleep 1;cat > $TEST_DIR/file) <<< "hello" > /dev/null
-    [[ $(< $TEST_DIR/file) == hello ]] ||
-        log_error "process substitution does not wait for >() to complete with $tee"
+    $tee  >(sleep 1; cat > $TEST_DIR/file) <<< "hello" > /dev/null
+    actual=$(< $TEST_DIR/file)
+    expect=hello
+    [[ $actual == $expect ]] ||
+        log_error "process substitution does not wait for >() to complete with $tee" "$expect" "$actual"
+
     print yyy > $TEST_DIR/file2
-    $tee >(cat > $TEST_DIR/file) >(sleep 1;cat > $TEST_DIR/file2) <<< "hello" > /dev/null
-    [[ $(< $TEST_DIR/file2) == hello ]] ||
-        log_error "process substitution does not wait for second of two >() to complete with $tee"
+    $tee >(cat > $TEST_DIR/file) >(sleep 1; cat > $TEST_DIR/file2) <<< "hello" > /dev/null
+    actual=$(< $TEST_DIR/file2)
+    expect=hello
+    [[ $actual == $expect ]] ||
+        log_error "process substitution does not wait for second of two >() to complete with $tee" "$expect" "$actual"
+
     print xxx > $TEST_DIR/file
-    $tee  >(sleep 1;cat > $TEST_DIR/file) >(cat > $TEST_DIR/file2) <<< "hello" > /dev/null
-    [[ $(< $TEST_DIR/file) == hello ]] ||
-        log_error "process substitution does not wait for first of two >() to complete with $tee"
+    $tee  >(sleep 1; cat > $TEST_DIR/file) >(cat > $TEST_DIR/file2) <<< "hello" > /dev/null
+    actual=$(< $TEST_DIR/file)
+    expect=hello
+    [[ $actual == $expect ]] ||
+        log_error "process substitution does not wait for first of two >() to complete with $tee" "$expect" "$actual"
 done
 
-if [[ -d /dev/fd ]]
+if [[ $HAS_DEV_FD == yes ]]
 then
-    if [[ $(print <(print foo) & sleep .5; kill $! 2>/dev/null) == /dev/fd/* ]]
-    then
-        expect='/dev/fd/+(\d) v=bam /dev/fd/+(\d)'
-        actual=$( print <(print foo) v=bam <(print bar))
-        [[ $actual == $expect ]] ||
-            log_error 'assignments after command subst not treated as arguments' "$expect" "$actual"
-    fi
+    expect='/dev/fd/+(\d) v=bam /dev/fd/+(\d)'
+    actual=$( print <(print foo) v=bam <(print bar))
+    [[ $actual == $expect ]] ||
+        log_error 'assignments after command substitution not treated as arguments' "$expect" "$actual"
 fi
 
 # ========
diff --git a/src/cmd/ksh93/tests/test.exp b/src/cmd/ksh93/tests/test.exp
index 00a6c14a00f8..03d630082d42 100644
--- a/src/cmd/ksh93/tests/test.exp
+++ b/src/cmd/ksh93/tests/test.exp
@@ -19,6 +19,28 @@ expect "\r\nPASS\r\n" {
 }
 expect_prompt
 
+# ==========
+# Verify that constructs like `[ -t 1 ]` behave sensibly even inside a command substitution.
+
+# 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.
+send {v=$(echo begin; [ -t 1 ] && echo -t1 is true; echo end); echo "v=:$v:"}
+send "\r"
+expect "\r\nv=:begin\r\nend:\r\n" {
+    puts "-t1 in comsub works correctly"
+}
+expect_prompt
+
+# 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.
+send {v=$(echo begin; exec >/dev/tty; [ -t 1 ] && echo -t1 is true; echo end); echo "v=:$v:"}
+send "\r"
+expect "\r\n-t1 is true\r\nend\r\nv=:begin:\r\n" {
+    puts "-t1 in comsub with exec >/dev/tty works correctly"
+}
+expect_prompt
+
+# ==========
 # Exit shell with ctrl-d
 log_test_entry
 send [ctrl D]
diff --git a/src/cmd/ksh93/tests/test.exp.out b/src/cmd/ksh93/tests/test.exp.out
index 26dfb94ddf4a..ab5e8d71e38f 100644
--- a/src/cmd/ksh93/tests/test.exp.out
+++ b/src/cmd/ksh93/tests/test.exp.out
@@ -1,2 +1,4 @@
 stdin is a terminal device
 stdout is a terminal device
+-t1 in comsub works correctly
+-t1 in comsub with exec >/dev/tty works correctly
diff --git a/src/cmd/ksh93/tests/util/preamble.sh b/src/cmd/ksh93/tests/util/preamble.sh
index d55b5fbed0a5..20d8171e1cc8 100644
--- a/src/cmd/ksh93/tests/util/preamble.sh
+++ b/src/cmd/ksh93/tests/util/preamble.sh
@@ -85,6 +85,19 @@ function empty_fifos {
 }
 alias empty_fifos='empty_fifos $LINENO'
 
+#
+# Verify that /dev/fd is functional. Note that on some systems (e.g., FreeBSD) there is a stub
+# /dev/fd that only supports 0, 1, and 2. On such systems a special pseudo-filesystem may need to
+# be mounted or a custom kernel built to get full /dev/fd support.
+#
+# Note that we can't do the straightforward `[[ -p /dev/fd/8 ]]` because such paths are
+# special-cased by ksh and work even if the system doesn't support /dev/fd. But there may be tests
+# where we need to know if those paths are recognized by the OS.
+#
+HAS_DEV_FD=no
+[[ $(print /dev/fd/*) == *' /dev/fd/8 '* ]] && HAS_DEV_FD=yes
+readonly HAS_DEV_FD
+
 #
 # Platforms like OpenBSD have `jot` instead of `seq`. For the simple case of emitting ints from one
 # to n they are equivalent. And that should be the only use of `seq` in unit tests.