* Crash when completion script call itself. @ 2017-09-29 7:25 ` Nicolas Desprès 2017-09-29 9:34 ` Peter Stephenson 0 siblings, 1 reply; 20+ messages in thread From: Nicolas Desprès @ 2017-09-29 7:25 UTC (permalink / raw) To: zsh-workers [-- Attachment #1: Type: text/plain, Size: 340 bytes --] Hi all, I have experienced a crash while debugging a completion script for a tool. Here how to reproduce: $ zsh --version zsh 5.4.2 (x86_64-apple-darwin16.7.0) $ cat >~/.zsh/Completion/_crash <<EOF #compdef crash _crash "$@" EOF $ exec zsh $ crash [TAB] zsh: segmentation fault zsh Cheers, -- Nicolas Desprès ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 7:25 ` Crash when completion script call itself Nicolas Desprès @ 2017-09-29 9:34 ` Peter Stephenson 2017-09-29 10:30 ` Nicolas Desprès 0 siblings, 1 reply; 20+ messages in thread From: Peter Stephenson @ 2017-09-29 9:34 UTC (permalink / raw) To: Nicolas Desprès, zsh-workers On Fri, 29 Sep 2017 20:25:44 +1300 Nicolas Desprès <nicolas.despres@gmail.com> wrote: > $ cat >~/.zsh/Completion/_crash <<EOF > > #compdef crash > _crash "$@" > EOF > $ exec zsh > $ crash [TAB] zsh: segmentation fault zsh This is certainly a real problem that people hit, but unfortuately there's not a lot we can do to make a general robust fix. There is already a check in the shell for the function recursion level; the default value is 1000 (it is configurable at build time). Some systems will get this far without crashing, as mine does, and I get _recurse: maximum nested function level reached which is what's supposed to happen. However, it's inevitable that systems vary in the resources they have available so a single hard and fast limit doesn't always work. I suspect that typically the crash happens when the user stack runs out (which we can't detect portably, indeed it's hard enough to detect at all as this is usually hidden from user-level code). It would be possible to store more function frame information in other forms of memory which are much less likely to run out. Unfortunately, while things like this could help they are just moving the problem around a bit and are quite hard to test widely enough to know if they're even making an improvement. pws ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 9:34 ` Peter Stephenson @ 2017-09-29 10:30 ` Nicolas Desprès 2017-09-29 10:40 ` Peter Stephenson 0 siblings, 1 reply; 20+ messages in thread From: Nicolas Desprès @ 2017-09-29 10:30 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh-workers [-- Attachment #1: Type: text/plain, Size: 370 bytes --] On Fri, Sep 29, 2017 at 10:34 PM, Peter Stephenson <p.stephenson@samsung.com > wrote: > > There is already a check in the shell for the function recursion level; > the default value is 1000 (it is configurable at build time). Some > systems will get this far without crashing, as mine does, and I get > I have not built zsh myself. Where can I check this value? -Nico ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 10:30 ` Nicolas Desprès @ 2017-09-29 10:40 ` Peter Stephenson 2017-09-29 10:45 ` Peter Stephenson 2017-09-29 11:37 ` Martijn Dekker 0 siblings, 2 replies; 20+ messages in thread From: Peter Stephenson @ 2017-09-29 10:40 UTC (permalink / raw) To: Nicolas Desprès, zsh-workers On Fri, 29 Sep 2017 23:30:56 +1300 Nicolas Desprès <nicolas.despres@gmail.com> wrote: > On Fri, Sep 29, 2017 at 10:34 PM, Peter Stephenson <p.stephenson@samsung.com > > wrote: > > > > There is already a check in the shell for the function recursion level; > > the default value is 1000 (it is configurable at build time). Some > > systems will get this far without crashing, as mine does, and I get > > > > I have not built zsh myself. Where can I check this value? If you don't have the build configuration there's no easy way. I have a strong suspicion none of the distributions is actually changing it anyway --- they have the same problem that it can run in a huge number of different environments. So very likely it's 1000. (I'm thinking of making it a shell variable in future --- this makes debugging and testing easier even if most users never touch it.) pws ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 10:40 ` Peter Stephenson @ 2017-09-29 10:45 ` Peter Stephenson 2017-09-29 11:03 ` Nicolas Desprès 2017-09-29 11:37 ` Martijn Dekker 1 sibling, 1 reply; 20+ messages in thread From: Peter Stephenson @ 2017-09-29 10:45 UTC (permalink / raw) To: Nicolas Desprès, zsh-workers On Fri, 29 Sep 2017 11:40:46 +0100 Peter Stephenson <p.stephenson@samsung.com> wrote: > On Fri, 29 Sep 2017 23:30:56 +1300 > Nicolas Desprès <nicolas.despres@gmail.com> wrote: > > On Fri, Sep 29, 2017 at 10:34 PM, Peter Stephenson <p.stephenson@samsung.com > > > wrote: > > > > > > There is already a check in the shell for the function recursion level; > > > the default value is 1000 (it is configurable at build time). Some > > > systems will get this far without crashing, as mine does, and I get > > > > > > > I have not built zsh myself. Where can I check this value? > > If you don't have the build configuration there's no easy way. ...come to think of it, slightly more helpfully, you can at least see how far it gets on your system... i=0; fn() { print $(( ++i )); fn; }; fn For me that prints 1000 and then the error, but in your case it might crash first (so start a new zsh before running). Note that's not actually quite the same as the completion case you were looking at --- completion has extra hooks so it is likely to be using more system resources at each recursion level. pws ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 10:45 ` Peter Stephenson @ 2017-09-29 11:03 ` Nicolas Desprès 2017-09-29 11:10 ` Peter Stephenson 0 siblings, 1 reply; 20+ messages in thread From: Nicolas Desprès @ 2017-09-29 11:03 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh-workers [-- Attachment #1: Type: text/plain, Size: 459 bytes --] On Fri, Sep 29, 2017 at 11:45 PM, Peter Stephenson <p.stephenson@samsung.com > wrote: > > ...come to think of it, slightly more helpfully, you can at least see > how far it gets on your system... > > i=0; fn() { print $(( ++i )); fn; }; fn > 767 and then crash. Maybe setting it to 500 would be enough? I think I remember that Python limit is 500 too. I have OSX Sierra 10.12.6 up to date. Zsh is the one installed by homebrew with no customization. -Nico ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 11:03 ` Nicolas Desprès @ 2017-09-29 11:10 ` Peter Stephenson 2017-09-29 11:27 ` Kamil Dudka 2017-09-29 11:38 ` Nicolas Desprès 0 siblings, 2 replies; 20+ messages in thread From: Peter Stephenson @ 2017-09-29 11:10 UTC (permalink / raw) To: Nicolas Desprès, zsh-workers On Sat, 30 Sep 2017 00:03:53 +1300 Nicolas Desprès <nicolas.despres@gmail.com> wrote: > On Fri, Sep 29, 2017 at 11:45 PM, Peter Stephenson <p.stephenson@samsung.com > > wrote: > > > > ...come to think of it, slightly more helpfully, you can at least see > > how far it gets on your system... > > > > i=0; fn() { print $(( ++i )); fn; }; fn > > > > 767 and then crash. Maybe setting it to 500 would be enough? I think I > remember that Python limit is 500 too. I've no personal objection to reducing it to 500 by default (and I'll probably add a variable anyway), but I'd like to know if anyone has strong feelings --- given this is a bit of a kludge anyway and this is based on a sample of one. pws ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 11:10 ` Peter Stephenson @ 2017-09-29 11:27 ` Kamil Dudka 2017-09-29 14:16 ` Peter Stephenson 2017-09-29 23:00 ` Nicolas Desprès 2017-09-29 11:38 ` Nicolas Desprès 1 sibling, 2 replies; 20+ messages in thread From: Kamil Dudka @ 2017-09-29 11:27 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh-workers, Nicolas Desprès On Friday, September 29, 2017 1:10:08 PM CEST Peter Stephenson wrote: > On Sat, 30 Sep 2017 00:03:53 +1300 > > Nicolas Desprès <nicolas.despres@gmail.com> wrote: > > On Fri, Sep 29, 2017 at 11:45 PM, Peter Stephenson > > <p.stephenson@samsung.com> > > > wrote: > > > > > > ...come to think of it, slightly more helpfully, you can at least see > > > how far it gets on your system... > > > > > > i=0; fn() { print $(( ++i )); fn; }; fn > > > > 767 and then crash. Maybe setting it to 500 would be enough? I think I > > remember that Python limit is 500 too. > > I've no personal objection to reducing it to 500 by default (and I'll > probably add a variable anyway), but I'd like to know if anyone has > strong feelings --- given this is a bit of a kludge anyway and this is > based on a sample of one. > > pws Recompiling zsh with the -fconserve-stack option of GCC made the default nesting limit 1000 reachable again on Fedora: https://src.fedoraproject.org/rpms/zsh/c/2524ac47 But decreasing the limit to 500 sounds reasonable... Kamil ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 11:27 ` Kamil Dudka @ 2017-09-29 14:16 ` Peter Stephenson 2017-09-29 15:22 ` Bart Schaefer 2017-10-02 15:40 ` Peter Stephenson 2017-09-29 23:00 ` Nicolas Desprès 1 sibling, 2 replies; 20+ messages in thread From: Peter Stephenson @ 2017-09-29 14:16 UTC (permalink / raw) To: zsh-workers This is the proposed change. I think the compromise of reducing the value but making it more easily configurable is probably a good one. It currently makes the new variable appear in all emulations, but I can change that. On Fri, 29 Sep 2017 13:27:58 +0200 Kamil Dudka <kdudka@redhat.com> wrote: > Recompiling zsh with the -fconserve-stack option of GCC made the default > nesting limit 1000 reachable again on Fedora: This is where it gets interesting owing to trade offs. People do care about function execution time --- not necessarily the same people that care as much about crashing if they introduce an accidental infinite recursion. I see this flag introduces a heuristic based on frame size, so tweaking memory management internally might have a similar but more controllable effect; on the other hand, it simultaneously removes any easy trade-off you can make directly using compiler flags, and I'm not keen on introducing #ifdef's where one branch would be underused and rot. Obviously we can document the gcc tweak but I don't think all that many people will notice. pws diff --git a/Doc/Zsh/params.yo b/Doc/Zsh/params.yo index 05ea45f..5757111 100644 --- a/Doc/Zsh/params.yo +++ b/Doc/Zsh/params.yo @@ -731,6 +731,16 @@ This value is system dependent and is intended for debugging purposes. It is also useful with the tt(zsh/system) module which allows the number to be turned into a name or message. ) +vindex(FUNCNEST) +item(tt(FUNCNEST) <S>)( +Integer. If greater than or equal to zero, the maximum nesting depth of +shell functions. When it is exceeded, an error is raised at the point +where a function is called. The default value is determined when +the shell is configured, but is typically 500. Increasing +the value increases the danger of a runaway function recursion +causing the shell to crash. Setting a negative value turns off +the check. +) vindex(GID) item(tt(GID) <S>)( The real group ID of the shell process. If you have sufficient privileges, diff --git a/README b/README index 7373306..6fad1d5 100644 --- a/README +++ b/README @@ -30,9 +30,33 @@ Zsh is a shell with lots of features. For a list of some of these, see the file FEATURES, and for the latest changes see NEWS. For more details, see the documentation. -Incompatibilities since 5.3.1 +Incompatibilities since 5.4.2 ----------------------------- +1) The default build-time maximum nested function depth has been +decreased from 1000 to 500 based on user experience. However, +it can now be changed at run time via the variable FUNCNEST. +If you previously configured the shell to set a different value, +or to remove the check, this is now reflected in the default +value of the variable. + +2) The syntax + +foo=([key]=value) + +can be used to set elements of arrays and associative arrays. In the +unlikely event that you need to set an array by matching files using a +pattern that starts with a character range followed by '=', you need to +quote the '=', e.g.: + +foo=([aeiou]\=vowel) + +This is only required for array values contained within parentheses; +command line expansion for normal arguments has not changed. + +Incompatibilities between 5.3.1 and 5.4.2 +----------------------------------------- + 1) The default behaviour of code like the following has changed: alias foo='noglob foo' diff --git a/Src/exec.c b/Src/exec.c index 0d2dc4e..2a95528 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -5505,9 +5505,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) struct funcstack fstack; static int oflags; Emulation_options save_sticky = NULL; -#ifdef MAX_FUNCTION_DEPTH static int funcdepth; -#endif Heap funcheap; queue_signals(); /* Lots of memory and global state changes coming */ @@ -5637,13 +5635,12 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) argzero = ztrdup(argzero); } } -#ifdef MAX_FUNCTION_DEPTH - if(++funcdepth > MAX_FUNCTION_DEPTH) - { - zerr("maximum nested function level reached"); - goto undoshfunc; - } -#endif + ++funcdepth; + if (zsh_funcnest >= 0 && funcdepth > zsh_funcnest) { + zerr("maximum nested function level reached"); + lastval = 1; + goto undoshfunc; + } fstack.name = dupstring(name); /* * The caller is whatever is immediately before on the stack, @@ -5682,10 +5679,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) runshfunc(prog, wrappers, fstack.name); doneshfunc: funcstack = fstack.prev; -#ifdef MAX_FUNCTION_DEPTH undoshfunc: --funcdepth; -#endif if (retflag) { retflag = 0; breaks = obreaks; diff --git a/Src/params.c b/Src/params.c index 3236f71..507d069 100644 --- a/Src/params.c +++ b/Src/params.c @@ -101,6 +101,19 @@ zlong lastval, /* $? */ rprompt_indent, /* $ZLE_RPROMPT_INDENT */ ppid, /* $PPID */ zsh_subshell; /* $ZSH_SUBSHELL */ + +/* $FUNCNEST */ +/**/ +mod_export +zlong zsh_funcnest = +#ifdef MAX_FUNCTION_DEPTH + MAX_FUNCTION_DEPTH +#else + /* Disabled by default but can be enabled at run time */ + -1 +#endif + ; + /**/ zlong lineno, /* $LINENO */ zoptind, /* $OPTIND */ @@ -337,6 +350,9 @@ IPDEF5("COLUMNS", &zterm_columns, zlevar_gsu), IPDEF5("LINES", &zterm_lines, zlevar_gsu), IPDEF5U("ZLE_RPROMPT_INDENT", &rprompt_indent, rprompt_indent_gsu), IPDEF5("SHLVL", &shlvl, varinteger_gsu), +#ifdef MAX_FUNCTION_DEPTH +IPDEF5("FUNCNEST", &zsh_funcnest, varinteger_gsu), +#endif /* Don't import internal integer status variables. */ #define IPDEF6(A,B,F) {{NULL,A,PM_INTEGER|PM_SPECIAL|PM_DONTIMPORT},BR((void *)B),GSU(F),10,0,NULL,NULL,NULL,0} diff --git a/Test/C04funcdef.ztst b/Test/C04funcdef.ztst index 6a675e0..90b77ab 100644 --- a/Test/C04funcdef.ztst +++ b/Test/C04funcdef.ztst @@ -515,6 +515,14 @@ 0:autoload with absolute path not cancelled by bare autoload >I have been loaded by explicit path. + ( + FUNCNEST=0 + fn() { true; } + fn + ) +1: +?fn:4: maximum nested function level reached + %clean rm -f file.in file.out diff --git a/configure.ac b/configure.ac index ec0bdae..1a498f8 100644 --- a/configure.ac +++ b/configure.ac @@ -415,13 +415,13 @@ ifdef([max_function_depth],[undefine([max_function_depth])])dnl AH_TEMPLATE([MAX_FUNCTION_DEPTH], [Define for function depth limits]) AC_ARG_ENABLE(max-function-depth, -AC_HELP_STRING([--enable-max-function-depth=MAX], [limit function depth to MAX, default 1000]), +AC_HELP_STRING([--enable-max-function-depth=MAX], [limit function depth to MAX, default 500]), [if test x$enableval = xyes; then - AC_DEFINE(MAX_FUNCTION_DEPTH, 1000) + AC_DEFINE(MAX_FUNCTION_DEPTH, 500) elif test x$enableval != xno; then AC_DEFINE_UNQUOTED(MAX_FUNCTION_DEPTH, $enableval) fi], -[AC_DEFINE(MAX_FUNCTION_DEPTH, 1000)] +[AC_DEFINE(MAX_FUNCTION_DEPTH, 500)] ) ifdef([default_readnullcmd],[undefine([default_readnullcmd])])dnl ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 14:16 ` Peter Stephenson @ 2017-09-29 15:22 ` Bart Schaefer 2017-09-29 15:36 ` Peter Stephenson 2017-10-02 15:40 ` Peter Stephenson 1 sibling, 1 reply; 20+ messages in thread From: Bart Schaefer @ 2017-09-29 15:22 UTC (permalink / raw) To: zsh-workers On Fri, Sep 29, 2017 at 7:16 AM, Peter Stephenson <p.stephenson@samsung.com> wrote: > This is the proposed change. I think the compromise of reducing the > value but making it more easily configurable is probably a good one. It > currently makes the new variable appear in all emulations, but I can > change that. Philosophically speaking, the value of 1000 for max depth was also based on user experience, and one might expect that most environments now have more stack space available than they did many years ago when 1000 was chosen. So reducing this value might make it more likely that people will encounter the "depth exceeded" error. At the least the error message should be updated to mention increasing FUNCNEST ? With respect to the #ifdefs, I think entirely removing FUNCNEST is a bad idea -- it should remain so that its value can be examined. We could go so far as to mark it readonly if preventing it from being reset is the intention. I guess that affects whether the shell keeps track of the current depth, too, but that doesn't seem like much overhead. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 15:22 ` Bart Schaefer @ 2017-09-29 15:36 ` Peter Stephenson 2017-09-29 17:48 ` Bart Schaefer 0 siblings, 1 reply; 20+ messages in thread From: Peter Stephenson @ 2017-09-29 15:36 UTC (permalink / raw) To: zsh-workers On Fri, 29 Sep 2017 08:22:16 -0700 Bart Schaefer <schaefer@brasslantern.com> wrote: > Philosophically speaking, the value of 1000 for max depth was also > based on user experience, and one might expect that most environments > now have more stack space available than they did many years ago when > 1000 was chosen. So reducing this value might make it more likely > that people will encounter the "depth exceeded" error. I think what's actually happened is we're saving and restoring rather more stuff. > At the least > the error message should be updated to mention increasing FUNCNEST ? I can certainly do that. > With respect to the #ifdefs, I think entirely removing FUNCNEST is a > bad idea -- it should remain so that its value can be examined. We > could go so far as to mark it readonly if preventing it from being > reset is the intention. I guess that affects whether the shell keeps > track of the current depth, too, but that doesn't seem like much > overhead. That was the intention but it escaped in one place --- the only remaining place should be where FUNCNEST gets initialised to -1 if the configuration parameter was undefined. pws ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 15:36 ` Peter Stephenson @ 2017-09-29 17:48 ` Bart Schaefer 0 siblings, 0 replies; 20+ messages in thread From: Bart Schaefer @ 2017-09-29 17:48 UTC (permalink / raw) To: zsh-workers On Sep 29, 4:36pm, Peter Stephenson wrote: } Subject: Re: Crash when completion script call itself. } } On Fri, 29 Sep 2017 08:22:16 -0700 } Bart Schaefer <schaefer@brasslantern.com> wrote: } > So reducing this value might make it more likely } > that people will encounter the "depth exceeded" error. } } I think what's actually happened is we're saving and restoring rather } more stuff. These are not mutually exclusive conditions. Note that we already switched from stack to zsh-heap for VARARR() as the default, back in July 2014. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 14:16 ` Peter Stephenson 2017-09-29 15:22 ` Bart Schaefer @ 2017-10-02 15:40 ` Peter Stephenson 2017-10-03 14:45 ` Kamil Dudka [not found] ` <9379.1507044225@thecus.kiddle.eu> 1 sibling, 2 replies; 20+ messages in thread From: Peter Stephenson @ 2017-10-02 15:40 UTC (permalink / raw) To: Zsh Hackers' List On Fri, 29 Sep 2017 15:16:14 +0100 Peter Stephenson <p.stephenson@samsung.com> wrote: > I see this flag introduces a heuristic based on frame size, so tweaking > memory management internally might have a similar but more controllable > effect; on the other hand, it simultaneously removes any easy trade-off > you can make directly using compiler flags, and I'm not keen on > introducing #ifdef's where one branch would be underused and rot. This looks like low-hanging fruit. Allocating memory to save over a shell function happens just at the point where we've created the new heap for the function, which expires immediately after the call. The allocation itself should take very little time and on most architectures accessing the structure should have a similar overhead to accessing the stack. I had to tweak the debug in parse.c to avoid a message if I increased FUNCSAVE to see where there was a crash. pws diff --git a/Src/exec.c b/Src/exec.c index dfb50c3..16f9f16 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -41,6 +41,20 @@ enum { ADDVAR_RESTORE = 1 << 2 }; +/* Structure in which to save values around shell function call */ + +struct funcsave { + char opts[OPT_SIZE]; + char *argv0; + int zoptind, lastval, optcind, numpipestats; + int *pipestats; + char *scriptname; + int breaks, contflag, loops, emulation, noerrexit, oflags, restore_sticky; + Emulation_options sticky; + struct funcstack fstack; +}; +typedef struct funcsave *Funcsave; + /* * used to suppress ERREXIT and trapping of SIGZERR, SIGEXIT. * Bits from noerrexit_bits. @@ -5495,34 +5509,31 @@ int sticky_emulation_differs(Emulation_options sticky2) mod_export int 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 **pptab, **x; + int ret; char *name = shfunc->node.nam; - int flags = shfunc->node.flags, ooflags; - int savnoerrexit; + int flags = shfunc->node.flags; char *fname = dupstring(name); - int obreaks, ocontflag, oloops, saveemulation, restore_sticky; Eprog prog; - struct funcstack fstack; static int oflags; - Emulation_options save_sticky = NULL; static int funcdepth; Heap funcheap; queue_signals(); /* Lots of memory and global state changes coming */ NEWHEAPS(funcheap) { - oargv0 = NULL; - obreaks = breaks; - ocontflag = contflag; - oloops = loops; + Funcsave funcsave = zhalloc(sizeof(struct funcsave)); + funcsave->scriptname = scriptname; + funcsave->argv0 = NULL; + funcsave->breaks = breaks; + funcsave->contflag = contflag; + funcsave->loops = loops; + funcsave->lastval = lastval; + funcsave->pipestats = NULL; + funcsave->numpipestats = numpipestats; + funcsave->noerrexit = noerrexit; if (trap_state == TRAP_STATE_PRIMED) trap_return--; - oldlastval = lastval; - oldnumpipestats = numpipestats; - savnoerrexit = noerrexit; /* * Suppression of ERR_RETURN is turned off in function scope. */ @@ -5533,8 +5544,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) * immediately by a pushheap/popheap pair. */ size_t bytes = sizeof(int)*numpipestats; - oldpipestats = (int *)zhalloc(bytes); - memcpy(oldpipestats, pipestats, bytes); + funcsave->pipestats = (int *)zhalloc(bytes); + memcpy(funcsave->pipestats, pipestats, bytes); } starttrapscope(); @@ -5543,8 +5554,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) pptab = pparams; if (!(flags & PM_UNDEFINED)) scriptname = dupstring(name); - oldzoptind = zoptind; - oldoptcind = optcind; + funcsave->zoptind = zoptind; + funcsave->optcind = optcind; if (!isset(POSIXBUILTINS)) { zoptind = 1; optcind = 0; @@ -5553,9 +5564,9 @@ 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. */ - memcpy(saveopts, opts, sizeof(opts)); - saveemulation = emulation; - save_sticky = sticky; + memcpy(funcsave->opts, opts, sizeof(opts)); + funcsave->emulation = emulation; + funcsave->sticky = sticky; if (sticky_emulation_differs(shfunc->sticky)) { /* @@ -5572,7 +5583,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) */ sticky = sticky_emulation_dup(shfunc->sticky, 1); emulation = sticky->emulation; - restore_sticky = 1; + funcsave->restore_sticky = 1; installemulation(emulation, opts); if (sticky->n_on_opts) { OptIndex *onptr; @@ -5591,7 +5602,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) /* All emulations start with pattern disables clear */ clearpatterndisables(); } else - restore_sticky = 0; + funcsave->restore_sticky = 0; if (flags & (PM_TAGGED|PM_TAGGED_LOCAL)) opts[XTRACE] = 1; @@ -5609,11 +5620,11 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) else opts[WARNNESTEDVAR] = 0; } - ooflags = oflags; + funcsave->oflags = oflags; /* * oflags is static, because we compare it on the next recursive - * call. Hence also we maintain ooflags for restoring the previous - * value of oflags after the call. + * call. Hence also we maintain a saved version for restoring + * the previous value of oflags after the call. */ oflags = flags; opts[PRINTEXITVALUE] = 0; @@ -5624,7 +5635,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) pparams = x = (char **) zshcalloc(((sizeof *x) * (1 + countlinknodes(doshargs)))); if (isset(FUNCTIONARGZERO)) { - oargv0 = argzero; + funcsave->argv0 = argzero; argzero = ztrdup(getdata(node)); } /* first node contains name regardless of option */ @@ -5634,7 +5645,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) } else { pparams = (char **) zshcalloc(sizeof *pparams); if (isset(FUNCTIONARGZERO)) { - oargv0 = argzero; + funcsave->argv0 = argzero; argzero = ztrdup(argzero); } } @@ -5644,21 +5655,21 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) lastval = 1; goto undoshfunc; } - fstack.name = dupstring(name); + funcsave->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 : - dupstring(oargv0 ? oargv0 : argzero); - fstack.lineno = lineno; - fstack.prev = funcstack; - fstack.tp = FS_FUNC; - funcstack = &fstack; + funcsave->fstack.caller = funcstack ? funcstack->name : + dupstring(funcsave->argv0 ? funcsave->argv0 : argzero); + funcsave->fstack.lineno = lineno; + funcsave->fstack.prev = funcstack; + funcsave->fstack.tp = FS_FUNC; + funcstack = &funcsave->fstack; - fstack.flineno = shfunc->lineno; - fstack.filename = getshfuncfile(shfunc); + funcsave->fstack.flineno = shfunc->lineno; + funcsave->fstack.filename = getshfuncfile(shfunc); prog = shfunc->funcdef; if (prog->flags & EF_RUN) { @@ -5666,7 +5677,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) prog->flags &= ~EF_RUN; - runshfunc(prog, NULL, fstack.name); + runshfunc(prog, NULL, funcsave->fstack.name); if (!(shf = (Shfunc) shfunctab->getnode(shfunctab, (name = fname)))) { @@ -5679,52 +5690,52 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) } prog = shf->funcdef; } - runshfunc(prog, wrappers, fstack.name); + runshfunc(prog, wrappers, funcsave->fstack.name); doneshfunc: - funcstack = fstack.prev; + funcstack = funcsave->fstack.prev; undoshfunc: --funcdepth; if (retflag) { retflag = 0; - breaks = obreaks; + breaks = funcsave->breaks; } freearray(pparams); - if (oargv0) { + if (funcsave->argv0) { zsfree(argzero); - argzero = oargv0; + argzero = funcsave->argv0; } pparams = pptab; if (!isset(POSIXBUILTINS)) { - zoptind = oldzoptind; - optcind = oldoptcind; + zoptind = funcsave->zoptind; + optcind = funcsave->optcind; } - scriptname = oldscriptname; - oflags = ooflags; + scriptname = funcsave->scriptname; + oflags = funcsave->oflags; endpatternscope(); /* before restoring old LOCALPATTERNS */ - if (restore_sticky) { + if (funcsave->restore_sticky) { /* * If we switched to an emulation environment just for * this function, we interpret the option and emulation * switch as being a firewall between environments. */ - memcpy(opts, saveopts, sizeof(opts)); - emulation = saveemulation; - sticky = save_sticky; + memcpy(opts, funcsave->opts, sizeof(opts)); + emulation = funcsave->emulation; + sticky = funcsave->sticky; } else if (isset(LOCALOPTIONS)) { /* restore all shell options except PRIVILEGED and RESTRICTED */ - saveopts[PRIVILEGED] = opts[PRIVILEGED]; - saveopts[RESTRICTED] = opts[RESTRICTED]; - memcpy(opts, saveopts, sizeof(opts)); - emulation = saveemulation; + funcsave->opts[PRIVILEGED] = opts[PRIVILEGED]; + funcsave->opts[RESTRICTED] = opts[RESTRICTED]; + memcpy(opts, funcsave->opts, sizeof(opts)); + emulation = funcsave->emulation; } else { /* just restore a couple. */ - opts[XTRACE] = saveopts[XTRACE]; - opts[PRINTEXITVALUE] = saveopts[PRINTEXITVALUE]; - opts[LOCALOPTIONS] = saveopts[LOCALOPTIONS]; - opts[LOCALLOOPS] = saveopts[LOCALLOOPS]; - opts[WARNNESTEDVAR] = saveopts[WARNNESTEDVAR]; + opts[XTRACE] = funcsave->opts[XTRACE]; + opts[PRINTEXITVALUE] = funcsave->opts[PRINTEXITVALUE]; + opts[LOCALOPTIONS] = funcsave->opts[LOCALOPTIONS]; + opts[LOCALLOOPS] = funcsave->opts[LOCALLOOPS]; + opts[WARNNESTEDVAR] = funcsave->opts[WARNNESTEDVAR]; } if (opts[LOCALLOOPS]) { @@ -5732,9 +5743,9 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) zwarn("`continue' active at end of function scope"); if (breaks) zwarn("`break' active at end of function scope"); - breaks = obreaks; - contflag = ocontflag; - loops = oloops; + breaks = funcsave->breaks; + contflag = funcsave->contflag; + loops = funcsave->loops; } endtrapscope(); @@ -5742,11 +5753,11 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) if (trap_state == TRAP_STATE_PRIMED) trap_return++; ret = lastval; - noerrexit = savnoerrexit; + noerrexit = funcsave->noerrexit; if (noreturnval) { - lastval = oldlastval; - numpipestats = oldnumpipestats; - memcpy(pipestats, oldpipestats, sizeof(int)*numpipestats); + lastval = funcsave->lastval; + numpipestats = funcsave->numpipestats; + memcpy(pipestats, funcsave->pipestats, sizeof(int)*numpipestats); } } OLDHEAPS; diff --git a/Src/parse.c b/Src/parse.c index 6e0856b..a6a3b09 100644 --- a/Src/parse.c +++ b/Src/parse.c @@ -2742,7 +2742,7 @@ freeeprog(Eprog p) DPUTS(p->nref < 0 && !(p->flags & EF_HEAP), "Real EPROG has nref < 0"); DPUTS(p->nref < -1, "Uninitialised EPROG nref"); #ifdef MAX_FUNCTION_DEPTH - DPUTS(p->nref > MAX_FUNCTION_DEPTH + 10, "Overlarge EPROG nref"); + DPUTS(p->nref > zsh_funcnest + 10, "Overlarge EPROG nref"); #endif if (p->nref > 0 && !--p->nref) { for (i = p->npats, pp = p->pats; i--; pp++) ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-10-02 15:40 ` Peter Stephenson @ 2017-10-03 14:45 ` Kamil Dudka 2017-10-03 17:55 ` Mikael Magnusson [not found] ` <9379.1507044225@thecus.kiddle.eu> 1 sibling, 1 reply; 20+ messages in thread From: Kamil Dudka @ 2017-10-03 14:45 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh-workers On Monday, October 2, 2017 5:40:18 PM CEST Peter Stephenson wrote: > On Fri, 29 Sep 2017 15:16:14 +0100 > > Peter Stephenson <p.stephenson@samsung.com> wrote: > > I see this flag introduces a heuristic based on frame size, so tweaking > > memory management internally might have a similar but more controllable > > effect; on the other hand, it simultaneously removes any easy trade-off > > you can make directly using compiler flags, and I'm not keen on > > introducing #ifdef's where one branch would be underused and rot. > > This looks like low-hanging fruit. Allocating memory to save over a > shell function happens just at the point where we've created the new > heap for the function, which expires immediately after the call. The > allocation itself should take very little time and on most architectures > accessing the structure should have a similar overhead to accessing the > stack. I was experimenting with a similar, slightly smaller, patch in April: http://www.zsh.org/mla/workers/2017/msg00630.html Now I tried to run the following command on my system: % FUNCNEST=4096 Src/zsh -c 'i=0; fn() { print $(( ++i )); fn; }; fn' With the current upstream HEAD, it counted to 1180. With my old patch, it counted to 1224. With your current patch, it counted to 1242. The difference becomes slightly more significant when zsh is compiled with -fconserve-stack. Then it counts to 2682 with the current version and 3024 with your patch applied. Kamil > I had to tweak the debug in parse.c to avoid a message if I increased > FUNCSAVE to see where there was a crash. > > pws > > diff --git a/Src/exec.c b/Src/exec.c > index dfb50c3..16f9f16 100644 > --- a/Src/exec.c > +++ b/Src/exec.c > @@ -41,6 +41,20 @@ enum { > ADDVAR_RESTORE = 1 << 2 > }; > > +/* Structure in which to save values around shell function call */ > + > +struct funcsave { > + char opts[OPT_SIZE]; > + char *argv0; > + int zoptind, lastval, optcind, numpipestats; > + int *pipestats; > + char *scriptname; > + int breaks, contflag, loops, emulation, noerrexit, oflags, > restore_sticky; + Emulation_options sticky; > + struct funcstack fstack; > +}; > +typedef struct funcsave *Funcsave; > + > /* > * used to suppress ERREXIT and trapping of SIGZERR, SIGEXIT. > * Bits from noerrexit_bits. > @@ -5495,34 +5509,31 @@ int sticky_emulation_differs(Emulation_options > sticky2) mod_export int > 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 **pptab, **x; > + int ret; > char *name = shfunc->node.nam; > - int flags = shfunc->node.flags, ooflags; > - int savnoerrexit; > + int flags = shfunc->node.flags; > char *fname = dupstring(name); > - int obreaks, ocontflag, oloops, saveemulation, restore_sticky; > Eprog prog; > - struct funcstack fstack; > static int oflags; > - Emulation_options save_sticky = NULL; > static int funcdepth; > Heap funcheap; > > queue_signals(); /* Lots of memory and global state changes coming */ > > NEWHEAPS(funcheap) { > - oargv0 = NULL; > - obreaks = breaks; > - ocontflag = contflag; > - oloops = loops; > + Funcsave funcsave = zhalloc(sizeof(struct funcsave)); > + funcsave->scriptname = scriptname; > + funcsave->argv0 = NULL; > + funcsave->breaks = breaks; > + funcsave->contflag = contflag; > + funcsave->loops = loops; > + funcsave->lastval = lastval; > + funcsave->pipestats = NULL; > + funcsave->numpipestats = numpipestats; > + funcsave->noerrexit = noerrexit; > if (trap_state == TRAP_STATE_PRIMED) > trap_return--; > - oldlastval = lastval; > - oldnumpipestats = numpipestats; > - savnoerrexit = noerrexit; > /* > * Suppression of ERR_RETURN is turned off in function scope. > */ > @@ -5533,8 +5544,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) * immediately by a pushheap/popheap pair. > */ > size_t bytes = sizeof(int)*numpipestats; > - oldpipestats = (int *)zhalloc(bytes); > - memcpy(oldpipestats, pipestats, bytes); > + funcsave->pipestats = (int *)zhalloc(bytes); > + memcpy(funcsave->pipestats, pipestats, bytes); > } > > starttrapscope(); > @@ -5543,8 +5554,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) pptab = pparams; > if (!(flags & PM_UNDEFINED)) > scriptname = dupstring(name); > - oldzoptind = zoptind; > - oldoptcind = optcind; > + funcsave->zoptind = zoptind; > + funcsave->optcind = optcind; > if (!isset(POSIXBUILTINS)) { > zoptind = 1; > optcind = 0; > @@ -5553,9 +5564,9 @@ 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. */ > - memcpy(saveopts, opts, sizeof(opts)); > - saveemulation = emulation; > - save_sticky = sticky; > + memcpy(funcsave->opts, opts, sizeof(opts)); > + funcsave->emulation = emulation; > + funcsave->sticky = sticky; > > if (sticky_emulation_differs(shfunc->sticky)) { > /* > @@ -5572,7 +5583,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) */ > sticky = sticky_emulation_dup(shfunc->sticky, 1); > emulation = sticky->emulation; > - restore_sticky = 1; > + funcsave->restore_sticky = 1; > installemulation(emulation, opts); > if (sticky->n_on_opts) { > OptIndex *onptr; > @@ -5591,7 +5602,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) /* All emulations start with pattern disables clear */ > clearpatterndisables(); > } else > - restore_sticky = 0; > + funcsave->restore_sticky = 0; > > if (flags & (PM_TAGGED|PM_TAGGED_LOCAL)) > opts[XTRACE] = 1; > @@ -5609,11 +5620,11 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) else > opts[WARNNESTEDVAR] = 0; > } > - ooflags = oflags; > + funcsave->oflags = oflags; > /* > * oflags is static, because we compare it on the next recursive > - * call. Hence also we maintain ooflags for restoring the previous > - * value of oflags after the call. > + * call. Hence also we maintain a saved version for restoring > + * the previous value of oflags after the call. > */ > oflags = flags; > opts[PRINTEXITVALUE] = 0; > @@ -5624,7 +5635,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) pparams = x = (char **) zshcalloc(((sizeof *x) * > (1 + countlinknodes(doshargs)))); > if (isset(FUNCTIONARGZERO)) { > - oargv0 = argzero; > + funcsave->argv0 = argzero; > argzero = ztrdup(getdata(node)); > } > /* first node contains name regardless of option */ > @@ -5634,7 +5645,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) } else { > pparams = (char **) zshcalloc(sizeof *pparams); > if (isset(FUNCTIONARGZERO)) { > - oargv0 = argzero; > + funcsave->argv0 = argzero; > argzero = ztrdup(argzero); > } > } > @@ -5644,21 +5655,21 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) lastval = 1; > goto undoshfunc; > } > - fstack.name = dupstring(name); > + funcsave->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 : > - dupstring(oargv0 ? oargv0 : argzero); > - fstack.lineno = lineno; > - fstack.prev = funcstack; > - fstack.tp = FS_FUNC; > - funcstack = &fstack; > + funcsave->fstack.caller = funcstack ? funcstack->name : > + dupstring(funcsave->argv0 ? funcsave->argv0 : argzero); > + funcsave->fstack.lineno = lineno; > + funcsave->fstack.prev = funcstack; > + funcsave->fstack.tp = FS_FUNC; > + funcstack = &funcsave->fstack; > > - fstack.flineno = shfunc->lineno; > - fstack.filename = getshfuncfile(shfunc); > + funcsave->fstack.flineno = shfunc->lineno; > + funcsave->fstack.filename = getshfuncfile(shfunc); > > prog = shfunc->funcdef; > if (prog->flags & EF_RUN) { > @@ -5666,7 +5677,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) > > prog->flags &= ~EF_RUN; > > - runshfunc(prog, NULL, fstack.name); > + runshfunc(prog, NULL, funcsave->fstack.name); > > if (!(shf = (Shfunc) shfunctab->getnode(shfunctab, > (name = fname)))) { > @@ -5679,52 +5690,52 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) } > prog = shf->funcdef; > } > - runshfunc(prog, wrappers, fstack.name); > + runshfunc(prog, wrappers, funcsave->fstack.name); > doneshfunc: > - funcstack = fstack.prev; > + funcstack = funcsave->fstack.prev; > undoshfunc: > --funcdepth; > if (retflag) { > retflag = 0; > - breaks = obreaks; > + breaks = funcsave->breaks; > } > freearray(pparams); > - if (oargv0) { > + if (funcsave->argv0) { > zsfree(argzero); > - argzero = oargv0; > + argzero = funcsave->argv0; > } > pparams = pptab; > if (!isset(POSIXBUILTINS)) { > - zoptind = oldzoptind; > - optcind = oldoptcind; > + zoptind = funcsave->zoptind; > + optcind = funcsave->optcind; > } > - scriptname = oldscriptname; > - oflags = ooflags; > + scriptname = funcsave->scriptname; > + oflags = funcsave->oflags; > > endpatternscope(); /* before restoring old LOCALPATTERNS */ > > - if (restore_sticky) { > + if (funcsave->restore_sticky) { > /* > * If we switched to an emulation environment just for > * this function, we interpret the option and emulation > * switch as being a firewall between environments. > */ > - memcpy(opts, saveopts, sizeof(opts)); > - emulation = saveemulation; > - sticky = save_sticky; > + memcpy(opts, funcsave->opts, sizeof(opts)); > + emulation = funcsave->emulation; > + sticky = funcsave->sticky; > } else if (isset(LOCALOPTIONS)) { > /* restore all shell options except PRIVILEGED and RESTRICTED */ > - saveopts[PRIVILEGED] = opts[PRIVILEGED]; > - saveopts[RESTRICTED] = opts[RESTRICTED]; > - memcpy(opts, saveopts, sizeof(opts)); > - emulation = saveemulation; > + funcsave->opts[PRIVILEGED] = opts[PRIVILEGED]; > + funcsave->opts[RESTRICTED] = opts[RESTRICTED]; > + memcpy(opts, funcsave->opts, sizeof(opts)); > + emulation = funcsave->emulation; > } else { > /* just restore a couple. */ > - opts[XTRACE] = saveopts[XTRACE]; > - opts[PRINTEXITVALUE] = saveopts[PRINTEXITVALUE]; > - opts[LOCALOPTIONS] = saveopts[LOCALOPTIONS]; > - opts[LOCALLOOPS] = saveopts[LOCALLOOPS]; > - opts[WARNNESTEDVAR] = saveopts[WARNNESTEDVAR]; > + opts[XTRACE] = funcsave->opts[XTRACE]; > + opts[PRINTEXITVALUE] = funcsave->opts[PRINTEXITVALUE]; > + opts[LOCALOPTIONS] = funcsave->opts[LOCALOPTIONS]; > + opts[LOCALLOOPS] = funcsave->opts[LOCALLOOPS]; > + opts[WARNNESTEDVAR] = funcsave->opts[WARNNESTEDVAR]; > } > > if (opts[LOCALLOOPS]) { > @@ -5732,9 +5743,9 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) zwarn("`continue' active at end of function scope"); > if (breaks) > zwarn("`break' active at end of function scope"); > - breaks = obreaks; > - contflag = ocontflag; > - loops = oloops; > + breaks = funcsave->breaks; > + contflag = funcsave->contflag; > + loops = funcsave->loops; > } > > endtrapscope(); > @@ -5742,11 +5753,11 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) if (trap_state == TRAP_STATE_PRIMED) > trap_return++; > ret = lastval; > - noerrexit = savnoerrexit; > + noerrexit = funcsave->noerrexit; > if (noreturnval) { > - lastval = oldlastval; > - numpipestats = oldnumpipestats; > - memcpy(pipestats, oldpipestats, sizeof(int)*numpipestats); > + lastval = funcsave->lastval; > + numpipestats = funcsave->numpipestats; > + memcpy(pipestats, funcsave->pipestats, sizeof(int)*numpipestats); > } > } OLDHEAPS; > > diff --git a/Src/parse.c b/Src/parse.c > index 6e0856b..a6a3b09 100644 > --- a/Src/parse.c > +++ b/Src/parse.c > @@ -2742,7 +2742,7 @@ freeeprog(Eprog p) > DPUTS(p->nref < 0 && !(p->flags & EF_HEAP), "Real EPROG has nref < 0"); > DPUTS(p->nref < -1, "Uninitialised EPROG nref"); > #ifdef MAX_FUNCTION_DEPTH > - DPUTS(p->nref > MAX_FUNCTION_DEPTH + 10, "Overlarge EPROG nref"); > + DPUTS(p->nref > zsh_funcnest + 10, "Overlarge EPROG nref"); > #endif > if (p->nref > 0 && !--p->nref) { > for (i = p->npats, pp = p->pats; i--; pp++) ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-10-03 14:45 ` Kamil Dudka @ 2017-10-03 17:55 ` Mikael Magnusson 2017-10-04 8:20 ` Peter Stephenson 0 siblings, 1 reply; 20+ messages in thread From: Mikael Magnusson @ 2017-10-03 17:55 UTC (permalink / raw) To: Kamil Dudka; +Cc: Peter Stephenson, zsh workers On Tue, Oct 3, 2017 at 4:45 PM, Kamil Dudka <kdudka@redhat.com> wrote: > On Monday, October 2, 2017 5:40:18 PM CEST Peter Stephenson wrote: >> On Fri, 29 Sep 2017 15:16:14 +0100 >> >> Peter Stephenson <p.stephenson@samsung.com> wrote: >> > I see this flag introduces a heuristic based on frame size, so tweaking >> > memory management internally might have a similar but more controllable >> > effect; on the other hand, it simultaneously removes any easy trade-off >> > you can make directly using compiler flags, and I'm not keen on >> > introducing #ifdef's where one branch would be underused and rot. >> >> This looks like low-hanging fruit. Allocating memory to save over a >> shell function happens just at the point where we've created the new >> heap for the function, which expires immediately after the call. The >> allocation itself should take very little time and on most architectures >> accessing the structure should have a similar overhead to accessing the >> stack. > > I was experimenting with a similar, slightly smaller, patch in April: > > http://www.zsh.org/mla/workers/2017/msg00630.html > > Now I tried to run the following command on my system: > > % FUNCNEST=4096 Src/zsh -c 'i=0; fn() { print $(( ++i )); fn; }; fn' > > With the current upstream HEAD, it counted to 1180. With my old patch, > it counted to 1224. With your current patch, it counted to 1242. > > The difference becomes slightly more significant when zsh is compiled > with -fconserve-stack. Then it counts to 2682 with the current version > and 3024 with your patch applied. If you want to do very deep nesting you can always increase the stack limit with ulimit -s (it's 8192kB on most distributions by default I believe). For example if I set it to 256 the above command only counts to 29, with ulimit -s unlimited I got this cool error with a higher FUNCNEST set: 16380 16381 16382 16383 16384 fn: maximum nested function level reached; increase FUNCNEST? parse.c:2745: Overlarge EPROG nref parse.c:2745: Overlarge EPROG nref parse.c:2745: Overlarge EPROG nref parse.c:2745: Overlarge EPROG nref but looking at the code that's just a debug code error, it only compares with the defined limit rather than the runtime limit. With a very high FUNCNEST it just keeps running until I run out of physical RAM, I had to ctrl-c at 92375. Does this look right? diff --git i/Src/parse.c w/Src/parse.c index 6e0856bbc1..ee1a1230ba 100644 --- i/Src/parse.c +++ w/Src/parse.c @@ -2742,7 +2742,7 @@ freeeprog(Eprog p) DPUTS(p->nref < 0 && !(p->flags & EF_HEAP), "Real EPROG has nref < 0"); DPUTS(p->nref < -1, "Uninitialised EPROG nref"); #ifdef MAX_FUNCTION_DEPTH - DPUTS(p->nref > MAX_FUNCTION_DEPTH + 10, "Overlarge EPROG nref"); + DPUTS(zsh_funcnest >= 0 && p->nref > zsh_funcnest + 10, "Overlarge EPROG nref"); #endif if (p->nref > 0 && !--p->nref) { for (i = p->npats, pp = p->pats; i--; pp++) -- Mikael Magnusson ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-10-03 17:55 ` Mikael Magnusson @ 2017-10-04 8:20 ` Peter Stephenson 0 siblings, 0 replies; 20+ messages in thread From: Peter Stephenson @ 2017-10-04 8:20 UTC (permalink / raw) To: zsh workers On Tue, 3 Oct 2017 19:55:07 +0200 Mikael Magnusson <mikachu@gmail.com> wrote: > but looking at the code that's just a debug code error, it only > compares with the defined limit rather than the runtime limit. I just committed my patch for this, though I've borrowed the extra sanity check from yours. pws ^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <9379.1507044225@thecus.kiddle.eu>]
* Re: Crash when completion script call itself. [not found] ` <9379.1507044225@thecus.kiddle.eu> @ 2017-10-03 15:50 ` Peter Stephenson 0 siblings, 0 replies; 20+ messages in thread From: Peter Stephenson @ 2017-10-03 15:50 UTC (permalink / raw) To: Zsh Hackers' List; +Cc: Oliver Kiddle On Tue, 03 Oct 2017 17:23:45 +0200 Oliver Kiddle <okiddle@yahoo.co.uk> wrote: > I still have issues with posting to the mailing list (tends to take > several resends) so I've not bothered... > > You wrote: > > This looks like low-hanging fruit. Allocating memory to save over a > > shell function happens just at the point where we've created the new > > heap for the function, which expires immediately after the call. The > > I'm not sure what you mean by "save over a shell function"? Do I > understand this as just using the heap instead of the stack for whatever > needs to be saved for each nested function? Is that so that we can > detect the eventual out-of-memory condition and can report it or to try > to maximise the possible number of recursions? Either way, it could do > with a comment to make the purpose clear. It's just to minimise stack usage. The fact the data is going somewhere other than the stack doesn't obviously need a comment, but I'll note the change in case someone sees odd effects and starts looking for a cause. If someone finds pathological behaviour with this, it's not a huge gain anyway so could be abandoned. > I'd have thought we could compress the data somewhat. In particular the > opts array must be around 200 bytes. That could be divided by eight with > bit fields or reduced near to nothing for functions that don't do setopt > localoptions. That's definitely going to impact on the time taken. If someone wants to start looking at real trade-offs of this sort they're welcome, but I'm not going anywhere near this. 200 bytes over 1000 functions is not huge by the standards of modern PC non-stack memory (I've taken off my embedded programming hat which is quite tightly fitting anyway...). > I also wonder if funcsave might be combined with funcstack > somehow: are both a stack of function calls? funcstack and funcsave are there for rather different purposes. funcstack is exposed to give a user-visible state; it's not necessarily constrained to grow or shrink within execshfunc(). funcsave is the dirty washing inside execshfunc() there's no obvious reason to expose. pws ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 11:27 ` Kamil Dudka 2017-09-29 14:16 ` Peter Stephenson @ 2017-09-29 23:00 ` Nicolas Desprès 1 sibling, 0 replies; 20+ messages in thread From: Nicolas Desprès @ 2017-09-29 23:00 UTC (permalink / raw) To: Kamil Dudka; +Cc: Peter Stephenson, zsh-workers [-- Attachment #1: Type: text/plain, Size: 277 bytes --] On Sat, Sep 30, 2017 at 12:27 AM, Kamil Dudka <kdudka@redhat.com> > > Recompiling zsh with the -fconserve-stack option of GCC made the default > nesting limit 1000 reachable again on Fedora: > I think the default compiler on OSX is clang. Maybe it is a compiler issue. -Nico ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 11:10 ` Peter Stephenson 2017-09-29 11:27 ` Kamil Dudka @ 2017-09-29 11:38 ` Nicolas Desprès 1 sibling, 0 replies; 20+ messages in thread From: Nicolas Desprès @ 2017-09-29 11:38 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh-workers [-- Attachment #1: Type: text/plain, Size: 349 bytes --] On Sat, Sep 30, 2017 at 12:10 AM, Peter Stephenson <p.stephenson@samsung.com > wrote: > > I've no personal objection to reducing it to 500 by default (and I'll > probably add a variable anyway), but I'd like to know if anyone has > strong feelings --- given this is a bit of a kludge anyway and this is > based on a sample of one. > Agreed. -Nico ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Crash when completion script call itself. 2017-09-29 10:40 ` Peter Stephenson 2017-09-29 10:45 ` Peter Stephenson @ 2017-09-29 11:37 ` Martijn Dekker 1 sibling, 0 replies; 20+ messages in thread From: Martijn Dekker @ 2017-09-29 11:37 UTC (permalink / raw) To: Zsh hackers list Op 29-09-17 om 12:40 schreef Peter Stephenson: [re: max shell function recursion level]> (I'm thinking of making it a shell variable in future --- this makes > debugging and testing easier even if most users never touch it.) FYI, on bash this variable is called FUNCNEST. If you choose to implement this, it might be good to use the same name. - M. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2017-10-04 8:20 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20170929072715epcas1p4171c28e9b82f94d79796ecca7e564ec3@epcas1p4.samsung.com> 2017-09-29 7:25 ` Crash when completion script call itself Nicolas Desprès 2017-09-29 9:34 ` Peter Stephenson 2017-09-29 10:30 ` Nicolas Desprès 2017-09-29 10:40 ` Peter Stephenson 2017-09-29 10:45 ` Peter Stephenson 2017-09-29 11:03 ` Nicolas Desprès 2017-09-29 11:10 ` Peter Stephenson 2017-09-29 11:27 ` Kamil Dudka 2017-09-29 14:16 ` Peter Stephenson 2017-09-29 15:22 ` Bart Schaefer 2017-09-29 15:36 ` Peter Stephenson 2017-09-29 17:48 ` Bart Schaefer 2017-10-02 15:40 ` Peter Stephenson 2017-10-03 14:45 ` Kamil Dudka 2017-10-03 17:55 ` Mikael Magnusson 2017-10-04 8:20 ` Peter Stephenson [not found] ` <9379.1507044225@thecus.kiddle.eu> 2017-10-03 15:50 ` Peter Stephenson 2017-09-29 23:00 ` Nicolas Desprès 2017-09-29 11:38 ` Nicolas Desprès 2017-09-29 11:37 ` Martijn Dekker
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).