Re: Bug: [ "(" = ")" ] is true

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Wed, 9 Dec 2015 16:34:34 +0100
Martijn Dekker <martijn@xxxxxxxx> wrote:
> There is a string comparison bug with `[' and `test'; the result is true
> if the first string starts with '(' and the second string starts with ')'.
> 
> $ [ "(" = ")" ] && echo oops || echo ok
> oops

This is yet another result of a folorn attempt to handle extensions of
the "[" syntax consistently.  We can't tell if "(" ... ")" are for group
or are arguments to a command, so we guess.  However, as groups are an
extension of the most basic syntax, we should first try the latter.

Note that

[ "(" string ")" ]

and

[ "(" "" ")" ]

are valid tests that return true and false respectively.  (They tend to
suggest that whoever wrote those tests has lost the plot, but, well.)

> $ [ "((" = "))" ] && echo oops || echo ok
> oops

This seems to be a more obvious bug that we only care about the first
character.  I can't see how that could ever possibly be right.  It's
possible there may have been some confusion over the fact that if we
were parsing this properly for "[[" then the "(" and ")" wouldn't
need to be in separate words, but that doesn't apply to "test" / "[".

Added some "const"s in text.c with a little trepidation --- but I think
it's a long time since we had problems with older OSes in that department.

diff --git a/Src/builtin.c b/Src/builtin.c
index cac4f42..b06bc6d 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -6463,7 +6463,13 @@ bin_test(char *name, char **argv, UNUSED(Options ops), int func)
     nargs = arrlen(argv);
     if (nargs == 3 || nargs == 4)
     {
-	if (*argv[0] == '(' && *argv[nargs-1] == ')') {
+	/*
+	 * As parentheses are an extension, we need to be careful ---
+	 * if this is a three-argument expression that could
+	 * be a binary operator, prefer that.
+	 */
+	if (!strcmp(argv[0], "(") && !strcmp(argv[nargs-1],")") &&
+	    (nargs != 3 || !is_cond_binary_op(argv[1]))) {
 	    argv[nargs-1] = NULL;
 	    argv++;
 	}
diff --git a/Src/text.c b/Src/text.c
index 9421d70..04acd2a 100644
--- a/Src/text.c
+++ b/Src/text.c
@@ -40,9 +40,32 @@
 /**/
 int text_expand_tabs;
 
+/*
+ * Binary operators in conditions.
+ * There order is tied to the order of the definitions COND_STREQ
+ * et seq. in zsh.h.
+ */
+static const char *cond_binary_ops[] = {
+    "=", "!=", "<", ">", "-nt", "-ot", "-ef", "-eq",
+    "-ne", "-lt", "-gt", "-le", "-ge", "=~"
+};
+
 static char *tptr, *tbuf, *tlim, *tpending;
 static int tsiz, tindent, tnewlins, tjob;
 
+/**/
+int
+is_cond_binary_op(const char *str)
+{
+    const char **op;
+    for (op = cond_binary_ops; *op; op++)
+    {
+	if (!strcmp(str, *op))
+	    return 1;
+    }
+    return 0;
+}
+
 static void
 dec_tindent(void)
 {
@@ -120,7 +143,7 @@ taddchr(int c)
 
 /**/
 static void
-taddstr(char *s)
+taddstr(const char *s)
 {
     int sl = strlen(s);
     char c;
@@ -822,11 +845,6 @@ gettext2(Estate state)
 	    break;
 	case WC_COND:
 	    {
-		static char *c1[] = {
-		    "=", "!=", "<", ">", "-nt", "-ot", "-ef", "-eq",
-		    "-ne", "-lt", "-gt", "-le", "-ge", "=~"
-		};
-
 		int ctype;
 
 		if (!s) {
@@ -912,7 +930,7 @@ gettext2(Estate state)
 			    /* Binary test: `a = b' etc. */
 			    taddstr(ecgetstr(state, EC_NODUP, NULL));
 			    taddstr(" ");
-			    taddstr(c1[ctype - COND_STREQ]);
+			    taddstr(cond_binary_ops[ctype - COND_STREQ]);
 			    taddstr(" ");
 			    taddstr(ecgetstr(state, EC_NODUP, NULL));
 			    if (ctype == COND_STREQ ||
diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst
index 40bbf42..9e13696 100644
--- a/Test/C02cond.ztst
+++ b/Test/C02cond.ztst
@@ -389,6 +389,18 @@ F:Failures in these cases do not indicate a problem in the shell.
 >Not zero 5
 >Not zero 6
 
+  [ '(' = ')' ] || print OK 1
+  [ '((' = '))' ] || print OK 2
+  [ '(' = '(' ] && print OK 3
+  [ '(' non-empty-string ')' ] && echo OK 4
+  [ '(' '' ')' ] || echo OK 5
+0:yet more old-fashioned test fix ups: prefer comparison to parentheses
+>OK 1
+>OK 2
+>OK 3
+>OK 4
+>OK 5
+
 %clean
   # This works around a bug in rm -f in some versions of Cygwin
   chmod 644 unmodish