* Bug: [ "(" = ")" ] is true @ 2015-12-09 15:34 Martijn Dekker 2015-12-09 15:58 ` Stephane Chazelas 2015-12-09 16:24 ` Peter Stephenson 0 siblings, 2 replies; 5+ messages in thread From: Martijn Dekker @ 2015-12-09 15:34 UTC (permalink / raw) To: zsh-workers 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 $ [ ")" = "(" ] && echo oops || echo ok ok $ [ "((" = "))" ] && echo oops || echo ok oops $ [ "((" = ")x" ] && echo oops || echo ok oops $ [ "(x" = ")" ] && echo oops || echo ok oops $ [ "x(" = ")" ] && echo oops || echo ok ok $ [ "(" = "x)" ] && echo oops || echo ok ok This appears to be a long-standing bug. I confirmed it in: zsh 4.3.6 zsh 5.1.1 zsh 5.2 I also found that the bug does *not* exist in zsh 4.1.1. Also, `[[' does not have this problem. Credit to /u/tilkau on reddit for discovering this bug: https://www.reddit.com/r/bash/comments/3w1hm4/how_to_compare_values_to_literal/cxsknzh Thanks, - Martijn ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bug: [ "(" = ")" ] is true 2015-12-09 15:34 Bug: [ "(" = ")" ] is true Martijn Dekker @ 2015-12-09 15:58 ` Stephane Chazelas 2015-12-09 16:31 ` Martijn Dekker 2015-12-09 16:24 ` Peter Stephenson 1 sibling, 1 reply; 5+ messages in thread From: Stephane Chazelas @ 2015-12-09 15:58 UTC (permalink / raw) To: Martijn Dekker; +Cc: zsh-workers 2015-12-09 16:34:34 +0100, Martijn Dekker: > 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 > $ [ ")" = "(" ] && echo oops || echo ok > ok > $ [ "((" = "))" ] && echo oops || echo ok > oops > $ [ "((" = ")x" ] && echo oops || echo ok > oops > $ [ "(x" = ")" ] && echo oops || echo ok > oops > $ [ "x(" = ")" ] && echo oops || echo ok > ok > $ [ "(" = "x)" ] && echo oops || echo ok > ok > > This appears to be a long-standing bug. I confirmed it in: > > zsh 4.3.6 > zsh 5.1.1 > zsh 5.2 > > I also found that the bug does *not* exist in zsh 4.1.1. [...] http://www.zsh.org/mla/users/2007/msg01223.html seems to be to blame. If the first argument starts with a "(" and the last one starts in a ")", they are both removed. So all operators are affected not just "=" ([ "(file1)" -nt ")file2(" ]...) -- Stephane ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bug: [ "(" = ")" ] is true 2015-12-09 15:58 ` Stephane Chazelas @ 2015-12-09 16:31 ` Martijn Dekker 2015-12-09 20:27 ` Bart Schaefer 0 siblings, 1 reply; 5+ messages in thread From: Martijn Dekker @ 2015-12-09 16:31 UTC (permalink / raw) To: zsh-workers Stephane Chazelas schreef op 09-12-15 om 16:58: > http://www.zsh.org/mla/users/2007/msg01223.html seems to be to > blame. > > If the first argument starts with a "(" and the last one starts in > a ")", they are both removed. > > So all operators are affected not just "=" ([ "(file1)" -nt > ")file2(" ]...) Wow. Not only that, the results are incorrect even after the removal. $ test '(1' -eq ')2' && echo oops || echo ok oops $ test '1' -eq '2' && echo oops || echo ok ok - M. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bug: [ "(" = ")" ] is true 2015-12-09 16:31 ` Martijn Dekker @ 2015-12-09 20:27 ` Bart Schaefer 0 siblings, 0 replies; 5+ messages in thread From: Bart Schaefer @ 2015-12-09 20:27 UTC (permalink / raw) To: zsh-workers On Dec 9, 5:31pm, Martijn Dekker wrote: } } Wow. } } Not only that, the results are incorrect even after the removal. } } $ test '(1' -eq ')2' && echo oops || echo ok } oops That's because the parens weren't removed, the entire argument that contains each paren was removed. [ \(1 -eq \)2 ] was becoming [ -eq ]. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bug: [ "(" = ")" ] is true 2015-12-09 15:34 Bug: [ "(" = ")" ] is true Martijn Dekker 2015-12-09 15:58 ` Stephane Chazelas @ 2015-12-09 16:24 ` Peter Stephenson 1 sibling, 0 replies; 5+ messages in thread From: Peter Stephenson @ 2015-12-09 16:24 UTC (permalink / raw) To: zsh-workers On Wed, 9 Dec 2015 16:34:34 +0100 Martijn Dekker <martijn@inlv.org> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-09 20:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-09 15:34 Bug: [ "(" = ")" ] is true Martijn Dekker 2015-12-09 15:58 ` Stephane Chazelas 2015-12-09 16:31 ` Martijn Dekker 2015-12-09 20:27 ` Bart Schaefer 2015-12-09 16:24 ` Peter Stephenson
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).