dash: Fix stack overflow from infinite recursion in script
diff mbox series

Message ID 20190718153028.18791-1-andrew.shadura@collabora.co.uk
State Rejected
Delegated to: Herbert Xu
Headers show
Series
  • dash: Fix stack overflow from infinite recursion in script
Related show

Commit Message

Andrej Shadura July 18, 2019, 3:30 p.m. UTC
From: Chris Lamb <lamby@debian.org>

Bug-Debian: https://bugs.debian.org/579815
Signed-off-by: Chris Lamb <lamby@debian.org>
Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
---
 src/eval.c | 8 +++++++-
 src/eval.h | 2 ++
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments

Steffen Nurpmeso July 18, 2019, 5:34 p.m. UTC | #1
Andrej Shadura wrote in <20190718153028.18791-1-andrew.shadura@collabora\
.co.uk>:
 |From: Chris Lamb <lamby@debian.org>
 |
 |Bug-Debian: https://bugs.debian.org/579815
 |Signed-off-by: Chris Lamb <lamby@debian.org>
 |Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
  ...
 |+static int evalcount;                /* number of nested evalfun calls */
 ...
 |+             if (evalcount++ >= MAX_RECURSION)
 |+                     sh_error("Maximum function recursion depth (%d) \
 ...
 |+#define MAX_RECURSION 1000   /* maximum recursion level */

A couple of years ago i discovered mksh was crashing due to not
having such a limit, and in the discussion which started with
Thorsten (the mksh developer) it became obvious that a then-new
bash seemed to have stopped using a limit; Thorsten's point was
that the real limit can only be artitificial or very costly (if
i recall correctly).  By then i sent Mr. Xu a message in private
with i think a summary of all that, but never got any response.
I know that the Lua scripting language seems be in the process of
introducing such a limit with the upcoming release.

--steffen
|
|Der Kragenbaer,                The moon bear,
|der holt sich munter           he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)
Jason Bowen July 18, 2019, 6:27 p.m. UTC | #2
>  > +static int evalcount;                /* number of nested evalfun calls */
>  ...
>  > +             if (evalcount++ >= MAX_RECURSION)
>  > +                     sh_error("Maximum function recursion depth (%d) \
>  ...
>  > +#define MAX_RECURSION 1000   /* maximum recursion level */
>
> A couple of years ago i discovered mksh was crashing due to not
> having such a limit, and in the discussion which started with
> Thorsten (the mksh developer) it became obvious that a then-new
> bash seemed to have stopped using a limit; Thorsten's point was
> that the real limit can only be artitificial or very costly (if
> i recall correctly).  By then i sent Mr. Xu a message in private
> with i think a summary of all that, but never got any response.
> I know that the Lua scripting language seems be in the process of
> introducing such a limit with the upcoming release.

A recursion limit is not without precedent. CPython sets a platform-
dependent recursion limit for this same purpose:

https://docs.python.org/3/library/sys.html#sys.setrecursionlimit

But indeed, the limit is "artificial" or arbitrary. It should just be
"enough" that it isn't hit often with typical use.
Chris Lamb July 18, 2019, 6:35 p.m. UTC | #3
Hi all,

> >  > +             if (evalcount++ >= MAX_RECURSION)
> >  > +                     sh_error("Maximum function recursion depth (%d) \
> >  ...
> >  > +#define MAX_RECURSION 1000   /* maximum recursion level */
[…]
> A recursion limit is not without precedent. CPython sets a platform-
> dependent recursion limit for this same purpose:

CPython's dumb-yet-highly-effective limit was actually my source of
inspiration for this patch.


Regards,
Martijn Dekker July 18, 2019, 8:15 p.m. UTC | #4
Op 18-07-19 om 20:27 schreef Jason Bowen:
> A recursion limit is not without precedent. CPython sets a platform-
> dependent recursion limit for this same purpose:
> 
> https://docs.python.org/3/library/sys.html#sys.setrecursionlimit
> 
> But indeed, the limit is "artificial" or arbitrary. It should just be
> "enough" that it isn't hit often with typical use.

The arbitrariness of a hard-coded limit is indeed a problem. OK, 
crashing with a segfault is ugly because it makes the user suspect the 
shell is buggy, not the script. On the other hand, with a 'reasonable' 
limit, a script might still segfault on very small systems, while that 
limit disallows full use of system resources on large systems.

Regarding precedent: as far as I know, among POSIXy shells, a recursion 
limit is currently only implemented in zsh, with a user-settable 
FUNCNEST variable.

If such a feature were to be implemented, I think a configurable limit 
is preferable to a hard-coded limit, and IMHO zsh's precedent should be 
followed (including the name of the variable).

$ zsh -c 'f() { f; }; f'
f: maximum nested function level reached; increase FUNCNEST?

Of course, if you set FUNCNEST to a very large value, the stack will 
still overflow.

$ zsh -c 'FUNCNEST=999999999999999999; f() { f; }; f'
Segmentation fault: 11

Thanks,

- M.
Martijn Dekker July 18, 2019, 8:25 p.m. UTC | #5
Op 18-07-19 om 22:15 schreef Martijn Dekker:
> Regarding precedent: as far as I know, among POSIXy shells, a recursion 
> limit is currently only implemented in zsh, with a user-settable 
> FUNCNEST variable.

I was mistaken: both AT&T ksh93 and NetBSD 8.1 sh have a hard-coded 
limit of 1000.

- M.
Herbert Xu July 21, 2019, 3:10 p.m. UTC | #6
Martijn Dekker <martijn@inlv.org> wrote:
>
> I was mistaken: both AT&T ksh93 and NetBSD 8.1 sh have a hard-coded 
> limit of 1000.

Which still doesn't help you when you set the ulimit stack limit
to some ridiculously small value.

Such a limit in the presence of variable stack ulimits is just wrong.

Cheers,

Patch
diff mbox series

diff --git a/src/eval.c b/src/eval.c
index 1aad31a..ee36c39 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -69,6 +69,7 @@  int evalskip;			/* set if we are skipping commands */
 STATIC int skipcount;		/* number of levels to skip */
 MKINIT int loopnest;		/* current loop nesting level */
 static int funcline;		/* starting line number of current function, or 0 if not in a function */
+static int evalcount;		/* number of nested evalfun calls */
 
 
 char *commandname;
@@ -903,7 +904,12 @@  raise:
 		break;
 
 	case CMDFUNCTION:
-		if (evalfun(cmdentry.u.func, argc, argv, flags))
+		if (evalcount++ >= MAX_RECURSION)
+			sh_error("Maximum function recursion depth (%d) reached",
+			         MAX_RECURSION);
+		int i = evalfun(cmdentry.u.func, argc, argv, flags);
+		evalcount--;
+		if (i)
 			goto raise;
 		break;
 	}
diff --git a/src/eval.h b/src/eval.h
index 63e7d86..38dffbd 100644
--- a/src/eval.h
+++ b/src/eval.h
@@ -51,6 +51,8 @@  struct backcmd {		/* result of evalbackcmd */
 #define EV_EXIT 01		/* exit after evaluating tree */
 #define EV_TESTED 02		/* exit status is checked; ignore -e flag */
 
+#define MAX_RECURSION 1000	/* maximum recursion level */
+
 int evalstring(char *, int);
 union node;	/* BLETCH for ansi C */
 int evaltree(union node *, int);