Re: Wordcode functions with empty bodies

Re: Wordcode functions with empty bodies

[Sven Wischnowsky]

Peter Stephenson wrote:

> > If you can
> > find a particular chunk of code which seems to be doing something wrong
> > (shorter than the complete traps test) I'll have a look at it sometime.
> 
> While I'm waiting, there's something obviously wrong with the way exit
> tests are unset, but this looks too simple to be the source of everyone's
> problems --- which generically stem from the fact that traps can take two
> forms which are stored in the same place but manipulated in different ways.

No need to wait...

Try this: start with -f, autoload promptinit and call it. Then install 
break points at settrap(), freeeprog() and endtrapscope(). For even
more fun, do `display sigfuncs[28]' (SIGWINCH). Now type `prompt bart'.

It first hits some rather uninteresting break points, then settrap()
which installs the handler for SIGWINCH (from setfunction() in
parameter.c). Fine. Continue, it hits endtrapscode() and from there
frees the eprog in sigfuncs[28]. BUT it doesn't reset sigfuncs[28].
Continue again until you get to the shell prompt. Type `echo <RET>'
(this makes the memory for the eprog be freed) and look at
*sigfuncs[28]: garbage.

If you want to get the full picture: set a break point at
signals.c:675 and continue till there. It is now trying to dupeprog()
the thing in sigfuncs[28] -- kaboom.

So, where do we need to put the `sigfuncs[sig] = NULL'?


Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: Wordcode functions with empty bodies

[Peter Stephenson]

> Continue, it hits endtrapscode() and from there
> frees the eprog in sigfuncs[28]. BUT it doesn't reset sigfuncs[28].

Oh, I see, sorry.  The answer is it should disappear from there at the
point where it would do with an ordinary (non-function) trap.  The value is
passed back because it may still be required as a function node in its
other guise --- that came out of the previous set of fixes to local traps.

Ideally this value shouldn't be used anyway, because sigtrapped[SIGWINCH]
is now clear.  That was a problem in dosavetrap() --- it was correct to
save the trap, because it needs something to prompt it to delete the local
trap even if it just unsets it, but it should have checked sigtrapped to
see if the sigfuncs value was valid.  I've also stuck in a DPUTS there to
see if the invalid combination ever comes up.

In principle, the trap test should already pick this sort of thing up, it
just didn't crash often enough.  More DPUTS()'s are probably the answer to
similar future problems.

It would probably be better to rewrite the code so that trap functions are
never recorded in sigfuncs, just as a flag in sigtrapped, and always taken
from the functions hash when needed.

Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.8
diff -u -r1.8 signals.c
--- Src/signals.c	2000/06/14 14:19:20	1.8
+++ Src/signals.c	2000/06/14 15:06:43
@@ -671,8 +671,11 @@
 	    newshf->funcdef = dupeprog(shf->funcdef, 0);
 	}
 	st->list = newshf;
-    } else {
+    } else if (sigtrapped[sig]) {
 	st->list = sigfuncs[sig] ? dupeprog(sigfuncs[sig], 0) : NULL;
+    } else {
+	DPUTS(sigfuncs[sig], "BUG: sigfuncs not null for untrapped signal");
+	st->list = NULL;
     }
     if (!savetraps)
 	savetraps = znewlinklist();
@@ -789,6 +792,7 @@
 	 * As in dosavetrap(), don't call removeshfuncnode() because
 	 * that calls back into unsettrap();
 	 */
+	sigfuncs[sig] = NULL;
 	return removehashnode(shfunctab, func);
     } else if (sigfuncs[sig]) {
 	freeeprog(sigfuncs[sig]);

-- 
Peter Stephenson <pws@cambridgesiliconradio.com>
Cambridge Silicon Radio, Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070

Re: Wordcode functions with empty bodies

[Peter Stephenson]

> In principle, the trap test should already pick this sort of thing up, it
> just didn't crash often enough.  More DPUTS()'s are probably the answer to
> similar future problems.

Even so, it might be better to add this.  Does anyone have a better
version?

By the way, this answers a previous question of Bart's --- function traps
should be removed from the function table when localtraps is set.

Index: Test/08traps.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/08traps.ztst,v
retrieving revision 1.3
diff -u -r1.3 08traps.ztst
--- Test/08traps.ztst	2000/05/04 11:46:20	1.3
+++ Test/08traps.ztst	2000/06/14 15:15:18
@@ -175,3 +175,12 @@
 >testunset
 >f
 >ERR-or!
+
+  f() {
+    setopt localtraps
+    TRAPWINCH() { print "Window changed.  That wrecked the test."; }
+  }
+  f
+  f
+  functions TRAPWINCH
+1:Unsetting ordinary traps with localtraps.

-- 
Peter Stephenson <pws@cambridgesiliconradio.com>
Cambridge Silicon Radio, Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070

Re: Wordcode functions with empty bodies

[Bart Schaefer]

On Jun 14,  4:08pm, Peter Stephenson wrote:
} Subject: Re: Wordcode functions with empty bodies
}
} > Continue, it hits endtrapscode() and from there
} > frees the eprog in sigfuncs[28]. BUT it doesn't reset sigfuncs[28].
} 
} Oh, I see, sorry.  The answer is it should disappear from there at the
} point where it would do with an ordinary (non-function) trap.  [...]
} 
} Ideally this value shouldn't be used anyway, because sigtrapped[SIGWINCH]
} is now clear.  That was a problem in dosavetrap() --- it was correct to
} save the trap, because it needs something to prompt it to delete the local
} trap even if it just unsets it, but it should have checked sigtrapped to
} see if the sigfuncs value was valid.

This has fixed the "prompt bart" crash, though I still don't quite follow
why putting a `:' command in the function body (instead of using an empty
function) prevented the crash before.

However, either Clint's 11839 or my 11857 is still needed to prevent the
crash reported in 11837, so there's still something fishy going on with
saving and restoring the EXIT trap.  Or else the lack of 11857 _is_ the
fishy thing that's going on, but I don't follow how the ZSIG_FUNC bit is
set when there is no function.

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: Wordcode functions with empty bodies

[Peter Stephenson]

"Bart Schaefer" wrote:
> However, either Clint's 11839 or my 11857 is still needed to prevent the
> crash reported in 11837, so there's still something fishy going on with
> saving and restoring the EXIT trap.  Or else the lack of 11857 _is_ the
> fishy thing that's going on, but I don't follow how the ZSIG_FUNC bit is
> set when there is no function.

I found another *two* bugs (that's five today).  Apart from the bug which
was causing this --- premature setting of sigtrapped[SIGEXIT], causing that
to be set when resetting the old trap so that there as a flag but no
function --- there was another one:

% TRAPEXIT() { true; }
% accept-line() { zle .accept-line; }
% zle -N accept-line
% trap
TRAPEXIT () {
        true
}
% trap
# with the first bug, printed
BUG: I did not find any trap functions!
# with the second bug printed nothing

--- because dosavetrap() was saving an extra empty TRAPEXIT at locallevel
0, which got restored as no trap, the reason being TRAPEXIT is handled
specially to save an empty trap even if LOCALTRAPS is not set because it
gets saved and restored for every single function scope.  However, it
shouldn't get saved when *leaving* the function scope, which was what was
happening here.

Anybody else is welcome to rewrite this from scratch, taking account of all
the special cases.

I've added some extra DPUTS's, but they still don't catch all possible
problems.

Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.9
diff -u -r1.9 signals.c
--- Src/signals.c	2000/06/14 15:14:49	1.9
+++ Src/signals.c	2000/06/14 21:51:18
@@ -642,6 +642,7 @@
 };
 
 static LinkList savetraps;
+static int dontsavetrap;
 
 /*
  * Save the current trap by copying it.  This does nothing to
@@ -670,6 +671,10 @@
 	    newshf->flags = shf->flags;
 	    newshf->funcdef = dupeprog(shf->funcdef, 0);
 	}
+#ifdef DEBUG
+	else dputs("BUG: no function present with function trap flag set.");
+#endif
+	    
 	st->list = newshf;
     } else if (sigtrapped[sig]) {
 	st->list = sigfuncs[sig] ? dupeprog(sigfuncs[sig], 0) : NULL;
@@ -754,7 +759,8 @@
      * one, to aid in removing this one.  However, if there's
      * already one at the current locallevel we just overwrite it.
      */
-    if ((isset(LOCALTRAPS) || sig == SIGEXIT) && locallevel &&
+    if (!dontsavetrap && (isset(LOCALTRAPS) || sig == SIGEXIT) &&
+	locallevel &&
 	(!trapped || locallevel > (sigtrapped[sig] >> ZSIG_SHIFT)))
 	dosavetrap(sig, locallevel);
 
@@ -854,21 +860,19 @@
 
 	    remnode(savetraps, ln);
 
-	    if (sigtrapped[sig])
-		unsettrap(sig);
-	    sigtrapped[sig] = st->flags;
 	    if (st->flags && (st->list != NULL)) {
 		Eprog prog = (st->flags & ZSIG_FUNC) ?
 		    ((Shfunc) st->list)->funcdef : (Eprog) st->list;
 		/* prevent settrap from saving this */
-		int oldlt = opts[LOCALTRAPS];
-		opts[LOCALTRAPS] = 0;
+		dontsavetrap++;
 		settrap(sig, prog);
-		opts[LOCALTRAPS] = oldlt;
+		dontsavetrap--;
 		if ((sigtrapped[sig] = st->flags) & ZSIG_FUNC)
 		    shfunctab->addnode(shfunctab, ((Shfunc)st->list)->nam,
 				       (Shfunc) st->list);
-	    }
+	    } else if (sigtrapped[sig])
+		unsettrap(sig);
+
 	    zfree(st, sizeof(*st));
 	}
     }
@@ -881,6 +885,8 @@
 	else
 	    freeeprog(exitfn);
     }
+    DPUTS(!locallevel && savetraps && firstnode(savetraps),
+	  "BUG: still saved traps outside all function scope");
 }
 
 /* Execute a trap function for a given signal, possibly

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

Re: Wordcode functions with empty bodies

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> This has fixed the "prompt bart" crash, though I still don't quite follow
> why putting a `:' command in the function body (instead of using an empty
> function) prevented the crash before.

Purely accidental. It freed the eprog, then allocated some more memory 
(sometimes for another eprog(!)) and later copied the freed eprog. If
the thing allocated into the freed eprog was something looking
(almost) like a valid eprog, it worked. With something in the body of
the function this (sometimes) happened, sometimes not. While debugging 
this, I had cases where it allocated something sensible into the freed 
eprog, sometimes not, both with and without something in the function
body.


Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: Wordcode functions with empty bodies

[Sven Wischnowsky]

Bart Schaefer wrote:

> On Jun 14,  8:14am, Sven Wischnowsky wrote:
> } Subject: Re: Wordcode functions with empty bodies
> }
> } I had a little debugging session yesterday evening... I could
> } reproduce the segv with the `prompt' thing (although I had to invoke
> } `prompt bart' a second time with some other command before it to make
> } it go kaboom).
> } 
> } I could not, however, see any problems with bld_eprog(). In which way
> } do you think it produces garbled eprogs? (I mean, what do those eprogs 
> } look like?)
> 
> They have a `len' of 4 and a `strs' that points to four bytes of garbage.
> The crashes appear to happen after dupeprog() copies `strs' -- the new
> copy often ends up pointing to a different four bytes of garbage.

I almost thought that you meant this... that's ok, even if it looks
weird. `len' is the total length of the memory block used for
patterns, the word code and the string table. `prog' and `strs' point
into that memory at the rightpositions, `strs' after the word
code. Since there are no strings it points to the memory *after* the
word code... but it will never be used.


Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: Wordcode functions with empty bodies

[Bart Schaefer]

On Jun 14,  8:14am, Sven Wischnowsky wrote:
} Subject: Re: Wordcode functions with empty bodies
}
} I had a little debugging session yesterday evening... I could
} reproduce the segv with the `prompt' thing (although I had to invoke
} `prompt bart' a second time with some other command before it to make
} it go kaboom).
} 
} I could not, however, see any problems with bld_eprog(). In which way
} do you think it produces garbled eprogs? (I mean, what do those eprogs 
} look like?)

They have a `len' of 4 and a `strs' that points to four bytes of garbage.
The crashes appear to happen after dupeprog() copies `strs' -- the new
copy often ends up pointing to a different four bytes of garbage.

In the prompt stuff, as long as I never let the function body be empty --
that is, I replace e.g.

        functions[TRAPWINCH]="${functions[TRAPWINCH]//prompt_bart_winch}"

with

        functions[TRAPWINCH]="${functions[TRAPWINCH]//prompt_bart_winch/:}"

the crashes never happen.  Only if the function body becomes empty do I
have any problem.
 
} Ok. If you could give me an easy example of how to make bld_eprog()
} give garbled results, I'd be thankful. Just doing `functions[foo]=""'
} at least gives me sensible results.

Try `functions[foo]=" "' so there is a non-empty string there but it
contains no useful tokens.

I'll try to do some more debugging this morning and send some gdb output.

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: Wordcode functions with empty bodies

[Peter Stephenson]

> If you can
> find a particular chunk of code which seems to be doing something wrong
> (shorter than the complete traps test) I'll have a look at it sometime.

While I'm waiting, there's something obviously wrong with the way exit
tests are unset, but this looks too simple to be the source of everyone's
problems --- which generically stem from the fact that traps can take two
forms which are stored in the same place but manipulated in different ways.

Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.7
diff -u -r1.7 signals.c
--- Src/signals.c	2000/06/09 14:40:54	1.7
+++ Src/signals.c	2000/06/14 12:40:24
@@ -837,9 +837,9 @@
 	    exitfn = removehashnode(shfunctab, "TRAPEXIT");
 	} else {
 	    exitfn = sigfuncs[SIGEXIT];
-	    sigfuncs[SIGEXIT] = NULL;
 	}
-	unsettrap(SIGEXIT);
+	sigfuncs[SIGEXIT] = NULL;
+	sigtrapped[SIGEXIT] = 0;
     }
 
     if (savetraps) {

-- 
Peter Stephenson <pws@cambridgesiliconradio.com>
Cambridge Silicon Radio, Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070

Re: Wordcode functions with empty bodies

[Peter Stephenson]

> Placing some breakpoints in signals.c, I noticed a) that I think the
> memory handling there seems to be wrong and b) that I don't understand 
> the control flow (again), so I don't dare to fiddle with it.
> 
> About a): the freeeprogs() don't seem to get called. See also Felix'
> last memory leak message (11766), describing a memory leak there. So I 
> think the eprog copied at line 675 somehow survives too long, is freed 
> elsewhere (does signals.c really keep the original?) and later
> accessed. Or something. (In my reply to 11766 in 11796 I meant to say
> that maybe Peter should have a look at it, him probably being the only 
> one who really understands what goes on in signals.c or what should go 
> on...)

I don't, and never have, understood signal handling, I just rewrote the
trap handling rather minimally to implement localtraps, and have simplified
it at least twice since then, so it probably needs it again.  If you can
find a particular chunk of code which seems to be doing something wrong
(shorter than the complete traps test) I'll have a look at it sometime.

-- 
Peter Stephenson <pws@cambridgesiliconradio.com>
Cambridge Silicon Radio, Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070

Re: Wordcode functions with empty bodies

[Sven Wischnowsky]

Bart Schaefer wrote:

> This is mostly aimed at Sven ... just a reminder that the bug discussed in
> 11837-11841 and 11854-11859 appears to have something to do with wordcode
> compiled from a function whose body is the single token NULLTOK.

I had a little debugging session yesterday evening... I could
reproduce the segv with the `prompt' thing (although I had to invoke
`prompt bart' a second time with some other command before it to make
it go kaboom).

I could not, however, see any problems with bld_eprog(). In which way
do you think it produces garbled eprogs? (I mean, what do those eprogs 
look like?)

Placing some breakpoints in signals.c, I noticed a) that I think the
memory handling there seems to be wrong and b) that I don't understand 
the control flow (again), so I don't dare to fiddle with it.

About a): the freeeprogs() don't seem to get called. See also Felix'
last memory leak message (11766), describing a memory leak there. So I 
think the eprog copied at line 675 somehow survives too long, is freed 
elsewhere (does signals.c really keep the original?) and later
accessed. Or something. (In my reply to 11766 in 11796 I meant to say
that maybe Peter should have a look at it, him probably being the only 
one who really understands what goes on in signals.c or what should go 
on...)


Ok. If you could give me an easy example of how to make bld_eprog()
give garbled results, I'd be thankful. Just doing `functions[foo]=""'
at least gives me sensible results.

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Wordcode functions with empty bodies

[Bart Schaefer]

This is mostly aimed at Sven ... just a reminder that the bug discussed in
11837-11841 and 11854-11859 appears to have something to do with wordcode
compiled from a function whose body is the single token NULLTOK.

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net