Crash on valid input

Crash on valid input

[Dan Kegel]

If I check for an empty string like this:

+ test ! $foo

dash crashes.  This occurs both in the version shipped with ubuntu
10.04 and 12.04
as well as with dash from git.

Here's the stack:

Program received signal SIGSEGV, Segmentation fault.
__strcmp_sse4_2 () at ../sysdeps/i386/i686/multiarch/strcmp-sse4.S:221
221 ../sysdeps/i386/i686/multiarch/strcmp-sse4.S: No such file or directory.
(gdb) bt
#0  __strcmp_sse4_2 () at ../sysdeps/i386/i686/multiarch/strcmp-sse4.S:221
#1  0x0805938a in getop (s=0x202b <Address 0x202b out of bounds>) at
bltin/test.c:168
#2  0x08059c91 in t_lex (tp=0x806581c) at bltin/test.c:431
#3  0x080595cb in aexpr (n=UNOT) at bltin/test.c:260
#4  0x08059557 in oexpr (n=UNOT) at bltin/test.c:243
#5  0x080594ba in testcmd (argc=1, argv=0x8065814) at bltin/test.c:219
#6  0x0804c526 in evalbltin (cmd=0x805da1c, argc=2, argv=0x8065810,
flags=0) at eval.c:910
#7  0x0804c383 in evalcommand (cmd=0x80657cc, flags=0) at eval.c:850
#8  0x0804b4ab in evaltree (n=0x80657cc, flags=0) at eval.c:280
#9  0x08052d72 in cmdloop (top=1) at main.c:238
#10 0x08052c76 in main (argc=2, argv=0xbffff944) at main.c:178
(gdb) frame 2
#2  0x08059c91 in t_lex (tp=0x806581c) at bltin/test.c:431
431 op = getop(s);
(gdb) print s
$1 = 0x202b <Address 0x202b out of bounds>

Thanks for dash!

Re: Crash on valid input

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

On 04/08/2013 08:43 PM, Dan Kegel wrote:
> If I check for an empty string like this:
> 
> + test ! $foo

You know, this is highly reliant on $foo expanding to either nothing (it
is empty, so you are really invoking the one-argument form 'test !'
which is true) or a single shell word (the two-argument form 'test !
nonempty' is false).  But if $foo has embedded whitespace, it expands to
more than one word after word-splitting, and you're hosed ('test ! one
two' can give interesting results, and probably not what you were
expecting).

You might want to fix your script to just use the safer:

! test "$foo"

which works even if $foo has embedded whitespace.  That said, you are
correct that dash should never crash.

> 
> dash crashes.  This occurs both in the version shipped with ubuntu
> 10.04 and 12.04
> as well as with dash from git.
> 
> Here's the stack:
> 
> Program received signal SIGSEGV, Segmentation fault.
> __strcmp_sse4_2 () at ../sysdeps/i386/i686/multiarch/strcmp-sse4.S:221
> 221 ../sysdeps/i386/i686/multiarch/strcmp-sse4.S: No such file or directory.
> (gdb) bt
> #0  __strcmp_sse4_2 () at ../sysdeps/i386/i686/multiarch/strcmp-sse4.S:221
> #1  0x0805938a in getop (s=0x202b <Address 0x202b out of bounds>) at
> bltin/test.c:168
> #2  0x08059c91 in t_lex (tp=0x806581c) at bltin/test.c:431
> #3  0x080595cb in aexpr (n=UNOT) at bltin/test.c:260
> #4  0x08059557 in oexpr (n=UNOT) at bltin/test.c:243
> #5  0x080594ba in testcmd (argc=1, argv=0x8065814) at bltin/test.c:219
> #6  0x0804c526 in evalbltin (cmd=0x805da1c, argc=2, argv=0x8065810,
> flags=0) at eval.c:910

and given this part of the stack trace, it looks like you were invoking
'test ! word'?  But I was unable to reproduce a crash when I tried
dash.git, so it would be helpful to know exactly what $foo was in your
reproducer.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: Crash on valid input

[Dan Kegel]

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



On Mon, Apr 8, 2013 at 8:08 PM, Eric Blake <eblake@redhat.com> wrote:
> On 04/08/2013 08:43 PM, Dan Kegel wrote:
>> If I check for an empty string like this:
>>
>> + test ! $foo
>
> You know, this is highly reliant on $foo expanding to either nothing (it
> is empty, so you are really invoking the one-argument form 'test !'
> which is true) or a single shell word (the two-argument form 'test !
> nonempty' is false).  But if $foo has embedded whitespace, it expands to
> more than one word after word-splitting, and you're hosed ('test ! one
> two' can give interesting results, and probably not what you were
> expecting).
>
> You might want to fix your script to just use the safer:
>
> ! test "$foo"
>
> which works even if $foo has embedded whitespace.  That said, you are
> correct that dash should never crash.
>
>>
>> dash crashes.  This occurs both in the version shipped with ubuntu
>> 10.04 and 12.04
>> as well as with dash from git.
>>
>> Here's the stack:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> __strcmp_sse4_2 () at ../sysdeps/i386/i686/multiarch/strcmp-sse4.S:221
>> 221 ../sysdeps/i386/i686/multiarch/strcmp-sse4.S: No such file or directory.
>> (gdb) bt
>> #0  __strcmp_sse4_2 () at ../sysdeps/i386/i686/multiarch/strcmp-sse4.S:221
>> #1  0x0805938a in getop (s=0x202b <Address 0x202b out of bounds>) at
>> bltin/test.c:168
>> #2  0x08059c91 in t_lex (tp=0x806581c) at bltin/test.c:431
>> #3  0x080595cb in aexpr (n=UNOT) at bltin/test.c:260
>> #4  0x08059557 in oexpr (n=UNOT) at bltin/test.c:243
>> #5  0x080594ba in testcmd (argc=1, argv=0x8065814) at bltin/test.c:219
>> #6  0x0804c526 in evalbltin (cmd=0x805da1c, argc=2, argv=0x8065810,
>> flags=0) at eval.c:910
>
> and given this part of the stack trace, it looks like you were invoking
> 'test ! word'?  But I was unable to reproduce a crash when I tried
> dash.git, so it would be helpful to know exactly what $foo was in your
> reproducer.
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: Crash on valid input

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 468 bytes --]

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 !'

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: Crash on valid input

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

On 04/08/2013 09:27 PM, 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 !'

If it helps, dash -cx '[ ! ]' does not crash.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: Crash on valid input

[Dan Kegel]

Yeah, even valgrind doesn't see anything without -x.


On Mon, Apr 8, 2013 at 8:34 PM, Eric Blake <eblake@redhat.com> wrote:
> On 04/08/2013 09:27 PM, 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 !'
>
> If it helps, dash -cx '[ ! ]' does not crash.
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: Crash on valid input

[Harald van Dijk]

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

Re: Crash on valid input

[Herbert Xu]

On Tue, Apr 09, 2013 at 09:20:52PM +0000, Harald van Dijk wrote:
> 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.:

Thanks for the patch.  However, I've decided to fix it like this:

commit b34499f5c851d1a70db95b56bd02eff0329d4a1a
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Fri Aug 23 21:58:55 2013 +1000

    [BUILTIN] Fixed argument parsing crash in test
    
    When nexpr gets an unexpected EOI, this may cause crashes further
    up the call chain because we've advanced t_wp too far.  Fix it by
    checking for EOI in nexpr and only advancing t_wp if we've got more
    arguments.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index 4276676..2a39e34 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,6 +2,7 @@
 
 	* Propagate EXP_QPAT in subevalvar.
 	* Initialise OPTIND after importing environment.
+	* Fixed argument parsing crash in test.
 
 2013-03-12  Peter Rosin <peda@lysator.liu.se>
 
diff --git a/src/bltin/test.c b/src/bltin/test.c
index 90135e1..baa91a5 100644
--- a/src/bltin/test.c
+++ b/src/bltin/test.c
@@ -268,9 +268,13 @@ aexpr(enum token n)
 static int
 nexpr(enum token n)
 {
-	if (n == UNOT)
-		return !nexpr(t_lex(++t_wp));
-	return primary(n);
+	if (n != UNOT)
+		return primary(n);
+
+	n = t_lex(t_wp + 1);
+	if (n != EOI)
+		t_wp++;
+	return !nexpr(n);
 }
 
 static int

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt