dash.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* SEGV parsing of ${#1\x82} and ${#1\x84}
@ 2021-06-19 12:44 Denys Vlasenko
  2021-06-21  9:57 ` [PATCH] parser: Fix VSLENGTH parsing with trailing garbage Herbert Xu
  0 siblings, 1 reply; 5+ messages in thread
From: Denys Vlasenko @ 2021-06-19 12:44 UTC (permalink / raw)
  To: Herbert Xu, DASH shell mailing list

Hi,

CTLVAR and CTLBACKQ are not properly handled if encountered
inside {$#...}. Testcase:

dash -c "`printf 'echo ${#1\x82}'`" 00 111 222

It should execute "echo ${#1 <byte 0x82> }" and thus print "3"
(the length of $1, which is "111").

Instead, it segfaults.

(Ideally, it should fail since "1 <byte 0x82>" is not a valid
variable name, but currently dash accepts e.g. "${#1abc}"
as if it is "${#1}bc". A separate, less serious bug...).

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] parser: Fix VSLENGTH parsing with trailing garbage
  2021-06-19 12:44 SEGV parsing of ${#1\x82} and ${#1\x84} Denys Vlasenko
@ 2021-06-21  9:57 ` Herbert Xu
  2021-06-21 14:21   ` Denys Vlasenko
  0 siblings, 1 reply; 5+ messages in thread
From: Herbert Xu @ 2021-06-21  9:57 UTC (permalink / raw)
  To: Denys Vlasenko; +Cc: DASH shell mailing list

On Sat, Jun 19, 2021 at 02:44:46PM +0200, Denys Vlasenko wrote:
> 
> CTLVAR and CTLBACKQ are not properly handled if encountered
> inside {$#...}. Testcase:
> 
> dash -c "`printf 'echo ${#1\x82}'`" 00 111 222
> 
> It should execute "echo ${#1 <byte 0x82> }" and thus print "3"
> (the length of $1, which is "111").
> 
> Instead, it segfaults.
> 
> (Ideally, it should fail since "1 <byte 0x82>" is not a valid
> variable name, but currently dash accepts e.g. "${#1abc}"
> as if it is "${#1}bc". A separate, less serious bug...).

In fact these two bugs are one and the same.  This patch fixes
both by detecting the invalid substitution and not emitting it
into the node tree.

Incidentally this reveals a bug in how we parse ${#10} that got
introduced recently, which is also fixed here.

Reported-by: Denys Vlasenko <vda.linux@googlemail.com>
Fixes: 7710a926b321 ("parser: Only accept single-digit parameter...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/src/parser.c b/src/parser.c
index 3c80d17..13c2df5 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1252,7 +1252,8 @@ varname:
 			do {
 				STPUTC(c, out);
 				c = pgetc_eatbnl();
-			} while (!subtype && is_digit(c));
+			} while ((subtype <= 0 || subtype >= VSLENGTH) &&
+				 is_digit(c));
 		} else if (c != '}') {
 			int cc = c;
 
@@ -1312,6 +1313,8 @@ varname:
 				break;
 			}
 		} else {
+			if (subtype == VSLENGTH && c != '}')
+				subtype = 0;
 badsub:
 			pungetc();
 		}
diff --git a/src/parser.h b/src/parser.h
index 524ac1c..7d2749b 100644
--- a/src/parser.h
+++ b/src/parser.h
@@ -62,6 +62,7 @@
 #define VSTRIMLEFT	0x8		/* ${var#pattern} */
 #define VSTRIMLEFTMAX	0x9		/* ${var##pattern} */
 #define VSLENGTH	0xa		/* ${#var} */
+/* VSLENGTH must come last. */
 
 /* values of checkkwd variable */
 #define CHKALIAS	0x1
-- 
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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] parser: Fix VSLENGTH parsing with trailing garbage
  2021-06-21  9:57 ` [PATCH] parser: Fix VSLENGTH parsing with trailing garbage Herbert Xu
@ 2021-06-21 14:21   ` Denys Vlasenko
  2021-06-22  0:19     ` Herbert Xu
  0 siblings, 1 reply; 5+ messages in thread
From: Denys Vlasenko @ 2021-06-21 14:21 UTC (permalink / raw)
  To: Herbert Xu; +Cc: DASH shell mailing list

On Mon, Jun 21, 2021 at 11:57 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Sat, Jun 19, 2021 at 02:44:46PM +0200, Denys Vlasenko wrote:
> > CTLVAR and CTLBACKQ are not properly handled if encountered
> > inside {$#...}. Testcase:
> >
> > dash -c "`printf 'echo ${#1\x82}'`" 00 111 222
...
> In fact these two bugs are one and the same.  This patch fixes
> both by detecting the invalid substitution and not emitting it
> into the node tree.
>
> Incidentally this reveals a bug in how we parse ${#10} that got
> introduced recently, which is also fixed here.
...
> --- a/src/parser.h
> +++ b/src/parser.h
> @@ -62,6 +62,7 @@
>  #define VSTRIMLEFT     0x8             /* ${var#pattern} */
>  #define VSTRIMLEFTMAX  0x9             /* ${var##pattern} */
>  #define VSLENGTH       0xa             /* ${#var} */
> +/* VSLENGTH must come last. */

The above assumption is not necessary if...


> diff --git a/src/parser.c b/src/parser.c
> index 3c80d17..13c2df5 100644
> --- a/src/parser.c
> +++ b/src/parser.c
> @@ -1252,7 +1252,8 @@ varname:
>                         do {
>                                 STPUTC(c, out);
>                                 c = pgetc_eatbnl();
> -                       } while (!subtype && is_digit(c));
> +                       } while ((subtype <= 0 || subtype >= VSLENGTH) &&
> +                                is_digit(c));

... you use (subtype == 0 || subtype == VSLENGTH) here.
Also, (subtype == 0 || subtype == VSLENGTH) is less confusing:
it says "loop if ${VAR} or ${#VAR} syntax", whereas <= >=
are a bit misleading.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] parser: Fix VSLENGTH parsing with trailing garbage
  2021-06-21 14:21   ` Denys Vlasenko
@ 2021-06-22  0:19     ` Herbert Xu
  2021-06-22  8:34       ` Denys Vlasenko
  0 siblings, 1 reply; 5+ messages in thread
From: Herbert Xu @ 2021-06-22  0:19 UTC (permalink / raw)
  To: Denys Vlasenko; +Cc: DASH shell mailing list

On Mon, Jun 21, 2021 at 04:21:40PM +0200, Denys Vlasenko wrote:
>
> > diff --git a/src/parser.c b/src/parser.c
> > index 3c80d17..13c2df5 100644
> > --- a/src/parser.c
> > +++ b/src/parser.c
> > @@ -1252,7 +1252,8 @@ varname:
> >                         do {
> >                                 STPUTC(c, out);
> >                                 c = pgetc_eatbnl();
> > -                       } while (!subtype && is_digit(c));
> > +                       } while ((subtype <= 0 || subtype >= VSLENGTH) &&
> > +                                is_digit(c));
> 
> ... you use (subtype == 0 || subtype == VSLENGTH) here.
> Also, (subtype == 0 || subtype == VSLENGTH) is less confusing:
> it says "loop if ${VAR} or ${#VAR} syntax", whereas <= >=
> are a bit misleading.

Yes it looks a bit confusing, but it turns into a single branch
instead of two.  Perhaps I should add a helper function for it.

Thanks,
-- 
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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] parser: Fix VSLENGTH parsing with trailing garbage
  2021-06-22  0:19     ` Herbert Xu
@ 2021-06-22  8:34       ` Denys Vlasenko
  0 siblings, 0 replies; 5+ messages in thread
From: Denys Vlasenko @ 2021-06-22  8:34 UTC (permalink / raw)
  To: Herbert Xu; +Cc: DASH shell mailing list

On Tue, Jun 22, 2021 at 2:19 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Mon, Jun 21, 2021 at 04:21:40PM +0200, Denys Vlasenko wrote:
> > > -                       } while (!subtype && is_digit(c));
> > > +                       } while ((subtype <= 0 || subtype >= VSLENGTH) &&
> > > +                                is_digit(c));
> >
> > ... you use (subtype == 0 || subtype == VSLENGTH) here.
> > Also, (subtype == 0 || subtype == VSLENGTH) is less confusing:
> > it says "loop if ${VAR} or ${#VAR} syntax", whereas <= >=
> > are a bit misleading.
>
> Yes it looks a bit confusing, but it turns into a single branch
> instead of two.

Yes, I know that. Compiler turns it into "(unsigned)(x-1) >= VSLENGTH-1"
expression.

But is it worth the obfuscation? Especially that it also has another
downside (it requires an additional free CPU register to hold (x-1)
result, which can force compiler to spill other values to stack).

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-06-22  8:35 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-19 12:44 SEGV parsing of ${#1\x82} and ${#1\x84} Denys Vlasenko
2021-06-21  9:57 ` [PATCH] parser: Fix VSLENGTH parsing with trailing garbage Herbert Xu
2021-06-21 14:21   ` Denys Vlasenko
2021-06-22  0:19     ` Herbert Xu
2021-06-22  8:34       ` Denys Vlasenko

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).