Crash when completion script call itself.

Crash when completion script call itself.

[Nicolas Desprès]

[-- 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

Re: Crash when completion script call itself.

[Peter Stephenson]

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

Re: Crash when completion script call itself.

[Nicolas Desprès]

[-- 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

Re: Crash when completion script call itself.

[Peter Stephenson]

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

Re: Crash when completion script call itself.

[Peter Stephenson]

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

Re: Crash when completion script call itself.

[Nicolas Desprès]

[-- 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

Re: Crash when completion script call itself.

[Peter Stephenson]

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

Re: Crash when completion script call itself.

[Kamil Dudka]

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

Re: Crash when completion script call itself.

[Peter Stephenson]

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

Re: Crash when completion script call itself.

[Bart Schaefer]

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.

Re: Crash when completion script call itself.

[Peter Stephenson]

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

Re: Crash when completion script call itself.

[Bart Schaefer]

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.

Re: Crash when completion script call itself.

[Peter Stephenson]

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

Re: Crash when completion script call itself.

[Kamil Dudka]

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

Re: Crash when completion script call itself.

[Mikael Magnusson]

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

Re: Crash when completion script call itself.

[Peter Stephenson]

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

Re: Crash when completion script call itself.

[Peter Stephenson]

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

Re: Crash when completion script call itself.

[Nicolas Desprès]

[-- 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

Re: Crash when completion script call itself.

[Nicolas Desprès]

[-- 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

Re: Crash when completion script call itself.

[Martijn Dekker]

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.