Re: PATCH: Re: localtraps causing core dump in 3.1.6+

[Peter Stephenson <pws@xxxxxxxxxxxxxxxxxxxxxxxx>]

"Bart Schaefer" wrote:
> On Apr 29,  4:42am, Bart Schaefer wrote:
> } Subject: Re: localtraps causing core dump in 3.1.6+
> }
> } } I recently started doing `setopt localtraps && unset -f TRAPZERR'
> } } because of some changes that took place for 3.1.7-pre-1.
> } } 
> } } However that led me to try the above, which dumps core
> } } on both 3.1.7-pre-1 and 3.1.6 when changing directory.
> 
> This is a bit ugly.

The following is less ugly, although a bit more inefficient, so I've backed
out Bart's noerr changes.

dosavetrap() now always copies the trap its saving.  In fact, we always
call it when we're unsetting a trap, so in most cases (this bug is the
exception) we don't need to do that.  However, it's much more logical and
fixes the bug neatly, since we are free to return the function node without
worrying where it's come from.  Since the changes in the code structure,
copying a chunk of shell code is now very much more efficient (thanks,
Sven).

I've merged it round Bart's changes, which were neater than mine in some
places, and added a minimal localtraps test.

It probably should be pointed out that `trap - ZERR' is a better thing to
use in general, since it's more standard and picks up both sorts of trap.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.9
diff -u -r1.9 builtin.c
--- Src/builtin.c	2000/04/29 06:33:48	1.9
+++ Src/builtin.c	2000/04/30 14:23:02
@@ -2592,14 +2592,12 @@
 
     /* Take arguments literally -- do not glob */
     for (; *argv; argv++) {
-	int ne = noerrs;
 	if ((hn = ht->removenode(ht, *argv))) {
 	    ht->freenode(hn);
 	} else {
 	    zwarnnam(name, "no such hash table element: %s", *argv, 0);
-	    returnval |= !noerrs;
+	    returnval = 1;
 	}
-	noerrs = ne;
     }
     return returnval;
 }
Index: Src/hashtable.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/hashtable.c,v
retrieving revision 1.4
diff -u -r1.4 hashtable.c
--- Src/hashtable.c	2000/04/29 06:33:48	1.4
+++ Src/hashtable.c	2000/04/30 14:23:04
@@ -787,8 +787,9 @@
 removeshfuncnode(HashTable ht, char *nam)
 {
     HashNode hn;
+    int signum;
 
-    if (!strncmp(nam, "TRAP", 4))
+    if (!strncmp(nam, "TRAP", 4) && (signum = getsignum(nam +4)) != -1)
 	hn = removetrap(getsignum(nam + 4));
     else
 	hn = removehashnode(shfunctab, nam);
Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.3
diff -u -r1.3 signals.c
--- Src/signals.c	2000/04/29 06:33:48	1.3
+++ Src/signals.c	2000/04/30 14:23:06
@@ -644,7 +644,8 @@
 static LinkList savetraps;
 
 /*
- * Save the current trap and unset it.
+ * Save the current trap by copying it.  This does nothing to
+ * the existing value of sigtrapped or sigfuncs.
  */
 
 static void
@@ -660,15 +661,18 @@
 	 * the new one yet.
 	 */
 	char func[20];
+	Shfunc shf, newshf = NULL;
 	sprintf(func, "TRAP%s", sigs[sig]);
-	/* We call removehashnode() directly because otherwise
-	 * removeshfuncnode() would be called which in turn would
-	 * call us again so that we would end up with a NULL pointer
-	 * instead of the list for the trap. */
-	st->list = removehashnode(shfunctab, func);
+	if ((shf = (Shfunc)shfunctab->getnode2(shfunctab, func))) {
+	    /* Copy the node for saving */
+	    newshf = (Shfunc) zalloc(sizeof(*newshf));
+	    newshf->nam = ztrdup(shf->nam);
+	    newshf->flags = shf->flags;
+	    newshf->funcdef = dupeprog(shf->funcdef, 0);
+	}
+	st->list = newshf;
     } else {
-	st->list = sigfuncs[sig];
-	sigfuncs[sig] = NULL;
+	st->list = sigfuncs[sig] ? dupeprog(sigfuncs[sig], 0) : NULL;
     }
     noerrs = !!st->list;
     if (!savetraps)
@@ -774,10 +778,11 @@
 
     /*
      * At this point we free the appropriate structs.  If we don't
-     * want that to happen (e.g. we are saving the trap), then
-     * either the function should already have been removed from shfunctab,
-     * or the entry in sigfuncs should have been set to NULL, and then
-     * we're laughing, in a sort of vague virtual sense.
+     * want that to happen then either the function should already have been
+     * removed from shfunctab, or the entry in sigfuncs 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.
      */
     if (trapped & ZSIG_FUNC) {
 	char func[20];
Index: Test/08traps.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/08traps.ztst,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 08traps.ztst
--- Test/08traps.ztst	2000/01/07 22:31:15	1.1.1.1
+++ Test/08traps.ztst	2000/04/30 14:23:06
@@ -126,3 +126,25 @@
   sleep 2
 0: Nested `trap ... TERM', triggered on outer loop
 >TERM1
+
+  TRAPZERR() { print error activated; }
+  fn() { print start of fn; false; print end of fn; }
+  fn
+  fn() {
+    setopt localoptions localtraps
+    unfunction TRAPZERR
+    print start of fn
+    false
+    print end of fn
+  }
+  fn
+  unfunction TRAPZERR
+  print finish
+0: basic localtraps handling
+>start of fn
+>error activated
+>end of fn
+>start of fn
+>end of fn
+>finish
+

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