On 09/04/13 05:27, Eric Blake wrote:
On 04/08/2013 09:12 PM, Dan Kegel wrote:
Yes, my script was crap, I've fixed it.
Here's the reproducer. Called with foo unset. I think it doesn't
crash without -x.
#!/bin/dash
set -x
test ! $foo
The 'set -x' was indeed the key to reproducing the problem. In fact,
this is the shortest I could make it:
dash -cx 'test !'
It is not limited to 'set -x'. dash continues reading after the NULL
value in argv, and usually that will be followed by another NULL if 'set
-x' is not used, but not necessarily.
$ dash -c 'test ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! !
! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! !'
$ dash -c 'test ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! !
! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! !'
Segmentation fault
$ dash -c 'test ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! !
! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! !'
$
When [ ! ] is used, the ! is necessarily followed by two NULLs (one
after the ], and one because the ] is replaced by NULL), so the problem
is hidden.
dash should check whether ! is followed by an argument, like bash does,
which would give an error message without a segmentation fault for all
three forms above.
This seems to be easily possible by manually inlining primary() into
nexpr(), and treating UNOT similarly to STREZ e.a.:
diff --git a/src/bltin/test.c b/src/bltin/test.c
index 90135e1..5cf4021 100644
--- a/src/bltin/test.c
+++ b/src/bltin/test.c
@@ -268,14 +268,6 @@ aexpr(enum token n)
static int
nexpr(enum token n)
{
- if (n == UNOT)
- return !nexpr(t_lex(++t_wp));
- return primary(n);
-}
-
-static int
-primary(enum token n)
-{
enum token nn;
int res;
@@ -289,11 +281,13 @@ primary(enum token n)
syntax(NULL, "closing paren expected");
return res;
}
- if (t_wp_op && t_wp_op->op_type == UNOP) {
+ if (t_wp_op && (t_wp_op->op_type == UNOP || t_wp_op->op_type ==
BUNOP)) {
/* unary expression */
if (*++t_wp == NULL)
syntax(t_wp_op->op_text, "argument expected");
switch (n) {
+ case UNOT:
+ return !nexpr(t_lex(t_wp));
case STREZ:
return strlen(*t_wp) == 0;
case STRNZ:
Unfortunately, this exposes the fact that POSIX test requires special
behaviour when called with fewer than five arguments. This change would
cause "test !" to start returning an error. Something like
diff --git a/src/bltin/test.c b/src/bltin/test.c
index 5cf4021..1e84423 100644
--- a/src/bltin/test.c
+++ b/src/bltin/test.c
@@ -177,6 +177,7 @@ testcmd(int argc, char **argv)
{
const struct t_op *op;
enum token n;
+ int not;
int res;
if (*argv[0] == '[') {
@@ -191,25 +192,44 @@ testcmd(int argc, char **argv)
if (argc < 1)
return 1;
+ not = 0;
+
/*
* POSIX prescriptions: he who wrote this deserves the Nobel
* peace prize.
*/
- switch (argc) {
- case 3:
- op = getop(argv[1]);
- if (op && op->op_type == BINOP) {
- n = OPERAND;
- goto eval;
+ for (;;) {
+ switch (argc) {
+ case 1:
+ res = strlen(argv[0]) == 0;
+ goto exit;
+
+ case 3:
+ op = getop(argv[1]);
+ if (op && op->op_type == BINOP) {
+ n = OPERAND;
+ goto eval;
+ }
+ /* fall through */
+
+ case 2:
+ case 4:
+ if (!strcmp(argv[0], "!")) {
+ not = !not;
+ argv++;
+ argc--;
+ continue;
+ }
+
+ if (!strcmp(argv[0], "(") && !strcmp(argv[argc -
1], ")")) {
+ argv[--argc] = NULL;
+ argv++;
+ argc--;
+ continue;
+ }
}
- /* fall through */
- case 4:
- if (!strcmp(argv[0], "(") && !strcmp(argv[argc - 1], ")")) {
- argv[--argc] = NULL;
- argv++;
- argc--;
- }
+ break;
}
n = t_lex(argv);
@@ -222,6 +242,10 @@ eval:
if (argv[0] != NULL && argv[1] != NULL)
syntax(argv[0], "unexpected operator");
+exit:
+ if (not)
+ res = !res;
+
return res;
}
Although this is a bit ugly, it gets the right results:
$ ./dash -c 'test'; echo $?
1
$ ./dash -c 'test !'; echo $?
0
$ ./dash -c 'test ! !'; echo $?
1
$ ./dash -c 'test ! ! !'; echo $?
0
$ ./dash -c 'test ! ! ! !'; echo $?
1
$ ./dash -c 'test ! ! ! ! !'; echo $?
./dash: 1: test: !: argument expected
2
One comment about this approach: to keep the code slightly simpler, it
also removes parentheses in two-argument calls to test. That is not
standard behaviour, but the standard leaves the behaviour unspecified,
so it is valid. This causes dash to return 1 for "test \( \)", and
previous versions of dash already returned 1 for other reasons.
Cheers,
Harald
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
