bleadperl 2154eca7 breaks HTML::Parser 3.66
(Andreas J. Koenig)
2154eca77956ce145743765bea9ce269e6227984 is the first bad commit
commit 2154eca77956ce145743765bea9ce269e6227984
Author: Eric Brine <ike...@adaelis.com>
Date: Sat Jul 31 01:56:43 2010 -0700
Fix untimely destruction introduced by lvalue ops [RT#67838] by returning a TEMP instead of using TARG. Made appropriate TODO tests live.
perl -V:
Summary of my perl5 (revision 5 version 13 subversion 3) configuration:
Commit id: 2154eca77956ce145743765bea9ce269e6227984
Platform:
osname=linux, osvers=2.6.32-2-amd64, archname=x86_64-linux
uname='linux k81 2.6.32-2-amd64 #1 smp fri feb 12 00:01:47 utc 2010 x86_64 gnulinux '
config_args='-Dprefix=/home/src/perl/repoperls/installed-perls/perl/v5.13.3-197-g2154eca -Dinstallusrbinperl=n -Uversiononly -Dusedevel -des -Ui_db -Uuseithreads -Uuselongdouble'
hint=recommended, useposix=true, d_sigaction=define
useithreads=undef, usemultiplicity=undef
useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
use64bitint=define, use64bitall=define, uselongdouble=undef
usemymalloc=n, bincompat5005=undef
Compiler:
cc='cc', ccflags ='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
optimize='-O2',
cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
ccversion='', gccversion='4.4.4', gccosandvers=''
intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
alignbytes=8, prototype=define
Linker and Libraries:
ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
libpth=/usr/local/lib /lib /usr/lib /lib64 /usr/lib64
libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
libc=/lib/libc-2.11.2.so, so=so, useshrplib=false, libperl=libperl.a
gnulibc_version='2.11.2'
Dynamic Linking:
dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'
Characteristics of this binary (from libperl):
Compile-time options: PERL_DONT_CREATE_GVSV PERL_MALLOC_WRAP PERL_USE_DEVEL
USE_64_BIT_ALL USE_64_BIT_INT USE_LARGE_FILES
USE_PERLIO USE_PERL_ATOF
Built under linux
Compiled at Aug 14 2010 05:40:01
@INC:
/home/src/perl/repoperls/installed-perls/perl/v5.13.3-197-g2154eca/lib/site_perl/5.13.3/x86_64-linux
/home/src/perl/repoperls/installed-perls/perl/v5.13.3-197-g2154eca/lib/site_perl/5.13.3
/home/src/perl/repoperls/installed-perls/perl/v5.13.3-197-g2154eca/lib/5.13.3/x86_64-linux
/home/src/perl/repoperls/installed-perls/perl/v5.13.3-197-g2154eca/lib/5.13.3
.
HTH,
--
andreas
Nicholas Clark
> git bisect:
>
> 2154eca77956ce145743765bea9ce269e6227984 is the first bad commit
> commit 2154eca77956ce145743765bea9ce269e6227984
> Author: Eric Brine <ike...@adaelis.com>
> Date: Sat Jul 31 01:56:43 2010 -0700
>
> Fix untimely destruction introduced by lvalue ops [RT#67838] by returning a TEMP instead of using TARG. Made appropriate TODO tests live.
OK, so what this boils down to is the change of flags on the return value
of substr
Before, pPOK is one of the flags, so SvOK() is true:
$ /Users/nick/Sandpit/snap5.9.x-v5.13.3-196-g0607bed/bin/perl5.13.3 -MDevel::Peek -e 'Dump(substr $a, 0, 0)'
SV = PVLV(0x100824000) at 0x1008126d0
REFCNT = 1
FLAGS = (PADMY,GMG,SMG,pPOK)
IV = 0
NV = 0
PV = 0x100501e70 ""\0
CUR = 0
LEN = 16
MAGIC = 0x1005093a0
MG_VIRTUAL = &PL_vtbl_substr
MG_TYPE = PERL_MAGIC_substr(x)
TYPE = x
TARGOFF = 0
TARGLEN = 0
TARG = 0x1008126a0
SV = PV(0x1008010f0) at 0x1008126a0
REFCNT = 2
FLAGS = (POK,pPOK)
PV = 0x100501150 ""\0
CUR = 0
LEN = 16
After, it's not there, so SvOK() is false:
$ /Users/nick/Sandpit/snap5.9.x-v5.13.3-197-g2154eca/bin/perl5.13.3 -MDevel::Peek -e 'Dump(substr $a, 0, 0)'
SV = PVLV(0x100824000) at 0x1008030b8
REFCNT = 1
FLAGS = (TEMP,GMG,SMG)
IV = 0
NV = 0
PV = 0
MAGIC = 0x1005093a0
MG_VIRTUAL = &PL_vtbl_substr
MG_TYPE = PERL_MAGIC_substr(x)
TYPE = x
TARGOFF = 0
TARGLEN = 0
TARG = 0x1008126a0
SV = PV(0x1008010f0) at 0x1008126a0
REFCNT = 2
FLAGS = (POK,pPOK)
PV = 0x100501e70 ""\0
CUR = 0
LEN = 16
In Parser.xs, check_handler looks like this:
static SV*
check_handler(pTHX_ SV* h)
{
if (SvROK(h)) {
SV* myref = SvRV(h);
if (SvTYPE(myref) == SVt_PVCV)
return newSVsv(h);
if (SvTYPE(myref) == SVt_PVAV)
return SvREFCNT_inc(myref);
croak("Only code or array references allowed as handler");
}
return SvOK(h) ? newSVsv(h) : 0;
}
It's called at this point in t/handler.t
$x = \substr("xfoo", 1);
$p->handler(start => $$x, "self,attr");
Before, when we get there, the SV (as shown above) has sufficient flags already
set that SvOK() will be true.
128 if (SvROK(h)) {
(gdb) call (void) Perl_sv_dump(h)
warning: .o file "/Volumes/Stuff/Perl/perl/libperl.a(dump.o)" more recent than executable timestamp in "/Volumes/Stuff/Sandpit/snap5.9.x-v5.13.3-196-g0607bed/bin/perl5.13.3"
warning: Couldn't open object file '/Volumes/Stuff/Perl/perl/libperl.a(dump.o)'
warning: Error expanding psymtab dump.c to symtab in lookup_symbol_aux_psymtabs()
SV = PVLV(0x10083b790) at 0x10085a4f8
REFCNT = 2
FLAGS = (PADMY,GMG,SMG,pPOK)
IV = 0
NV = 0
PV = 0x1006751d0 "foo"\0
CUR = 3
LEN = 16
MAGIC = 0x100675840
MG_VIRTUAL = &PL_vtbl_substr
MG_TYPE = PERL_MAGIC_substr(x)
TYPE = x
TARGOFF = 1
TARGLEN = 3
TARG = 0x10085a4b0
SV = PV(0x100840810) at 0x10085a4b0
REFCNT = 2
FLAGS = (POK,READONLY,pPOK)
PV = 0x100634030 "xfoo"\0
CUR = 4
LEN = 16
After, it doesn't
check_handler (h=0x100886df0) at Parser.xs:128
128 if (SvROK(h)) {
(gdb) call (void) Perl_sv_dump(h)
SV = PVLV(0x10083b600) at 0x100886df0
REFCNT = 1
FLAGS = (GMG,SMG)
IV = 0
NV = 0
PV = 0
MAGIC = 0x1006752e0
MG_VIRTUAL = &PL_vtbl_substr
MG_TYPE = PERL_MAGIC_substr(x)
TYPE = x
TARGOFF = 1
TARGLEN = 3
TARG = 0x1008bfc80
SV = PV(0x100840810) at 0x1008bfc80
REFCNT = 2
FLAGS = (POK,READONLY,pPOK)
PV = 0x100634170 "xfoo"\0
CUR = 4
LEN = 16
The appended patch makes all the tests pass - I'm not sure if it is the right
fix though. How much other code will be surprised because SvOK() is no longer
true on values where previously it was?
Nicholas Clark
--- Parser.xs~ 2009-10-23 19:31:00.000000000 +0100
+++ Parser.xs 2010-08-14 17:48:28.000000000 +0100
@@ -125,6 +125,7 @@
static SV*
check_handler(pTHX_ SV* h)
{
+ SvGETMAGIC(h);
if (SvROK(h)) {
SV* myref = SvRV(h);
if (SvTYPE(myref) == SVt_PVCV)
Vincent Pit
> The appended patch makes all the tests pass - I'm not sure if it is the right
> fix though.
> How much other code will be surprised because SvOK() is no longer
> true on values where previously it was?
Possibly some, but it's not really a new issue. Calling SvOK on a
magical SV is something that can have happened from way before this
change. So I suspect that for most of the uses of SvOK in the nature :
- either they are already broken with another kind of magic than those
concerned by this change ;
- or they are already calling SvGETMAGIC.
Vincent.
Eric Brine
> I'm not sure if it is the right fix though.
>
> +++ Parser.xs 2010-08-14 17:48:28.000000000 +0100
> @@ -125,6 +125,7 @@
> static SV*
> check_handler(pTHX_ SV* h)
> {
> + SvGETMAGIC(h);
> if (SvROK(h)) {
> SV* myref = SvRV(h);
> if (SvTYPE(myref) == SVt_PVCV)
>
The code didn't support magical value, but was being given some.
Making it handle magical values is definitely the way to go.
We could also monkeypatch substr to prefetch the string if it is
deemed necessary.
Gisle Aas
> On Sat, Aug 14, 2010 at 12:58 PM, Nicholas Clark <ni...@ccl4.org> wrote:
>> I'm not sure if it is the right fix though.
>>
>> --- Parser.xs~ 2009-10-23 19:31:00.000000000 +0100
>> +++ Parser.xs 2010-08-14 17:48:28.000000000 +0100
>> @@ -125,6 +125,7 @@
>> static SV*
>> check_handler(pTHX_ SV* h)
>> {
>> + SvGETMAGIC(h);
>> if (SvROK(h)) {
>> SV* myref = SvRV(h);
>> if (SvTYPE(myref) == SVt_PVCV)
>>
>
> The code didn't support magical value, but was being given some.
> Making it handle magical values is definitely the way to go.
I've now applied Nick's patch to my sources: <http://github.com/gisle/html-parser/commit/b57554389001>
> We could also monkeypatch substr to prefetch the string if it is
> deemed necessary.
I don't think that would be necessary.
Regards,
Gisle