Re: Bug in Dash's unquoting of backslashes within backquoted strings

[Ron Yorston]

Ron Yorston wrote:
>This seems to have been introduced by commit 6bbc71d (parser: use
>pgetc_eatbnl() in more places).  Reverting the following part of the
>commit makes the problem go away:
>
>case '\\':
>-if ((pc = pgetc()) == '\n') {
>-   nlprompt();
>-   /*
>-* If eating a newline, avoid putting
>-* the newline into the new character
>-* stream (via the STPUTC after the
>-* switch).
>-*/
>-   continue;
>-   }
>+pc = pgetc_eatbnl();

Alternatively I see that BusyBox ash did this:

case '\\':
-   pc = pgetc();
-   if (pc == '\n') {
-   nlprompt();
-   /*
-* If eating a newline, avoid putting
-* the newline into the new character
-* stream (via the STPUTC after the
-* switch).
-*/
-   continue;
-   }
+   pc = pgetc(); /* or pgetc_eatbnl()? why (example)? */

Ron

Re: Bug in Dash's unquoting of backslashes within backquoted strings

[Ron Yorston]

Matt Whitlock wrote:
>A minimal example:
>
>: `: "
>\\$(bug)"`

>However, when it appears inside a backquoted subcommand (with the 
>backslash characters being appropriately escaped), such as given at the top 
>of this report, then Dash processes it incorrectly:
>
>/bin/sh: 1: bug: not found

This seems to have been introduced by commit 6bbc71d (parser: use
pgetc_eatbnl() in more places).  Reverting the following part of the
commit makes the problem go away:

case '\\':
-if ((pc = pgetc()) == '\n') {
-   nlprompt();
-   /*
-* If eating a newline, avoid putting
-* the newline into the new character
-* stream (via the STPUTC after the
-* switch).
-*/
-   continue;
-   }
+pc = pgetc_eatbnl();

Ron

Re: [v2 PATCH] eval: avoid leaking memory associated with redirections

[Ron Yorston]

Herbert Xu wrote:
>I thinkg we should just move the stack mark into evaltree for
>everybody, like this:

Thanks for looking at this.  I've tried the revised patch and it does
the job.

I've ported it to BusyBox ash where it's also effective and passes the
test suite.  Plus it saves 67 byte, which will occasion much rejoicing.

Ron

[PATCH] eval: avoid leaking memory associated with redirections

[Ron Yorston]

The following constructs result in ever-increasing memory usage:

   while true; do { true; } https://bugs.busybox.net/show_bug.cgi?id=7748

Signed-off-by: Ron Yorston 
---
 src/eval.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/eval.c b/src/eval.c
index 546ee1b..9f117ea 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -202,6 +202,7 @@ evaltree(union node *n, int flags)
int (*evalfn)(union node *, int);
unsigned isor;
int status = 0;
+   struct stackmark smark;
if (n == NULL) {
TRACE(("evaltree(NULL) called\n"));
goto out;
@@ -227,6 +228,7 @@ evaltree(union node *n, int flags)
status = !evaltree(n->nnot.com, EV_TESTED);
goto setstatus;
case NREDIR:
+   setstackmark();
errlinno = lineno = n->nredir.linno;
if (funcline)
lineno -= funcline - 1;
@@ -236,6 +238,7 @@ evaltree(union node *n, int flags)
 evaltree(n->nredir.n, flags & EV_TESTED);
if (n->nredir.redirect)
popredir(0);
+   popstackmark();
goto setstatus;
case NCMD:
 #ifdef notyet
@@ -476,11 +479,13 @@ evalsubshell(union node *n, int flags)
struct job *jp;
int backgnd = (n->type == NBACKGND);
int status;
+   struct stackmark smark;
 
errlinno = lineno = n->nredir.linno;
if (funcline)
lineno -= funcline - 1;
 
+   setstackmark();
expredir(n->nredir.redirect);
if (!backgnd && flags & EV_EXIT && !have_traps())
goto nofork;
@@ -500,6 +505,7 @@ nofork:
if (! backgnd)
status = waitforjob(jp);
INTON;
+   popstackmark();
return status;
 }
 
-- 
2.19.1

Re: [PATCH] parser: preserve characters on heap in backquote parsing

[Ron Yorston]

Herbert Xu wrote:
>Sorry, but this is not the only place where dash relies on alloca.
>So you're bound to run into this problem again if you have a script
>that attempts to push dash to use more than 8MB in places like this.

Sure, but the same problem existing in other places isn't a reason not
to fix it here.

>So I'm not going to accept this patch as alloca makes things a lot
>simpler in some cases.

Granted, I recall the simplification of the code that resulted from
the replacement of malloc by alloca in this case.  This patch retains
that simplicity.

Ron

Re: [PATCH] var: ensure variables are fully initialised when unset

[Ron Yorston]

Harald,

Thanks for pointing out the flaw in my reasoning.

I can confirm your alternative patch is effective in eradicating
the bug.

Cheers,

Ron

[PATCH] parser: preserve characters on heap in backquote parsing

[Ron Yorston]

This bug report for BusyBox ash also applies to dash:

   https://bugs.busybox.net/show_bug.cgi?id=9246

With an 8MB stack the test case results in a segfault.

Instead of using alloca() to preserve characters keep them on the
memalloc stack.  With this change the test case returns:

   $ dash test_case
   test_case: 3141: test_case: Syntax error: Unterminated quoted string

If the heap is reduced to the same size as the stack, 8MB:

   $ ulimit -S -d 8192
   $ dash test_case
   test_case: 0: test_case: Out of space

Signed-off-by: Ron Yorston 
---
 src/parser.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/parser.c b/src/parser.c
index c4e6378..6efa8e2 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1373,8 +1373,7 @@ parsebackq: {
str = NULL;
savelen = out - (char *)stackblock();
if (savelen > 0) {
-   str = alloca(savelen);
-   memcpy(str, stackblock(), savelen);
+   str = grabstackstr(out);
}
 if (oldstyle) {
 /* We must read until the closing backquote, giving special
-- 
2.19.1

Re: Command substitution in here-documents

[Ron Yorston]

I've been continuing to worry about this.

In the old code (prior to commit 7c245aa) the TNL token that was
detected in list() had actually been read.  The current code uses
peektoken().

When parseheredoc() is called in interactive mode the last call to
readtoken() is in the code to set the prompt.  Even though the text
of that token is long gone it's used anyway.  Exactly what then happens
on a given platform depends on how memory allocation is handled.

I can make the error go away by explicitly reading the TNL token when
peektoken() detects a newline and we're about to call parseheredoc().

Of course, I may be wrong.

Ron
---
 src/parser.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/parser.c b/src/parser.c
index c4e6378..b9bc0ca 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -170,6 +170,7 @@ list(int nlflag)
case TNL:
if (!(nlflag & 1))
break;
+   readtoken();
parseheredoc();
return n1;
 
-- 
2.17.1

Re: Command substitution in here-documents

[Ron Yorston]

Simon Ser wrote:
>On August 13, 2018 4:22 PM, Martijn Dekker  wrote:
>> -   The latest release is 0.5.10.2. I can't reproduce the bug at all in
>> 0.5.10, 0.5.10.1 or 0.5.10.2.
>
>This is interesting. I can reproduce with the latest commit in master.

I'm with Simon, the bug is present in current master for me.

This is on x86_64 Linux.  Which platform are you using Martijn?

Ron

Re: Command substitution in here-documents

[Ron Yorston]

Simon Ser wrote:
>I'm trying to use command substitution in here-documents. Here's my script:
>
>  cat <  `echo hi`
>  EOF
>
>This seems not to work in dash 0.5.9.1. It fails with this error:
>
>  dash: 1: > : not found

'git bisect' points to commit 7c245aa8ed33ba5db30eef9369d67036a05b0371
([PARSER] Simplify EOF/newline handling in list parser) as being at
fault.  This dates from October 2014 so the last good tagged release
is 0.5.8.  Reverting the commit makes the problem go away.

Some observations:

- The error only occurs when the commands are run from the command line;
  if run from within a script it works.  This is probably because...

- It appears that the prompt is being injected into the text of the
  command substitution.  If PS2 is an empty string the command works.

- Fixing this (other than using the blunt instrument of reverting the
  faulty commit) is beyond my pay grade.  Someone with a better
  understanding of the code will need to take a look.

Ron

Re: [PATCH] expand: remove unnecessary code in backquote expansion

[Ron Yorston]

Herbert Xu  wrote:
>Unfortunately we may need this at some point in the future due
>to changes in POSIX.  So let's keep it around for now until we
>get things such as `jobs -p` to work.

As you wish.

Something even more trivial I noticed later:  the TRACE at the end of
expbackq incorrectly refers to the function as evalbackq.

Ron
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] expand: remove unnecessary code in backquote expansion

[Ron Yorston]

ash originally had support for omitting the fork when expanding a
builtin in backquotes.  dash has gradually been removing this support,
most recently in commit 66b614e29038e31745c4a5d296f64f8d64f5c377
("[EVAL] Remove unused EV_BACKCMD flag").

Some traces still remain, however.  Remove:

- the buf and nleft elements of the backcmd structure;
- a misleading comment regarding handling of builtins.

Signed-off-by: Ron Yorston <r...@frippery.org>
---
 src/eval.c   | 12 
 src/eval.h   |  2 --
 src/expand.c | 31 ++-
 3 files changed, 14 insertions(+), 31 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 7498f9d..32fc300 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -605,10 +605,9 @@ evalpipe(union node *n, int flags)
 
 
 /*
- * Execute a command inside back quotes.  If it's a builtin command, we
- * want to save its output in a block obtained from malloc.  Otherwise
- * we fork off a subprocess and get the output of the command via a pipe.
- * Should be called with interrupts off.
+ * Execute a command inside back quotes.  We fork off a subprocess and
+ * get the output of the command via a pipe.  Should be called with
+ * interrupts off.
  */
 
 void
@@ -618,8 +617,6 @@ evalbackcmd(union node *n, struct backcmd *result)
struct job *jp;
 
result->fd = -1;
-   result->buf = NULL;
-   result->nleft = 0;
result->jp = NULL;
if (n == NULL) {
goto out;
@@ -644,8 +641,7 @@ evalbackcmd(union node *n, struct backcmd *result)
result->jp = jp;
 
 out:
-   TRACE(("evalbackcmd done: fd=%d buf=0x%x nleft=%d jp=0x%x\n",
-   result->fd, result->buf, result->nleft, result->jp));
+   TRACE(("evalbackcmd done: fd=%d jp=0x%x\n", result->fd, result->jp));
 }
 
 static char **
diff --git a/src/eval.h b/src/eval.h
index 63e7d86..92227af 100644
--- a/src/eval.h
+++ b/src/eval.h
@@ -42,8 +42,6 @@ extern int savestatus;/* exit status of last 
command outside traps */
 
 struct backcmd {   /* result of evalbackcmd */
int fd; /* file descriptor to read from */
-   char *buf;  /* buffer */
-   int nleft;  /* number of chars in buffer */
struct job *jp; /* job structure for command */
 };
 
diff --git a/src/expand.c b/src/expand.c
index 30288db..04a3eeb 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -510,7 +510,6 @@ expbackq(union node *cmd, int flag)
struct backcmd in;
int i;
char buf[128];
-   char *p;
char *dest;
int startloc;
char const *syntax = flag & EXP_QUOTED ? DQSYNTAX : BASESYNTAX;
@@ -522,27 +521,17 @@ expbackq(union node *cmd, int flag)
evalbackcmd(cmd, (struct backcmd *) );
popstackmark();
 
-   p = in.buf;
-   i = in.nleft;
-   if (i == 0)
-   goto read;
-   for (;;) {
-   memtodest(p, i, syntax, flag & QUOTES_ESC);
-read:
-   if (in.fd < 0)
-   break;
-   do {
-   i = read(in.fd, buf, sizeof buf);
-   } while (i < 0 && errno == EINTR);
-   TRACE(("expbackq: read returns %d\n", i));
-   if (i <= 0)
-   break;
-   p = buf;
-   }
-
-   if (in.buf)
-   ckfree(in.buf);
if (in.fd >= 0) {
+   for (;;) {
+   do {
+   i = read(in.fd, buf, sizeof buf);
+   } while (i < 0 && errno == EINTR);
+   TRACE(("expbackq: read returns %d\n", i));
+   if (i <= 0)
+   break;
+   memtodest(buf, i, syntax, flag & QUOTES_ESC);
+   }
+
close(in.fd);
back_exitstatus = waitforjob(in.jp);
}
-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html