parser: don't keep alloca()ing in a loop for substitutions

Commit Message

наб Dec. 14, 2022, 11:39 p.m. UTC

When encountering
  printf %010000d | tr 0 \` | sh -n
  printf %09999d  | tr 0 \` | sh -n
you want no output and "Syntax error: EOF in backquote substitution",
respectively; instead, current dash segfaults.

This is because the alloca for the save buffer is run, naturally,
in the same function, so first it allocates one byte, then two,
then ..., then appx. 4000 (for me, depends on the binary),
then it segfaults on the memcpy (it's even worse, since due to
alignment, it usually allocates much more for the early stuff).

Nevertheless, the stack frame grows unboundedly, until we completely
destroy the stack. Instead, alloca a 1KiB buffer on first use and
fall back to ckmalloc for bigger save buffers. In practice this means
that we'll alloca nothing in a good amount of cases, then just about
always use the alloca buffer except for truly pathological input.

Fixes: https://bugs.debian.org/966156
---
 src/parser.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Herbert Xu Dec. 15, 2022, 10:27 a.m. UTC | #1

наб <[e-mail]> wrote:
>
> diff --git a/src/parser.c b/src/parser.c
> index a552c47..3f7e50a 100644
> --- a/src/parser.c
> +++ b/src/parser.c
> @@ -898,6 +898,7 @@ readtoken1(int firstc, char const *syntax, char *eofmark, int striptabs)
>        struct nodelist *bqlist;
>        int quotef;
>        int oldstyle;
> +       char *parsebackq_save;
>        /* syntax stack */
>        struct synstack synbase = { .syntax = syntax };
>        struct synstack *synstack = &synbase;
> @@ -906,6 +907,7 @@ readtoken1(int firstc, char const *syntax, char *eofmark, int striptabs)
>                synstack->dblquote = 1;
>        quotef = 0;
>        bqlist = NULL;
> +       parsebackq_save = NULL;
> 
>        STARTSTACKSTR(out);
>        loop: { /* for each line, until end of word */
> @@ -1355,15 +1357,18 @@ badsub:
> parsebackq: {
>        struct nodelist **nlpp;
>        union node *n;
> -       char *str;
> +       char *str, *mstr;
>        size_t savelen;
>        struct heredoc *saveheredoclist;
>        int uninitialized_var(saveprompt);
> 
> -       str = NULL;
> +       str = mstr = NULL;
>        savelen = out - (char *)stackblock();
>        if (savelen > 0) {
> -               str = alloca(savelen);
> +               if (savelen > 1024)
> +                       str = mstr = ckmalloc(savelen);
> +               else
> +                       str = parsebackq_save ?: (parsebackq_save = alloca(1024));
>                memcpy(str, stackblock(), savelen);
>        }
>         if (oldstyle) {
> @@ -1449,6 +1454,7 @@ done:
>        if (str) {
>                memcpy(out, str, savelen);
>                STADJUST(savelen, out);
> +               free(mstr);

You can't just call ckmalloc because that memory will be leaked
if there is a longjmp (such as sh_error) before you free it.

Cheers,

Patch

diff --git a/src/parser.c b/src/parser.c
index a552c47..3f7e50a 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -898,6 +898,7 @@  readtoken1(int firstc, char const *syntax, char *eofmark, int striptabs)
 	struct nodelist *bqlist;
 	int quotef;
 	int oldstyle;
+	char *parsebackq_save;
 	/* syntax stack */
 	struct synstack synbase = { .syntax = syntax };
 	struct synstack *synstack = &synbase;
@@ -906,6 +907,7 @@  readtoken1(int firstc, char const *syntax, char *eofmark, int striptabs)
 		synstack->dblquote = 1;
 	quotef = 0;
 	bqlist = NULL;
+	parsebackq_save = NULL;
 
 	STARTSTACKSTR(out);
 	loop: {	/* for each line, until end of word */
@@ -1355,15 +1357,18 @@  badsub:
 parsebackq: {
 	struct nodelist **nlpp;
 	union node *n;
-	char *str;
+	char *str, *mstr;
 	size_t savelen;
 	struct heredoc *saveheredoclist;
 	int uninitialized_var(saveprompt);
 
-	str = NULL;
+	str = mstr = NULL;
 	savelen = out - (char *)stackblock();
 	if (savelen > 0) {
-		str = alloca(savelen);
+		if (savelen > 1024)
+			str = mstr = ckmalloc(savelen);
+		else
+			str = parsebackq_save ?: (parsebackq_save = alloca(1024));
 		memcpy(str, stackblock(), savelen);
 	}
         if (oldstyle) {
@@ -1449,6 +1454,7 @@  done:
 	if (str) {
 		memcpy(out, str, savelen);
 		STADJUST(savelen, out);
+		free(mstr);
 	}
 	USTPUTC(CTLBACKQ, out);
 	if (oldstyle)