From: Stephen McCamant <alias@mcs.com>
Date: Sat, 26 Jul 1997 00:55:03 +0000 (+1200)
Subject: Weirdness in sv_peek()
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=756698e49da5b17014f51650e301ccc98df5154e;p=p5sagit%2Fp5-mst-13.2.git

Weirdness in sv_peek()

Tom Phoenix writes:
 > On Sat, 26 Jul 1997, Stephen McCamant sent a patch and wrote:
 >
 > > I don't know what to say about this one.
 >
 > Well, it would be nice to tell us what it's supposed to accomplish! :-)

I've given it more thought, and I've thought of something to say. :-)

You've snipped the patch, but here's the gist of it:

Look at sv_peek(), around line 900 of sv.c. What this function does is
build a short description of the contents of an SV, for use in things
like -Ds stack dumps. It starts with an empty SV `t', and appends
various strings to it: the SV's type, its numeric value, etc. When
it's done, it returns the char * (PV) value of the SV. On line 955,
one case in the switch that has the type name ends in `return
tokenbuf'. tokenbuf is not a local variable, but a global one, not
used anywhere else in the function, that holds the last keyword token
that the lexer scanned. Why would the function return it when the SV
happened to be undefined?

I noticed this `weirdness', as I called it in the subject, when
running perl -Dts on a short program that had a bug in it:

/src/perl5.004_01+% dperl -Dts -e 'for ($x) { map(die, 0) }'

EXECUTING...

    =>
(-e:0)  enter
    =>
(-e:0)  nextstate
    =>
(-e:1)  pushmark
    =>  *
(-e:1)  gvsv(main::x)
    =>  *  die
(-e:1)  gv(main::_)
    =>  *  die   GV()
(-e:1)  enteriter
    =>  die
(-e:1)  iter
    =>  die   SV_YES
(-e:1)  and
    =>  die
(-e:1)  nextstate
    =>  die
(-e:1)  pushmark
    =>  die   *
(-e:1)  const(IV(0))
    =>  die   *  IV(0)
(-e:1)  mapstart
    =>  die   *  IV(0)  ***
(-e:1)  pushmark
    =>  die   *  IV(0)  ****
(-e:1)  die
Died at -e line 1.
Attempt to free unreferenced scalar.

Notice the large number of `die's. If I didn't know better, I'd think
perl was threatening me.

At the time I mailed the patch, the presence of that line in sv_peek()
didn't make any sense at all to me -- a total non sequitur. I
considered writing something about my puzzlement (even checking how to
spell `non sequitur'), but I couldn't think of anything fitting to
say.

In the time since, I've come up with a theory of where that line came
from. I don't have the source for any old 5.0 versions handy, but I
realized I did have a debugging binary of 5.003_07, and it didn't have
the bug. I now think this bug was introduced late in the 5.003_9?
series when Chip decided to fix every buffer overflow he could
find. Specifically, I theorize, sv_peek used to use tokenbuf to hold
the string it was working on, but it was changed to use an SV instead
of a fixed size buffer (that the SV was called `t' was a clue). The
fact that this line wasn't updated can then be chalked up to simple
oversight, and my sense of the order of the universe is intact.

Turns out I can say a lot about this, if I put my mind to it.

p5p-msgid: m0wsEMU-000EYLC@alias-2.pr.mcs.net
---

diff --git a/sv.c b/sv.c
index 1d50cf8..166dc07 100644
--- a/sv.c
+++ b/sv.c
@@ -952,7 +952,7 @@ register SV *sv;
 
     case SVt_NULL:
 	sv_catpv(t, "UNDEF");
-	return tokenbuf;
+	goto finish;
     case SVt_IV:
 	sv_catpv(t, "IV");
 	break;