zsh-workers
 help / color / mirror / code / Atom feed
* unbounded recursive call in a shell script crashes zsh
@ 2017-04-11 13:00 Kamil Dudka
  2017-04-11 13:29 ` Jérémie Roquet
  0 siblings, 1 reply; 17+ messages in thread
From: Kamil Dudka @ 2017-04-11 13:00 UTC (permalink / raw)
  To: zsh-workers

The following shell script crashes zsh (tested with zsh-5.3.1-90-g63f086d):

    function foo() {
        if true; then
            foo
        fi
    }

    foo

Is it expected?

I tried to interpret the script with zsh-4.3.11 and it did not crash:

    $ zsh /tmp/test.zsh
    foo:1: job table full or recursion limit exceeded

The script is a minimal example taken out from a powerline sourced script,
for which the crash was originally reported at:

    https://bugzilla.redhat.com/1441092

Kamil


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-11 13:00 unbounded recursive call in a shell script crashes zsh Kamil Dudka
@ 2017-04-11 13:29 ` Jérémie Roquet
  2017-04-11 14:01   ` Jérémie Roquet
  0 siblings, 1 reply; 17+ messages in thread
From: Jérémie Roquet @ 2017-04-11 13:29 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Zsh Hackers' List

2017-04-11 15:00 GMT+02:00 Kamil Dudka <kdudka@redhat.com>:
> The following shell script crashes zsh (tested with zsh-5.3.1-90-g63f086d):
>
>     function foo() {
>         if true; then
>             foo
>         fi
>     }
>
>     foo

That's interesting:

$ zsh -c 'foo() { foo }; foo'
foo: maximum nested function level reached

$ zsh -c 'foo() { if true; then foo; fi }; foo'
Segmentation fault

-- 
Jérémie


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-11 13:29 ` Jérémie Roquet
@ 2017-04-11 14:01   ` Jérémie Roquet
  2017-04-11 14:38     ` Kamil Dudka
  0 siblings, 1 reply; 17+ messages in thread
From: Jérémie Roquet @ 2017-04-11 14:01 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Zsh Hackers' List

2017-04-11 15:29 GMT+02:00 Jérémie Roquet <jroquet@arkanosis.net>:
> 2017-04-11 15:00 GMT+02:00 Kamil Dudka <kdudka@redhat.com>:
>> The following shell script crashes zsh (tested with zsh-5.3.1-90-g63f086d):
>>
>>     function foo() {
>>         if true; then
>>             foo
>>         fi
>>     }
>>
>>     foo
>
> That's interesting:
>
> $ zsh -c 'foo() { foo }; foo'
> foo: maximum nested function level reached
>
> $ zsh -c 'foo() { if true; then foo; fi }; foo'
> Segmentation fault

That looks like a stack overflow…

Doing either of the following fixed the issue for me:
 - recompile after “./configure --enable-max-function-depth=500”
(default is 1000);
 - execute after “ulimit -s 16384” (default for me was 8192).

Best regards,

-- 
Jérémie


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-11 14:01   ` Jérémie Roquet
@ 2017-04-11 14:38     ` Kamil Dudka
  2017-04-12  2:12       ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Kamil Dudka @ 2017-04-11 14:38 UTC (permalink / raw)
  To: Jérémie Roquet; +Cc: Zsh Hackers' List

On Tuesday, April 11, 2017 16:01:10 Jérémie Roquet wrote:
> 2017-04-11 15:29 GMT+02:00 Jérémie Roquet <jroquet@arkanosis.net>:
> > 2017-04-11 15:00 GMT+02:00 Kamil Dudka <kdudka@redhat.com>:
> >> The following shell script crashes zsh (tested with zsh-5.3.1-90-
g63f086d):
> >>     function foo() {
> >>     
> >>         if true; then
> >>         
> >>             foo
> >>         
> >>         fi
> >>     
> >>     }
> >>     
> >>     foo
> > 
> > That's interesting:
> > 
> > $ zsh -c 'foo() { foo }; foo'
> > foo: maximum nested function level reached
> > 
> > $ zsh -c 'foo() { if true; then foo; fi }; foo'
> > Segmentation fault
> 
> That looks like a stack overflow…
> 
> Doing either of the following fixed the issue for me:
>  - recompile after “./configure --enable-max-function-depth=500”
> (default is 1000);
>  - execute after “ulimit -s 16384” (default for me was 8192).

Thanks!  I can confirm that the error is handled properly after increasing
the limit for stack allocation.

So the difference is caused by the fact that recent versions of zsh are
more hungry on stack, so the stack allocation limit is exceeded before the 
shell call recursion limit is reached.

Kamil

> Best regards,


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-11 14:38     ` Kamil Dudka
@ 2017-04-12  2:12       ` Bart Schaefer
  2017-04-12  7:30         ` Kamil Dudka
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2017-04-12  2:12 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Zsh Hackers' List

On Apr 11,  4:38pm, Kamil Dudka wrote:
}
} So the difference is caused by the fact that recent versions of zsh are
} more hungry on stack, so the stack allocation limit is exceeded before the 
} shell call recursion limit is reached.

Actually more recent versions should be *less* hungry because we no
longer use alloca() as the default for many local arrays (unless your
zsh was compiled with --enable-stack-allocation).

However, a LOT has changed since 4.3.11.  For example the warning about
"recursion limit exceeded" is from an entirely different part of the
code than "maximum nested function" -- the limit that was being hit in
4.3.11 is never reached with 5.3.1, so there's no way to tell whether
the older version might also have overflowed the stack.

In fact 4.3.11 running out of job table space means that it consumed
all malloc() memory before consuming all of the stack; 5.3.1 is much
better about heap footprint.


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-12  2:12       ` Bart Schaefer
@ 2017-04-12  7:30         ` Kamil Dudka
  2017-04-12 22:11           ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Kamil Dudka @ 2017-04-12  7:30 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

On Tuesday, April 11, 2017 19:12:41 Bart Schaefer wrote:
> In fact 4.3.11 running out of job table space means that it consumed
> all malloc() memory before consuming all of the stack;

I cannot confirm your hypothesis.  In my testing environment, zsh-4.3.11 
raised the error when the condition (newsize > MAX_MAXJOBS) in expandjobtab() 
became true:

https://sourceforge.net/p/zsh/code/ci/zsh-4.3.11/tree/Src/jobs.c#l1735

At that time, the total amount of memory allocated by malloc() was 689120 
bytes (<1 MiB) on an x86_64 machine with 16 GiB of physical memory.

> 5.3.1 is much better about heap footprint.

Definitely an improvement but it does not solve the problem in question.

Kamil


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-12  7:30         ` Kamil Dudka
@ 2017-04-12 22:11           ` Bart Schaefer
  2017-04-13 14:30             ` Kamil Dudka
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2017-04-12 22:11 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: zsh-workers

On Wed, Apr 12, 2017 at 12:30 AM, Kamil Dudka <kdudka@redhat.com> wrote:
> On Tuesday, April 11, 2017 19:12:41 Bart Schaefer wrote:
>> In fact 4.3.11 running out of job table space means that it consumed
>> all malloc() memory before consuming all of the stack;
>
> I cannot confirm your hypothesis.

Sorry, I forgot about (and missed because I was scanning for diffs in
job handling and that part hasn't changed) MAX_MAXJOBS.  Either way it
means 4.3.11 ran out of job table space and never had a chance to
exercise the function call recursion limit.


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-12 22:11           ` Bart Schaefer
@ 2017-04-13 14:30             ` Kamil Dudka
  2017-04-13 15:21               ` Jérémie Roquet
  0 siblings, 1 reply; 17+ messages in thread
From: Kamil Dudka @ 2017-04-13 14:30 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On Wednesday, April 12, 2017 15:11:05 Bart Schaefer wrote:
> On Wed, Apr 12, 2017 at 12:30 AM, Kamil Dudka <kdudka@redhat.com> wrote:
> > On Tuesday, April 11, 2017 19:12:41 Bart Schaefer wrote:
> >> In fact 4.3.11 running out of job table space means that it consumed
> >> all malloc() memory before consuming all of the stack;
> > 
> > I cannot confirm your hypothesis.
> 
> Sorry, I forgot about (and missed because I was scanning for diffs in
> job handling and that part hasn't changed) MAX_MAXJOBS.  Either way it
> means 4.3.11 ran out of job table space and never had a chance to
> exercise the function call recursion limit.

Still the question remains:  Is it expected to run out of stack after <1000 
nested calls of a trivial shell function when 1000 is the default limit for
function call depth?

I was trying to reduce the stack usage of zsh but was not really successful, 
mainly because I do not know how to efficiently find the automatic variables 
that consumed the biggest portion of the stack.  The attached patch reduces 
the peak stack allocation on my example from 5.558 MB to 5.345 MB, so it is 
probably not worth applying at all.  Do you have any estimation about where 
else the stack allocation could be reduced?

Kamil

[-- Attachment #2: 0001-doshfunc-reduce-stack-allocation-in-favour-of-heap.patch --]
[-- Type: text/x-patch, Size: 3468 bytes --]

>From 758a8ee2b36f73766b1387974e0b3bf4bc87d1d8 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Thu, 13 Apr 2017 14:08:49 +0200
Subject: [PATCH] doshfunc: reduce stack allocation in favour of heap

---
 Src/exec.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/Src/exec.c b/Src/exec.c
index f021a08..cde549e 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5358,13 +5358,13 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
     char **pptab, **x, *oargv0;
     int oldzoptind, oldlastval, oldoptcind, oldnumpipestats, ret;
     int *oldpipestats = NULL;
-    char saveopts[OPT_SIZE], *oldscriptname = scriptname;
+    char *saveopts, *oldscriptname = scriptname;
     char *name = shfunc->node.nam;
     int flags = shfunc->node.flags, ooflags;
     char *fname = dupstring(name);
     int obreaks, ocontflag, oloops, saveemulation, restore_sticky;
     Eprog prog;
-    struct funcstack fstack;
+    struct funcstack *fstack;
     static int oflags;
     Emulation_options save_sticky = NULL;
 #ifdef MAX_FUNCTION_DEPTH
@@ -5409,6 +5409,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	/* We need to save the current options even if LOCALOPTIONS is *
 	 * not currently set.  That's because if it gets set in the    *
 	 * function we need to restore the original options on exit.   */
+	saveopts = (char *)zalloc(OPT_SIZE * sizeof(char));
 	memcpy(saveopts, opts, sizeof(opts));
 	saveemulation = emulation;
 	save_sticky = sticky;
@@ -5501,21 +5502,22 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 		goto undoshfunc;
 	    }
 #endif
-	fstack.name = dupstring(name);
+	fstack = zalloc(sizeof(struct funcstack));
+	fstack->name = dupstring(name);
 	/*
 	 * The caller is whatever is immediately before on the stack,
 	 * unless we're at the top, in which case it's the script
 	 * or interactive shell name.
 	 */
-	fstack.caller = funcstack ? funcstack->name :
+	fstack->caller = funcstack ? funcstack->name :
 	    dupstring(oargv0 ? oargv0 : argzero);
-	fstack.lineno = lineno;
-	fstack.prev = funcstack;
-	fstack.tp = FS_FUNC;
-	funcstack = &fstack;
+	fstack->lineno = lineno;
+	fstack->prev = funcstack;
+	fstack->tp = FS_FUNC;
+	funcstack = fstack;
 
-	fstack.flineno = shfunc->lineno;
-	fstack.filename = getshfuncfile(shfunc);
+	fstack->flineno = shfunc->lineno;
+	fstack->filename = getshfuncfile(shfunc);
 
 	prog = shfunc->funcdef;
 	if (prog->flags & EF_RUN) {
@@ -5523,7 +5525,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 
 	    prog->flags &= ~EF_RUN;
 
-	    runshfunc(prog, NULL, fstack.name);
+	    runshfunc(prog, NULL, fstack->name);
 
 	    if (!(shf = (Shfunc) shfunctab->getnode(shfunctab,
 						    (name = fname)))) {
@@ -5536,9 +5538,10 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	    }
 	    prog = shf->funcdef;
 	}
-	runshfunc(prog, wrappers, fstack.name);
+	runshfunc(prog, wrappers, fstack->name);
     doneshfunc:
-	funcstack = fstack.prev;
+	funcstack = fstack->prev;
+	zfree(fstack, sizeof(struct funcstack));
 #ifdef MAX_FUNCTION_DEPTH
     undoshfunc:
 	--funcdepth;
@@ -5586,6 +5589,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	    opts[WARNNESTEDVAR] = saveopts[WARNNESTEDVAR];
 	}
 
+	zfree(saveopts, OPT_SIZE * sizeof(char));
+
 	if (opts[LOCALLOOPS]) {
 	    if (contflag)
 		zwarn("`continue' active at end of function scope");
-- 
2.10.2


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-13 14:30             ` Kamil Dudka
@ 2017-04-13 15:21               ` Jérémie Roquet
  2017-04-13 16:01                 ` Jérémie Roquet
  0 siblings, 1 reply; 17+ messages in thread
From: Jérémie Roquet @ 2017-04-13 15:21 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Bart Schaefer, zsh-workers

2017-04-13 16:30 GMT+02:00 Kamil Dudka <kdudka@redhat.com>:
> I was trying to reduce the stack usage of zsh but was not really successful,
> mainly because I do not know how to efficiently find the automatic variables
> that consumed the biggest portion of the stack. […]  Do you have any
> estimation about where else the stack allocation could be reduced?

We get some useful information if we link without the “-s” flag (and
it can help to compile with “-O0 -ggdb” as well).

Then, using gdb's “backtrace” we get that the stack is being consumed
by recursion throught the following 13 frames:

#18596 0x0000000000464733 in execif (state=0x7fffffffc9f0, do_exec=0)
at loop.c:572
#18597 0x00000000004360e8 in execcmd_exec (state=0x7fffffffc9f0,
eparams=0x7fffffffc5e0, input=0, output=0, how=18, last1=2) at
exec.c:3705
#18598 0x00000000004307ea in execpline2 (state=0x7fffffffc9f0,
pcode=67, how=18, input=0, output=0, last1=0) at exec.c:1872
#18599 0x000000000042f505 in execpline (state=0x7fffffffc9f0,
slcode=13314, how=18, last1=0) at exec.c:1602
#18600 0x000000000042e859 in execlist (state=0x7fffffffc9f0,
dont_change_job=1, exiting=0) at exec.c:1360
#18601 0x000000000042df2f in execode (p=0x7019e0, dont_change_job=1,
exiting=0, context=0x4ba7a8 "shfunc") at exec.c:1141
#18602 0x000000000043ae92 in runshfunc (prog=0x7019e0, wrap=0x0,
name=0x7ffff7fe7028 "foo") at exec.c:5675
#18603 0x000000000043a763 in doshfunc (shfunc=0x701a70,
doshargs=0x7ffff7ff2568, noreturnval=0) at exec.c:5539
#18604 0x000000000043979f in execshfunc (shf=0x701a70,
args=0x7ffff7ff2568) at exec.c:5113
#18605 0x00000000004362c3 in execcmd_exec (state=0x7fffffffd830,
eparams=0x7fffffffd420, input=0, output=0, how=18, last1=1) at
exec.c:3757
#18606 0x00000000004307ea in execpline2 (state=0x7fffffffd830,
pcode=131, how=18, input=0, output=0, last1=1) at exec.c:1872
#18607 0x000000000042f505 in execpline (state=0x7fffffffd830,
slcode=3074, how=18, last1=1) at exec.c:1602
#18608 0x000000000042e859 in execlist (state=0x7fffffffd830,
dont_change_job=0, exiting=1) at exec.c:1360

And using gdb's “info frame” on each frame and looking at “frame at”
and “called by frame at”, we get:

execlist: 416 bytes
execpline: 464 bytes
execpline2: 208 bytes
execcmd_exec: 1056 bytes
execshfunc: 336 bytes
doshfunc: 736 bytes
runshfunc: 336 bytes
execode: 96 bytes
execlist: 416 bytes
execpline: 464 bytes
execpline2: 208 bytes
execcmd_exec: 1056 bytes
execif: 64 bytes

If we aggregate, it gives us:

execcmd_exec: 2112 bytes
execpline: 928 bytes
execlist: 832 bytes
doshfunc: 736 bytes
execpline2: 416 bytes
execshfunc: 336 bytes
runshfunc: 336 bytes
execode: 96 bytes
execif: 64 bytes

Hence a total of 5856 bytes per recursion, or 5719 kiB for 10000 recursions.

Best regards,

-- 
Jérémie


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-13 15:21               ` Jérémie Roquet
@ 2017-04-13 16:01                 ` Jérémie Roquet
  2017-04-15 16:14                   ` Bart Schaefer
  2017-04-18 13:54                   ` Kamil Dudka
  0 siblings, 2 replies; 17+ messages in thread
From: Jérémie Roquet @ 2017-04-13 16:01 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Bart Schaefer, zsh-workers

2017-04-13 17:21 GMT+02:00 Jérémie Roquet <jroquet@arkanosis.net>:
> Hence a total of 5856 bytes per recursion, or 5719 kiB for 10000 recursions.

Sorry, I meant 1000 recursions, obviously.

Here are the numbers when compiling using -O3 instead of -O0 -ggdb —
probably more useful for optimization:

execlist: 400 bytes
execpline: 416 bytes
execpline2: 224 bytes
execcmd_exec: 4864 bytes
execshfunc: 336 bytes
doshfunc: 704 bytes
runshfunc: 336 bytes
execode: 80 bytes
execlist: 400 bytes
execpline: 416 bytes
execpline2: 224 bytes
execcmd_exec: 4864 bytes
execif: 80 bytes

Aggregated:

execlist: 800 bytes
execpline: 832 bytes
execpline2: 448 bytes
execcmd_exec: 9728 bytes
execshfunc: 336 bytes
doshfunc: 704 bytes
runshfunc: 336 bytes
execode: 80 bytes
execif: 80 bytes

Hence an even higher total of 13344 bytes per recursion, or 13032 kiB
for 1000 recursions.

If I'm not mistaken, execcmd_exec seems to account for 73% of the stack usage.

-- 
Jérémie


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-13 16:01                 ` Jérémie Roquet
@ 2017-04-15 16:14                   ` Bart Schaefer
  2017-04-16 18:56                     ` Daniel Shahaf
  2017-04-18 13:54                   ` Kamil Dudka
  1 sibling, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2017-04-15 16:14 UTC (permalink / raw)
  To: zsh-workers; +Cc: Kamil Dudka

On Apr 13,  6:01pm, Jeremie Roquet wrote:
} Subject: Re: unbounded recursive call in a shell script crashes zsh
}
} 2017-04-13 17:21 GMT+02:00 Jeremie Roquet <jroquet@arkanosis.net>:
} > Hence a total of 5856 bytes per recursion, or 5719 kiB for 10000 recursions.
} 
} Sorry, I meant 1000 recursions, obviously.
} 
} Here are the numbers when compiling using -O3 instead of -O0 -ggdb -
} [...] 
} Hence an even higher total of 13344 bytes per recursion, or 13032 kiB
} for 1000 recursions.

Much of the zsh internals are written in a recursive-call coding style;
e.g. the parser is a modified recursive-descent algorithm, and as you
have just demonstrated the execution engine replays the resulting parse
tree by recusive traversal.  So zsh is going to consume stack, maybe
more of it than other shells.

The best setting of --enable-max-function-depth devolves somewhat to
black magic.  There's unfortunately no way for the builder of a pre-
compiled package such as an RPM to know what stack limit will be in
place when zsh starts up, the compiler options affect how much stack
is needed, there is no way for zsh to tell at run time that stack is
about to run out, and there is no useful way to recover once the stack
has been filled.

This has been debated several times in the past, there's probably a
way to find it by searching the archives.  At one point the max depth
was reduced from something like 4096 to the current default of 1000,
because too many of those out-of-stack crashes were happening.

If not for the variable thrown in by the compiler options it might be
possible for zsh to estimate a value for max function depth at the
time it starts up, by examining the current limits, but there's also
no guarantee that the limit settings reflect the actual amount of
stack available.

If someone has a brilliant solution for this, we'd be glad to use it.


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-15 16:14                   ` Bart Schaefer
@ 2017-04-16 18:56                     ` Daniel Shahaf
  2017-04-16 21:00                       ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Daniel Shahaf @ 2017-04-16 18:56 UTC (permalink / raw)
  To: zsh-workers

Bart Schaefer wrote on Sat, Apr 15, 2017 at 09:14:56 -0700:
> If not for the variable thrown in by the compiler options it might be
> possible for zsh to estimate a value for max function depth at the
> time it starts up, by examining the current limits,
>

Hmm.  Would it work to measure the stack space used, by declaring
a local variable in main(), stashing its address in a global write-once
variable, and then subtracting the address of a function-local variable
from the global?  That is:

    extern const void *top_of_stack;
    main() {
        const int x = …;
        top_of_stack = &x;
        ⋮
    }
    foo() {
        int x;
        printf("Stack space in use: %llu\n", (unsigned long long)(top_of_stack - &x));
    }

(I somewhat expect this to be a bad idea...)

> but there's also no guarantee that the limit settings reflect the
> actual amount of stack available.


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-16 18:56                     ` Daniel Shahaf
@ 2017-04-16 21:00                       ` Bart Schaefer
  2017-04-16 23:12                         ` Daniel Shahaf
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2017-04-16 21:00 UTC (permalink / raw)
  To: zsh-workers

On Apr 16,  6:56pm, Daniel Shahaf wrote:
}
} Hmm.  Would it work to measure the stack space used, by declaring
} a local variable in main() [...]?

Aside from it depending on a particular stack architecture which may
not be shared by all hardware, it doesn't solve the problem of knowing
how much stack space will be occupied by the average call frame.

And even if you could manage to divide the available space by the
average size, that's still not a guarantee you won't run out if a
number of frames are larger than average.


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-16 21:00                       ` Bart Schaefer
@ 2017-04-16 23:12                         ` Daniel Shahaf
  2017-04-17  0:17                           ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Daniel Shahaf @ 2017-04-16 23:12 UTC (permalink / raw)
  To: zsh-workers

Bart Schaefer wrote on Sun, Apr 16, 2017 at 14:00:16 -0700:
> On Apr 16,  6:56pm, Daniel Shahaf wrote:
> }
> } Hmm.  Would it work to measure the stack space used, by declaring
> } a local variable in main() [...]?
> 
> Aside from it depending on a particular stack architecture which may
> not be shared by all hardware, it doesn't solve the problem of knowing
> how much stack space will be occupied by the average call frame.
> 
> And even if you could manage to divide the available space by the
> average size, that's still not a guarantee you won't run out if a
> number of frames are larger than average.

I originally thought of average inter-frame step size too, but then
I realized that the code could compare how far it is from main() to
RLIMIT_STACK and bail out gracefully if it has, say, less than X KB or
Y% of stack remaining.

But as you say, that's unportable.  (Not undefined behaviour; just not
guaranteed to be correct.)  Also it really shouldn't be an
application-level concern...


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-16 23:12                         ` Daniel Shahaf
@ 2017-04-17  0:17                           ` Bart Schaefer
  0 siblings, 0 replies; 17+ messages in thread
From: Bart Schaefer @ 2017-04-17  0:17 UTC (permalink / raw)
  To: zsh-workers

On Apr 16, 11:12pm, Daniel Shahaf wrote:
}
} I realized that the code could compare how far it is from main() to
} RLIMIT_STACK and bail out gracefully if it has, say, less than X KB or
} Y% of stack remaining.

Well ...

Another approach would be to add a C function that does nothing more
than declare a large array to force a stack allocation, and assign
something to the last byte; then in doshfunc(), trap the appropriate
signals (SIGSEGV and SIGSTKFLT ?) while calling that function.  If a
signal is raised, bail out as if MAX_FUNCTION_DEPTH has been reached.

I suspect this would destroy some efficiency gains Sebastian has been
so diligently wringing out of the code the last few months, but it'd
probably be fairly bulletpoof.  doshfunc() could remember the depth it
last safely reached and skip calling the stack-probe until that was
next exceeded, so you'd only pay the penalty once.

But is all this really worth it?

I note in passing that MAX_FUNCTION_DEPTH can be switched off at compile
time, leaving the shell entirely at the mercy of ths script writer in
this regard.


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-13 16:01                 ` Jérémie Roquet
  2017-04-15 16:14                   ` Bart Schaefer
@ 2017-04-18 13:54                   ` Kamil Dudka
  2017-04-19 21:01                     ` Bart Schaefer
  1 sibling, 1 reply; 17+ messages in thread
From: Kamil Dudka @ 2017-04-18 13:54 UTC (permalink / raw)
  To: Jérémie Roquet; +Cc: zsh-workers, Bart Schaefer

[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]

On Thursday, April 13, 2017 18:01:13 Jérémie Roquet wrote:
> 2017-04-13 17:21 GMT+02:00 Jérémie Roquet <jroquet@arkanosis.net>:
> > Hence a total of 5856 bytes per recursion, or 5719 kiB for 10000
> > recursions.
> Sorry, I meant 1000 recursions, obviously.
> 
> Here are the numbers when compiling using -O3 instead of -O0 -ggdb —
> probably more useful for optimization:
> 
> execlist: 400 bytes
> execpline: 416 bytes
> execpline2: 224 bytes
> execcmd_exec: 4864 bytes
> execshfunc: 336 bytes
> doshfunc: 704 bytes
> runshfunc: 336 bytes
> execode: 80 bytes
> execlist: 400 bytes
> execpline: 416 bytes
> execpline2: 224 bytes
> execcmd_exec: 4864 bytes
> execif: 80 bytes
> 
> Aggregated:
> 
> execlist: 800 bytes
> execpline: 832 bytes
> execpline2: 448 bytes
> execcmd_exec: 9728 bytes
> execshfunc: 336 bytes
> doshfunc: 704 bytes
> runshfunc: 336 bytes
> execode: 80 bytes
> execif: 80 bytes
> 
> Hence an even higher total of 13344 bytes per recursion, or 13032 kiB
> for 1000 recursions.
> 
> If I'm not mistaken, execcmd_exec seems to account for 73% of the stack
> usage.

Thanks for the analysis!  I tried to apply the attached patch on top of my 
previous patch but the total saving of stack allocation was only up to 10%, 
depending on the compiler flags.  So it is not worth the troubles.  What 
helped significantly to make the default shell call nesting limit reachable 
again was the -fconserve-stack option of GCC.

Kamil

[-- Attachment #2: 0001-execcmd_exec-reduce-stack-allocation-in-favour-of-he.patch --]
[-- Type: text/x-patch, Size: 2154 bytes --]

>From 98fb1642f4b809f4984390871b982cc37155d9ed Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Tue, 18 Apr 2017 15:03:55 +0200
Subject: [PATCH] execcmd_exec: reduce stack allocation in favour of heap

---
 Src/exec.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/Src/exec.c b/Src/exec.c
index cde549e..b28db61 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2654,9 +2654,9 @@ execcmd_exec(Estate state, Execcmd_params eparams,
     LinkList filelist = NULL;
     LinkNode node;
     Redir fn;
-    struct multio *mfds[10];
+    struct multio **mfds;
     char *text;
-    int save[10];
+    int *save;
     int fil, dfil, is_cursh, do_exec = 0, redir_err = 0, i;
     int nullexec = 0, magic_assign = 0, forked = 0;
     int is_shfunc = 0, is_builtin = 0, is_exec = 0, use_defpath = 0;
@@ -2689,11 +2689,6 @@ execcmd_exec(Estate state, Execcmd_params eparams,
      */
     use_cmdoutval = !args;
 
-    for (i = 0; i < 10; i++) {
-	save[i] = -2;
-	mfds[i] = NULL;
-    }
-
     /* If the command begins with `%', then assume it is a *
      * reference to a job in the job table.                */
     if ((type == WC_SIMPLE || type == WC_TYPESET) && args && nonempty(args) &&
@@ -3370,6 +3365,13 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	}
     }
 
+    save = zalloc(10 * sizeof(int));
+    mfds = zalloc(10 * sizeof(struct multio *));
+    for (i = 0; i < 10; i++) {
+	save[i] = -2;
+	mfds[i] = NULL;
+    }
+
     /* Add pipeline input/output to mnodes */
     if (input)
 	addfd(forked, save, mfds, 0, input, 0, NULL);
@@ -3616,6 +3618,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	if (mfds[i] && mfds[i]->ct >= 2)
 	    closemn(mfds, i, REDIR_CLOSE);
 
+    zfree(mfds, 10 * sizeof(struct multio *));
+
     if (nullexec) {
 	/*
 	 * If nullexec is 2, we have variables to add with the redirections
@@ -4003,6 +4007,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
     fixfds(save);
 
  done:
+    zfree(save, 10 * sizeof(int));
     if (isset(POSIXBUILTINS) &&
 	(cflags & (BINF_PSPECIAL|BINF_EXEC)) &&
 	!(orig_cflags & BINF_COMMAND)) {
-- 
2.10.2


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

* Re: unbounded recursive call in a shell script crashes zsh
  2017-04-18 13:54                   ` Kamil Dudka
@ 2017-04-19 21:01                     ` Bart Schaefer
  0 siblings, 0 replies; 17+ messages in thread
From: Bart Schaefer @ 2017-04-19 21:01 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: zsh-workers

On Apr 18,  3:54pm, Kamil Dudka wrote:
}
} What helped significantly to make the default shell call nesting limit
} reachable  again was the -fconserve-stack option of GCC.

If you're going to change the configure-time options to make the stack
compact, you could also/instead change them to reduce the nesting limit.

I suppose it depends on whether your primary goal is to make the call
depth consistent, or is to avoid the shell crashing.


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

end of thread, other threads:[~2017-04-19 21:00 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-11 13:00 unbounded recursive call in a shell script crashes zsh Kamil Dudka
2017-04-11 13:29 ` Jérémie Roquet
2017-04-11 14:01   ` Jérémie Roquet
2017-04-11 14:38     ` Kamil Dudka
2017-04-12  2:12       ` Bart Schaefer
2017-04-12  7:30         ` Kamil Dudka
2017-04-12 22:11           ` Bart Schaefer
2017-04-13 14:30             ` Kamil Dudka
2017-04-13 15:21               ` Jérémie Roquet
2017-04-13 16:01                 ` Jérémie Roquet
2017-04-15 16:14                   ` Bart Schaefer
2017-04-16 18:56                     ` Daniel Shahaf
2017-04-16 21:00                       ` Bart Schaefer
2017-04-16 23:12                         ` Daniel Shahaf
2017-04-17  0:17                           ` Bart Schaefer
2017-04-18 13:54                   ` Kamil Dudka
2017-04-19 21:01                     ` Bart Schaefer

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

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