PATCH: Re: localtraps causing core dump in 3.1.6+

PATCH: Re: localtraps causing core dump in 3.1.6+

[Bart Schaefer]

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.
} 
} There is a bug in unsetting TRAPxxx (as opposed to resetting them to
} something else) when localtraps is set.  You might try `trap - ZERR' or
} `disable -f TRAPZERR' instead for now.

This is a bit ugly.

Both unsettrap() and removeshfuncnode() want to call removenode().

removeshfuncnode() returns the hashnode, and a warning is generated if
it's NULL, so it has to get the node before unsettrap() removes it.
But unsettrap() subsequently calls freenode(), so it doesn't work to
grab the node with gethashnode2() before calling unsettrap().

The standard trick is `sigtrapped[sig] &= ~ZSIG_FUNC;' before calling
unsettrap(), but if that's done then dosavetrap() doesn't save the
shell function body and localtraps is foiled.

In the current state, dosavetrap() thinks there's a function and tries
to save the body, but it can't because removeshfuncnode() has already
removed it from the table.  Thus the core dump when endtrapscope() tries
to restore the nonexistent body.

If we let dosavetrap() do its work, then *that* removes the node to save
it, so again we can't pass it back up through removeshfuncnode() to stop
the error message.  Which is just as well, because if it did make it up
to bin_unhash(), it'd get freed, when the whole point is to save it.

Gaah.

If anyone can think of a better fix for this than diddling with `noerrs',
don't be shy.

By the way, the more I see of the "compiler-like error messages" from 8779,
the more I hate them.  Please let's back out that change, or at least put
the space back in after each colon.

Oh, yeah:  One side-effect of this patch is that `unfunction TRAPxxx' has
the same effect as `trap - xxx', which is symmetrical with `trap - xxx'
having the effect of `unfunction TRAPxxx'.  The "no such ..." warning is
suppressed when such a trap is removed, but that can be changed by moving
`noerrs = !!st->list;' up four lines to above `} else {'.

Index: Src/builtin.c
===================================================================
@@ -2592,12 +2592,14 @@
 
     /* 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 = 1;
+	    returnval |= !noerrs;
 	}
+	noerrs = ne;
     }
     return returnval;
 }
Index: Src/hashtable.c
===================================================================
@@ -788,12 +788,12 @@
 {
     HashNode hn;
 
-    if ((hn = removehashnode(shfunctab, nam))) {
-	if (!strncmp(hn->nam, "TRAP", 4))
-	    unsettrap(getsignum(hn->nam + 4));
-	return hn;
-    } else
-	return NULL;
+    if (!strncmp(nam, "TRAP", 4))
+	hn = removetrap(getsignum(nam + 4));
+    else
+	hn = removehashnode(shfunctab, nam);
+
+    return hn;
 }
 
 /* Disable an entry in the shell function hash table.    *
@@ -822,11 +822,10 @@
 enableshfuncnode(HashNode hn, int flags)
 {
     Shfunc shf = (Shfunc) hn;
-    int signum;
 
     shf->flags &= ~DISABLED;
     if (!strncmp(shf->nam, "TRAP", 4)) {
-	signum = getsignum(shf->nam + 4);
+	int signum = getsignum(shf->nam + 4);
 	if (signum != -1) {
 	    settrap(signum, shf->funcdef);
 	    sigtrapped[signum] |= ZSIG_FUNC;
Index: Src/signals.c
===================================================================
@@ -670,6 +670,7 @@
 	st->list = sigfuncs[sig];
 	sigfuncs[sig] = NULL;
     }
+    noerrs = !!st->list;
     if (!savetraps)
 	savetraps = znewlinklist();
     /*
@@ -726,11 +727,22 @@
 void
 unsettrap(int sig)
 {
+    int ne = noerrs;
+    HashNode hn = removetrap(sig);
+    noerrs = ne;
+    if (hn)
+	shfunctab->freenode(hn);
+}
+
+/**/
+HashNode
+removetrap(int sig)
+{
     int trapped;
 
     if (sig == -1 ||
 	(jobbing && (sig == SIGTTOU || sig == SIGTSTP || sig == SIGTTIN)))
-	return;
+	return NULL;
 
     trapped = sigtrapped[sig];
     /*
@@ -743,7 +755,7 @@
 	dosavetrap(sig, locallevel);
 
     if (!trapped)
-        return;
+        return NULL;
 
     sigtrapped[sig] = 0;
     if (sig == SIGINT && interact) {
@@ -769,19 +781,19 @@
      */
     if (trapped & ZSIG_FUNC) {
 	char func[20];
-	HashNode hn;
 
 	sprintf(func, "TRAP%s", sigs[sig]);
 	/*
 	 * As in dosavetrap(), don't call removeshfuncnode() because
 	 * that calls back into unsettrap();
 	 */
-	if ((hn = removehashnode(shfunctab, func)))
-	    shfunctab->freenode(hn);
+	return removehashnode(shfunctab, func);
     } else if (sigfuncs[sig]) {
 	freeeprog(sigfuncs[sig]);
 	sigfuncs[sig] = NULL;
     }
+
+    return NULL;
 }
 
 /**/

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

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

[Peter Stephenson]

"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@pwstephenson.fsnet.co.uk>
Work: pws@CambridgeSiliconRadio.com
Web: http://www.pwstephenson.fsnet.co.uk

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

[Bart Schaefer]

On Apr 30,  3:29pm, Peter Stephenson wrote:
} Subject: Re: PATCH: Re: localtraps causing core dump in 3.1.6+
}
} dosavetrap() now always copies the trap its saving.
} 
} I've merged it round Bart's changes, which were neater than mine in some
} places, and added a minimal localtraps test.

You missed a few tidbits.

} -    if (!strncmp(nam, "TRAP", 4))
} +    if (!strncmp(nam, "TRAP", 4) && (signum = getsignum(nam +4)) != -1)
}  	hn = removetrap(getsignum(nam + 4));

--- zsh-forge/current/Src/hashtable.c	Sun Apr 30 09:56:30 2000
+++ Src/hashtable.c	Sun Apr 30 09:58:25 2000
@@ -789,8 +789,8 @@
     HashNode hn;
     int signum;
 
-    if (!strncmp(nam, "TRAP", 4) && (signum = getsignum(nam +4)) != -1)
-	hn = removetrap(getsignum(nam + 4));
+    if (!strncmp(nam, "TRAP", 4) && (signum = getsignum(nam + 4)) != -1)
+	hn = removetrap(signum);
     else
 	hn = removehashnode(shfunctab, nam);
 
--- zsh-forge/current/Src/signals.c	Sun Apr 30 09:56:30 2000
+++ Src/signals.c	Sun Apr 30 10:01:28 2000
@@ -674,7 +674,6 @@
     } else {
 	st->list = sigfuncs[sig] ? dupeprog(sigfuncs[sig], 0) : NULL;
     }
-    noerrs = !!st->list;
     if (!savetraps)
 	savetraps = znewlinklist();
     /*
@@ -731,9 +730,7 @@
 void
 unsettrap(int sig)
 {
-    int ne = noerrs;
     HashNode hn = removetrap(sig);
-    noerrs = ne;
     if (hn)
 	shfunctab->freenode(hn);
 }

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com