Re: [arnova@xxxxxxxxx: Bug#293364: Bug in ZSH (zshell) using Debian-testing (Sarge)]

[Peter Stephenson <pws@xxxxxxxxxxxxxxxxxxxxxxxx>]

Clint Adams wrote:
> ----- Forwarded message from Arno van Amersfoort <arnova@xxxxxxxxx> -----
> 
> Package: zsh
> Version: 4.2.3-1
> 
>  When I exit a zshell (for example when exiting from su) zshell segfaults 
>  when:
>  - /usr/share/zsh/functions/TRAPEXIT exists and is executable
>  - And the function is loaded (for example from /etc/zsh/zshrc with 
>  "autoload ${^/usr/share/zsh/functions}/*(.xN:t)" )
> 
>  I am using Debian GNU/Linux 3.2 (testing/sarge)
> 
> 
> 
> ----- End forwarded message -----

3 1/2 hours of my life later...

Eprogs need to be set specially for not-yet-autoloaded functions,
apparently.

I have also rewritten function-style traps so that the code to execute
always comes from the function table.  This wasn't the problem here, but
it makes me very much happier.  The duplication in trapfuncs was always
rather hairy in cases like this where the function entry could be
manipulated behind your back.  trapfuncs has been renamed traplists to
reflect the new use.

I discoverd that C03traps exited early, so the last tests weren't called
properly (and two were wrong, including the one that caused the shell to
exit).  Now they are all executed, including the one I've added.

I'm reasonably happy this ought to be safe enough to go onto
zsh-4_2-patches.  (Note weasel words.)

Note also this needs patch -p1 since I made it offline.  However, I'll
commit it as soon as the message appears.

diff -ru zsh.old/Src/builtin.c zsh/Src/builtin.c
--- zsh.old/Src/builtin.c	2005-01-15 19:00:20.000000000 +0000
+++ zsh/Src/builtin.c	2005-02-06 18:49:07.352658776 +0000
@@ -2617,14 +2617,12 @@
 	    shfunctab->addnode(shfunctab, ztrdup(*argv), shf);
 
 	    if (signum != -1) {
-		if (settrap(signum, shf->funcdef)) {
+		if (settrap(signum, NULL, ZSIG_FUNC)) {
 		    shfunctab->removenode(shfunctab, *argv);
 		    shfunctab->freenode((HashNode)shf);
 		    returnval = 1;
 		    ok = 0;
 		}
-		else
-		    sigtrapped[signum] |= ZSIG_FUNC;
 	    }
 
 	    if (ok && OPT_ISSET(ops,'X') &&
@@ -4967,10 +4965,10 @@
 		    shfunctab->printnode(hn, 0);
 		DPUTS(!hn, "BUG: I did not find any trap functions!");
 	    } else if (sigtrapped[sig]) {
-		if (!sigfuncs[sig])
+		if (!siglists[sig])
 		    printf("trap -- '' %s\n", sigs[sig]);
 		else {
-		    s = getpermtext(sigfuncs[sig], NULL);
+		    s = getpermtext(siglists[sig], NULL);
 		    printf("trap -- ");
 		    quotedzputs(s, stdout);
 		    printf(" %s\n", sigs[sig]);
@@ -5013,7 +5011,7 @@
 	    break;
 	}
 	t = dupeprog(prog, 0);
-	if (settrap(sig, t))
+	if (settrap(sig, t, 0))
 	    freeeprog(t);
     }
     return *argv != NULL;
diff -ru zsh.old/Src/exec.c zsh/Src/exec.c
--- zsh.old/Src/exec.c	2005-02-05 17:25:22.000000000 +0000
+++ zsh/Src/exec.c	2005-02-06 18:49:31.972915928 +0000
@@ -2650,8 +2650,8 @@
 		unsettrap(sig);
     if (!(monitor = isset(MONITOR))) {
 	if (how & Z_ASYNC) {
-	    settrap(SIGINT, NULL);
-	    settrap(SIGQUIT, NULL);
+	    settrap(SIGINT, NULL, 0);
+	    settrap(SIGQUIT, NULL, 0);
 	    if (isatty(0)) {
 		close(0);
 		if (open("/dev/null", O_RDWR | O_NOCTTY)) {
@@ -3340,13 +3340,12 @@
 	/* is this shell function a signal trap? */
 	if (!strncmp(s, "TRAP", 4) &&
 	    (signum = getsignum(s + 4)) != -1) {
-	    if (settrap(signum, shf->funcdef)) {
+	    if (settrap(signum, NULL, ZSIG_FUNC)) {
 		freeeprog(shf->funcdef);
 		zfree(shf, sizeof(*shf));
 		state->pc = end;
 		return 1;
 	    }
-	    sigtrapped[signum] |= ZSIG_FUNC;
 
 	    /*
 	     * Remove the old node explicitly in case it has
diff -ru zsh.old/Src/hashtable.c zsh/Src/hashtable.c
--- zsh.old/Src/hashtable.c	2004-06-08 22:24:19.000000000 +0100
+++ zsh/Src/hashtable.c	2005-02-06 18:49:53.961573144 +0000
@@ -819,7 +819,6 @@
     if (!strncmp(hn->nam, "TRAP", 4)) {
 	int signum = getsignum(hn->nam + 4);
 	sigtrapped[signum] &= ~ZSIG_FUNC;
-	sigfuncs[signum] = NULL;
 	unsettrap(signum);
     }
 }
@@ -838,8 +837,7 @@
     if (!strncmp(shf->nam, "TRAP", 4)) {
 	int signum = getsignum(shf->nam + 4);
 	if (signum != -1) {
-	    settrap(signum, shf->funcdef);
-	    sigtrapped[signum] |= ZSIG_FUNC;
+	    settrap(signum, NULL, ZSIG_FUNC);
 	}
     }
 }
diff -ru zsh.old/Src/Modules/parameter.c zsh/Src/Modules/parameter.c
--- zsh.old/Src/Modules/parameter.c	2005-01-22 00:38:28.000000000 +0000
+++ zsh/Src/Modules/parameter.c	2005-02-06 18:54:07.519026552 +0000
@@ -332,13 +332,12 @@
 
     if (!strncmp(name, "TRAP", 4) &&
 	(sn = getsignum(name + 4)) != -1) {
-	if (settrap(sn, shf->funcdef)) {
+	if (settrap(sn, NULL, ZSIG_FUNC)) {
 	    freeeprog(shf->funcdef);
 	    zfree(shf, sizeof(*shf));
 	    zsfree(val);
 	    return;
 	}
-	sigtrapped[sn] |= ZSIG_FUNC;
     }
     shfunctab->addnode(shfunctab, ztrdup(name), shf);
     zsfree(val);
diff -ru zsh.old/Src/Modules/zftp.c zsh/Src/Modules/zftp.c
--- zsh.old/Src/Modules/zftp.c	2004-12-12 20:19:48.000000000 +0000
+++ zsh/Src/Modules/zftp.c	2005-02-06 18:39:58.503096592 +0000
@@ -436,7 +436,8 @@
     } else
 	alarm(0);
     if (sigtrapped[SIGALRM] || interact) {
-	if (sigfuncs[SIGALRM] || !sigtrapped[SIGALRM])
+	if (siglists[SIGALRM] || !sigtrapped[SIGALRM] ||
+	    (sigtrapped[SIGALRM] & ZSIG_FUNC))
 	    install_handler(SIGALRM);
 	else
 	    signal_ignore(SIGALRM);
@@ -452,7 +453,7 @@
 zfunpipe()
 {
     if (sigtrapped[SIGPIPE]) {
-	if (sigfuncs[SIGPIPE])
+	if (siglists[SIGPIPE] || (sigtrapped[SIGPIPE] & ZSIG_FUNC))
 	    install_handler(SIGPIPE);
 	else
 	    signal_ignore(SIGPIPE);
diff -ru zsh.old/Src/parse.c zsh/Src/parse.c
--- zsh.old/Src/parse.c	2004-10-06 20:22:52.000000000 +0100
+++ zsh/Src/parse.c	2005-02-06 20:03:30.949088976 +0000
@@ -2105,6 +2105,16 @@
 	errflag = 1;
 }
 
+/*
+ * Duplicate a programme list, on the heap if heap is 1, else
+ * in permanent storage.
+ *
+ * Be careful in case p is the Eprog for a function which will
+ * later be autoloaded.  The shf element of the returned Eprog
+ * must be set appropriately by the caller.  (Normally we create
+ * the Eprog in this case by using mkautofn.)
+ */
+
 /**/
 mod_export Eprog
 dupeprog(Eprog p, int heap)
diff -ru zsh.old/Src/signals.c zsh/Src/signals.c
--- zsh.old/Src/signals.c	2005-01-15 19:00:22.000000000 +0000
+++ zsh/Src/signals.c	2005-02-06 19:57:31.150786632 +0000
@@ -36,10 +36,19 @@
 /**/
 mod_export int sigtrapped[VSIGCOUNT];
 
-/* trap functions for each signal */
+/*
+ * Trap programme lists for each signal.
+ *
+ * If (sigtrapped[sig] & ZSIG_FUNC) is set, this isn't used.
+ * The corresponding shell function is used instead.
+ *
+ * Otherwise, if sigtrapped[sig] is not zero, this is NULL when a signal
+ * is to be ignored, and if not NULL contains the programme list to be
+ * eval'd.
+ */
 
 /**/
-mod_export Eprog sigfuncs[VSIGCOUNT];
+mod_export Eprog siglists[VSIGCOUNT];
 
 /* Total count of trapped signals */
 
@@ -682,7 +691,7 @@
 
 /*
  * Save the current trap by copying it.  This does nothing to
- * the existing value of sigtrapped or sigfuncs.
+ * the existing value of sigtrapped or siglists.
  */
 
 static void
@@ -704,15 +713,18 @@
 	    newshf->nam = ztrdup(shf->nam);
 	    newshf->flags = shf->flags;
 	    newshf->funcdef = dupeprog(shf->funcdef, 0);
+	    if (shf->flags & PM_UNDEFINED)
+		newshf->funcdef->shf = newshf;
 	}
 #ifdef DEBUG
 	else dputs("BUG: no function present with function trap flag set.");
 #endif
+	DPUTS(siglists[sig], "BUG: function signal has eval list, too.");
 	st->list = newshf;
     } else if (sigtrapped[sig]) {
-	st->list = sigfuncs[sig] ? dupeprog(sigfuncs[sig], 0) : NULL;
+	st->list = siglists[sig] ? dupeprog(siglists[sig], 0) : NULL;
     } else {
-	DPUTS(sigfuncs[sig], "BUG: sigfuncs not null for untrapped signal");
+	DPUTS(siglists[sig], "BUG: siglists not null for untrapped signal");
 	st->list = NULL;
     }
     if (!savetraps)
@@ -723,9 +735,24 @@
     zinsertlinknode(savetraps, (LinkNode)savetraps, st);
 }
 
+
+/*
+ * Set a trap:  note this does not handle manipulation of
+ * the function table for TRAPNAL functions.
+ *
+ * sig is the signal number.
+ *
+ * l is the list to be eval'd for a trap defined with the "trap"
+ * builtin and should be NULL for a function trap.
+ *
+ * flags includes any additional flags to be or'd into sigtrapped[sig],
+ * in particular ZSIG_FUNC; the basic flags will be assigned within
+ * settrap.
+ */
+
 /**/
 mod_export int
-settrap(int sig, Eprog l)
+settrap(int sig, Eprog l, int flags)
 {
     if (sig == -1)
         return 1;
@@ -741,8 +768,10 @@
     queue_signals();
     unsettrap(sig);
 
-    sigfuncs[sig] = l;
-    if (empty_eprog(l)) {
+    DPUTS((flags & ZSIG_FUNC) && l,
+	  "BUG: trap function has passed eval list, too");
+    siglists[sig] = l;
+    if (!(flags & ZSIG_FUNC) && empty_eprog(l)) {
 	sigtrapped[sig] = ZSIG_IGNORED;
         if (sig && sig <= SIGCOUNT &&
 #ifdef SIGWINCH
@@ -765,7 +794,7 @@
      * sigtrapped[sig] is zero or not, i.e. a test without a mask
      * works just the same.
      */
-    sigtrapped[sig] |= (locallevel << ZSIG_SHIFT);
+    sigtrapped[sig] |= (locallevel << ZSIG_SHIFT) | flags;
     unqueue_signals();
     return 0;
 }
@@ -829,7 +858,7 @@
     /*
      * At this point we free the appropriate structs.  If we don't
      * want that to happen then either the function should already have been
-     * removed from shfunctab, or the entry in sigfuncs should have been set
+     * removed from shfunctab, or the entry in siglists should have been set
      * to NULL.  This is no longer necessary for saving traps as that
      * copies the structures, so here we are remove the originals.
      * That causes a little inefficiency, but a good deal more reliability.
@@ -841,15 +870,14 @@
 	 * As in dosavetrap(), don't call removeshfuncnode() because
 	 * that calls back into unsettrap();
 	 */
-	sigfuncs[sig] = NULL;
 	if (node)
 	    removehashnode(shfunctab, node->nam);
 	unqueue_signals();
 
 	return node;
-    } else if (sigfuncs[sig]) {
-	freeeprog(sigfuncs[sig]);
-	sigfuncs[sig] = NULL;
+    } else if (siglists[sig]) {
+	freeeprog(siglists[sig]);
+	siglists[sig] = NULL;
     }
     unqueue_signals();
 
@@ -894,9 +922,9 @@
 	if (exittr & ZSIG_FUNC) {
 	    exitfn = removehashnode(shfunctab, "TRAPEXIT");
 	} else {
-	    exitfn = sigfuncs[SIGEXIT];
+	    exitfn = siglists[SIGEXIT];
+	    siglists[SIGEXIT] = NULL;
 	}
-	sigfuncs[SIGEXIT] = NULL;
 	if (sigtrapped[SIGEXIT] & ZSIG_TRAPPED)
 	    nsigtrapped--;
 	sigtrapped[SIGEXIT] = 0;
@@ -911,11 +939,12 @@
 	    remnode(savetraps, ln);
 
 	    if (st->flags && (st->list != NULL)) {
-		Eprog prog = (st->flags & ZSIG_FUNC) ?
-		    ((Shfunc) st->list)->funcdef : (Eprog) st->list;
 		/* prevent settrap from saving this */
 		dontsavetrap++;
-		settrap(sig, prog);
+		if (st->flags & ZSIG_FUNC)
+		    settrap(sig, NULL, ZSIG_FUNC);
+		else
+		    settrap(sig, (Eprog) st->list, 0);
 		dontsavetrap--;
 		/*
 		 * counting of nsigtrapped should presumably be handled
@@ -946,7 +975,7 @@
 }
 
 /* Execute a trap function for a given signal, possibly
- * with non-standard sigtrapped & sigfuncs values
+ * with non-standard sigtrapped & siglists values
  */
 
 /* Are we already executing a trap? */
@@ -1097,9 +1126,24 @@
 void
 dotrap(int sig)
 {
+    Eprog funcprog;
+
+    if (sigtrapped[sig] & ZSIG_FUNC) {
+	HashNode hn = gettrapnode(sig, 0);
+	if (hn)
+	    funcprog = ((Shfunc)hn)->funcdef;
+	else {
+#ifdef DEBUG
+	    dputs("BUG: running function trap which has escaped.");
+#endif
+	    funcprog = NULL;
+	}
+    } else
+	funcprog = siglists[sig];
+
     /* Copied from dotrapargs(). */
-    if ((sigtrapped[sig] & ZSIG_IGNORED) || !sigfuncs[sig] || errflag)
+    if ((sigtrapped[sig] & ZSIG_IGNORED) || !funcprog || errflag)
 	return;
 
-    dotrapargs(sig, sigtrapped+sig, sigfuncs[sig]);
+    dotrapargs(sig, sigtrapped+sig, funcprog);
 }
diff -ru zsh.old/Test/C03traps.ztst zsh/Test/C03traps.ztst
--- zsh.old/Test/C03traps.ztst	2004-07-27 21:59:33.000000000 +0100
+++ zsh/Test/C03traps.ztst	2005-02-06 20:00:45.012315200 +0000
@@ -183,17 +183,21 @@
   }
   testunset
   f
-1: more sophisticated error trapping
+  print status $?
+  unfunction TRAPZERR
+0: more sophisticated error trapping
 >f
 >ERR-or!
 >f
 >t
+>f
 >t
 >f
 >ERR-or!
 >testunset
 >f
 >ERR-or!
+>status 1
 
   f() {
     setopt localtraps
@@ -216,6 +220,32 @@
   fn
 1: ksh-style EXIT traps preserve return value
 
-  inner() { trap 'return 3' EXIT; return 2: }
+  inner() { trap 'return 3' EXIT; return 2; }
   outer() { inner; return 1; }
+  outer
 3: ksh-style EXIT traps can force return status of enclosing function
+
+# Autoloaded traps are horrid, but unfortunately people expect
+# them to work if we support them.
+  echo "print Running exit trap" >TRAPEXIT
+  $ZTST_testdir/../Src/zsh -fc '
+    fpath=(. $fpath)
+    autoload TRAPEXIT
+    print "Exiting, attempt 1"
+    exit
+    print "What?"
+  '
+  $ZTST_testdir/../Src/zsh -fc '
+    fpath=(. $fpath)
+    autoload TRAPEXIT;
+    fn() { print Some function }
+    fn
+    print "Exiting, attempt 2"
+    exit
+  '
+0: autoloaded TRAPEXIT (exit status > 128 indicates an old bug is back)
+>Exiting, attempt 1
+>Running exit trap
+>Some function
+>Exiting, attempt 2
+>Running exit trap

-- 
Peter Stephenson <pws@xxxxxxxxxxxxxxxxxxxxxxxx>
Work: pws@xxxxxxx
Web: http://www.pwstephenson.fsnet.co.uk