[v2] eval: avoid leaking memory associated with redirections

Commit Message

Herbert Xu Dec. 14, 2018, 5:44 a.m. UTC

Ron Yorston <[e-mail]> wrote:
> The following constructs result in ever-increasing memory usage:
> 
>   while true; do { true; } </dev/null; done
>   while true; do ( true; ) </dev/null; done
> 
> For comparison, bash displays static memory usage in both cases.
> 
> This issue was reported for BusyBox ash which is derived from dash:
> 
>   https://bugs.busybox.net/show_bug.cgi?id=7748
> 
> Signed-off-by: Ron Yorston <[e-mail]>

Thanks for the patch.

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

---8<---
The following constructs result in ever-increasing memory usage:

  while true; do { true; } </dev/null; done
  while true; do ( true; ) </dev/null; done

For comparison, bash displays static memory usage in both cases.

This issue was reported for BusyBox ash which is derived from dash:

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

Signed-off-by: Ron Yorston <[e-mail]>

I have simplified evaltree so that it simply sets the stack mark
unconditionally.  This allows us to remove the stack marks in the
functions called by evaltree.

Signed-off-by: Herbert Xu <[e-mail]>

Comments

Ron Yorston Dec. 14, 2018, 9:29 a.m. UTC | #1

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

diff --git a/src/eval.c b/src/eval.c
index 546ee1b..715aa8b 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -200,8 +200,12 @@  evaltree(union node *n, int flags)
 {
 	int checkexit = 0;
 	int (*evalfn)(union node *, int);
+	struct stackmark smark;
 	unsigned isor;
 	int status = 0;
+
+	setstackmark(&smark);
+
 	if (n == NULL) {
 		TRACE(("evaltree(NULL) called\n"));
 		goto out;
@@ -317,6 +321,8 @@  exexit:
 		exraise(EXEXIT);
 	}
 
+	popstackmark(&smark);
+
 	return exitstatus;
 }
 
@@ -396,14 +402,12 @@  evalfor(union node *n, int flags)
 	struct arglist arglist;
 	union node *argp;
 	struct strlist *sp;
-	struct stackmark smark;
 	int status;
 
 	errlinno = lineno = n->nfor.linno;
 	if (funcline)
 		lineno -= funcline - 1;
 
-	setstackmark(&smark);
 	arglist.lastp = &arglist.list;
 	for (argp = n->nfor.args ; argp ; argp = argp->narg.next) {
 		expandarg(argp, &arglist, EXP_FULL | EXP_TILDE);
@@ -420,7 +424,6 @@  evalfor(union node *n, int flags)
 			break;
 	}
 	loopnest--;
-	popstackmark(&smark);
 
 	return status;
 }
@@ -433,14 +436,12 @@  evalcase(union node *n, int flags)
 	union node *cp;
 	union node *patp;
 	struct arglist arglist;
-	struct stackmark smark;
 	int status = 0;
 
 	errlinno = lineno = n->ncase.linno;
 	if (funcline)
 		lineno -= funcline - 1;
 
-	setstackmark(&smark);
 	arglist.lastp = &arglist.list;
 	expandarg(n->ncase.expr, &arglist, EXP_TILDE);
 	for (cp = n->ncase.cases ; cp && evalskip == 0 ; cp = cp->nclist.next) {
@@ -459,8 +460,6 @@  evalcase(union node *n, int flags)
 		}
 	}
 out:
-	popstackmark(&smark);
-
 	return status;
 }
 
@@ -717,7 +716,6 @@  evalcommand(union node *cmd, int flags)
 	struct localvar_list *localvar_stop;
 	struct parsefile *file_stop;
 	struct redirtab *redir_stop;
-	struct stackmark smark;
 	union node *argp;
 	struct arglist arglist;
 	struct arglist varlist;
@@ -746,7 +744,6 @@  evalcommand(union node *cmd, int flags)
 
 	/* First expand the arguments. */
 	TRACE(("evalcommand(0x%lx, %d) called\n", (long)cmd, flags));
-	setstackmark(&smark);
 	file_stop = parsefile;
 	back_exitstatus = 0;
 
@@ -925,7 +922,6 @@  out:
 		 * However I implemented that within libedit itself.
 		 */
 		setvar("_", lastarg, 0);
-	popstackmark(&smark);
 
 	return status;
 }