Re: Probabilistic crash on zsh 5.9 on x86_64

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

> On 13/04/2023 17:55 Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx> wrote:
> > On 13/04/2023 17:40 Jun. T <takimoto-j@xxxxxxxxxxxxxxxxx> wrote:
> > Even with this, shfunc is still freed, although shfunc->funcdef is not freed.
> > But doshfunc() continues to use shfunc, at lines 5854, 5954, 5957, etc.
> > If it doesn't crash, it means the freed memory is not yet used by others.
> > But I think this is unsafe, and valgrind gives lots of warnings.
> 
> OK, so the starttrapscope() just above that point is pulling the rug out
> from under the function's feet.  We've looked up the TRAPEXIT function and
> now that function's just been undefined.
> 
> So some better save / restore for the shell function is probably needed.

I can't think of any major surgery I'd like to do for this special case
--- TRAPEXIT being from removed from within an execution of TRAPEXIT so
that the trap doesn't go off inside the function being executed.

Instead, here's a simple local fix-up which seems to keep valgrind quiet.
It's not the most elegant thing I can imagine.

diff --git a/Src/exec.c b/Src/exec.c
index 3b3d1235e..274800b10 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5779,12 +5779,25 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
     char *name = shfunc->node.nam;
     int flags = shfunc->node.flags;
     char *fname = dupstring(name);
-    Eprog prog;
+    Eprog prog, marked_prog;
     static int oflags;
     static int funcdepth;
     Heap funcheap;
 
     queue_signals();	/* Lots of memory and global state changes coming */
+    /*
+     * In case this is a special function such as a trap, mark it
+     * is in use right now, so it doesn't get freed early.  The
+     * worst that can happen is this hangs around in memory a little
+     * longer than strictly needed.
+     *
+     * Classic example of this happening is running TRAPEXIT directly.
+     *
+     * Because the shell function's contents may change, we'll ensure
+     * we use a consistent structure for use / free.
+     */
+    marked_prog = shfunc->funcdef;
+    useeprog(marked_prog);
 
     NEWHEAPS(funcheap) {
 	/*
@@ -5818,6 +5831,22 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	    memcpy(funcsave->pipestats, pipestats, bytes);
 	}
 
+	if (!strcmp(fname, "TRAPEXIT")) {
+	    /*
+	     * If we are executing TRAPEXIT directly, starttrapscope()
+	     * will pull the rug out from under us to ensure the
+	     * exit trap isn't run inside the function.  We just need
+	     * the information locally here, so copy it on the heap.
+	     *
+	     * The funcdef is separately handled by reference counting.
+	     */
+	    Shfunc shcopy = (Shfunc)zhalloc(sizeof(struct shfunc));
+	    memcpy(shcopy, shfunc, sizeof(struct shfunc));
+	    shcopy->node.nam = dupstring(shfunc->node.nam);
+	    shfunc = shcopy;
+	    name = shfunc->node.nam;
+	}
+
 	starttrapscope();
 	startpatternscope();
 
@@ -5942,6 +5971,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	funcsave->fstack.filename = getshfuncfile(shfunc);
 
 	prog = shfunc->funcdef;
+	DPUTS1(!prog->nref, "function definition %s has zero reference count",
+	       (fname && *fname) ? fname : "<anon>");
 	if (prog->flags & EF_RUN) {
 	    Shfunc shf;
 
@@ -6046,6 +6077,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	}
     } OLDHEAPS;
 
+    freeeprog(marked_prog);
     unqueue_signals();
 
     /*
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index e0b6afb5f..de57765a0 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -1083,6 +1083,17 @@ F:Must be tested with a top-level script rather than source or function
 >trap1
 # As of 5.7.1-test-2, the output was "out1 fn1 trap1 fn2" (on separate lines).
 
+  TRAPEXIT() { echo This is TRAPEXIT; }
+  TRAPEXIT
+  TRAPEXIT
+  TRAPEXIT
+0:No memory problems with explicit call to TRAPEXIT.
+>This is TRAPEXIT
+>This is TRAPEXIT
+>This is TRAPEXIT
+>This is TRAPEXIT
+# Three explicit calls, one implicit call at function exit.
+
 %clean
 
   rm -f TRAPEXIT