Re: Crash when completion script call itself.

[Peter Stephenson <p.stephenson@samsung.com>]

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