From at@altlinux.ru

Created by at@altlinux.ru

Hello,

Recently I reported this bug on perl5-porters mailing list.
As there seems to be no immediate interest to this bug,
I file it for future revision. I assign severity=medium
because reference counting mechanism seems to be affected.

Brief description​: consequent svref_2object calls produce wrong
B objects in certain cases. Detailed explanation and test cases
are available via the following links​:

http​://www.nntp.perl.org/group/perl.perl5.porters/96593
http​://www.nntp.perl.org/group/perl.perl5.porters/96600

--
Alexey Tourbin
ALT Linux Team

Flags:
    category=core
    severity=medium

Site configuration information for perl v5.8.4:

Configured by ALT Linux Team at Mon Aug  9 06:08:01 MSD 2004.

Summary of my perl5 (revision 5 version 8 subversion 4) configuration:
  Platform:
    osname=linux, osvers=2.4.18-alt6master-up, archname=i386-linux-thread-multi
    uname='linux solemn.turbinal.org 2.4.18-alt6master-up #1 tue apr 16 14:50:56 msd 2002 i686 unknown unknown gnulinux '
    config_args='-de -rs -Darchname=i386-linux -Dd_dosuid -Ud_csh -Dlibswanted=dl m c crypt db ndbm gdbm -Duseshrplib -Dlibperl=libperl.so.5.8 -Dcc=gcc -Doptimize=-pipe -mmmx -Wall -Os -march=i686 -D_GNU_SOURCE -momit-leaf-frame-pointer -Dcccdlflags=-fPIC -DPIC -Dccdlflags=-rdynamic -Wl,-O1 -Dlddlflags=-shared -Wl,-O1 -Dldflags=-Wl,-O1 -Dinstallprefix=/usr -Dprefix=/usr -Dprivlib=/usr/lib/perl5 -Darchlib=/usr/lib/perl5/i386-linux -Dvendorprefix=/usr -Dvendorlib=/usr/lib/perl5/vendor_perl -Dvendorarch=/usr/lib/perl5/vendor_perl/i386-linux -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dvendorman1dir=/usr/share/man/man1 -Dvendorman3dir=/usr/share/man/man3 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/lib/perl5/site_perl/5.8.4 -Dsitearch=/usr/local/lib/perl5/site_perl/5.8.4/i386-linux -Dsiteman1dir=/usr/local/man/man1 -Dsite_man3dir=/usr/local/man/man3 -Dcf_by=ALT Linux Team -Dcf_email=qa@altlinux.org -Dmyhostname=localhost -Dperladmin=root@localhost -Dmyuname=Linux 2.4.18-alt6master-up i686 -Dnewmyuname=Linux 2.4.18-alt6master-up i686 -Dinc_version_list=5.8.3/i386-linux 5.8.2/i386-linux 5.8.1/i386-linux 5.8.0/i386-linux 5.8.3 5.8.2 5.8.1 5.8.0 5.6.1 5.6.0 -Dpager=/usr/bin/less -isR -Di_shadow -Di_syslog -Dusethreads -Duseithreads -Duselargefiles -Di_db -Di_gdbm -Di_ndbm -Di_sdbm -Ui_odbm'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=define use5005threads=undef useithreads=define usemultiplicity=define
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=undef use64bitall=undef uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I/usr/include/gdbm',
    optimize='-pipe -mmmx -Wall -Os -march=i686 -D_GNU_SOURCE -momit-leaf-frame-pointer',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -fno-strict-aliasing -I/usr/local/include -I/usr/include/gdbm'
    ccversion='', gccversion='3.3.3 20040412 (ALT Linux, build 3.3.3-alt5)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags ='-Wl,-O1 -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-ldl -lm -lpthread -lc -lcrypt -ldb -lgdbm
    perllibs=-ldl -lm -lpthread -lc -lcrypt
    libc=/lib/libc-2.3.3.so, so=so, useshrplib=true, libperl=libperl.so.5.8
    gnulibc_version='2.3.3'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-rdynamic -Wl,-O1'
    cccdlflags='-fPIC -DPIC', lddlflags='-shared -Wl,-O1 -L/usr/local/lib'

Locally applied patches:
    


@INC for perl v5.8.4:
    /etc/perl5
    /usr/lib/perl5/i386-linux
    /usr/lib/perl5
    /usr/local/lib/perl5/site_perl/5.8.4/i386-linux
    /usr/local/lib/perl5/site_perl/5.8.4
    /usr/local/lib/perl5/site_perl/5.8.2/i386-linux
    /usr/local/lib/perl5/site_perl/5.8.1/i386-linux
    /usr/local/lib/perl5/site_perl/5.8.2
    /usr/local/lib/perl5/site_perl/5.8.1
    /usr/local/lib/perl5/site_perl/5.6.1
    /usr/local/lib/perl5/site_perl
    /usr/lib/perl5/vendor_perl/i386-linux
    /usr/lib/perl5/vendor_perl
    /usr/lib/perl5/vendor_perl
    .


Environment for perl v5.8.4:
    HOME=/home/at
    LANG=C
    LANGUAGE (unset)
    LC_COLLATE=ru_RU.CP1251
    LC_CTYPE=ru_RU.CP1251
    LC_MESSAGES=C
    LC_MONETARY=ru_RU.CP1251
    LC_NUMERIC=ru_RU.CP1251
    LC_TIME=C
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/at/bin:/bin:/usr/bin:/usr/X11R6/bin:/usr/local/bin:/usr/games:/sbin:/usr/sbin:/usr/local/sbin
    PERL_BADLANG (unset)
    SHELL=/bin/zsh

From smcc@OCF.Berkeley.EDU

"AT" == Alexey Tourbin <(via RT) <perlbug-followup@​perl.org>> writes​:

AT> Hello,

AT> Recently I reported this bug on perl5-porters mailing list.
AT> As there seems to be no immediate interest to this bug,
AT> I file it for future revision. I assign severity=medium
AT> because reference counting mechanism seems to be affected.

AT> Brief description​: consequent svref_2object calls produce wrong
AT> B objects in certain cases. Detailed explanation and test cases
AT> are available via the following links​:

AT> http​://www.nntp.perl.org/group/perl.perl5.porters/96593
AT> http​://www.nntp.perl.org/group/perl.perl5.porters/96600

I think your diagnosis is correct. The B module in general, and
svref_2object in particular, does not increase the reference count of
the SV and OP objects the B​::SV and B​::OP proxies point to, so bad
things happen if the proxy is used after the underlying object is
freed. (I bet with more experimentation you could get a segfault, for
instance).

However, I think the best solution is "don't do that, then". B is
intended for looking at the parse tree, which isn't generally in
danger of going away, rather than for examining transient values. It
wouldn't be out of the question to change B so that it always
SvREFCNT_inc'd the SVs it wrapped, but it would be a fairly major
change, with some tricky aspects (for instance, should it try to hide
the increased count from the B​::SV​:REFCNT method?). Trying to
reference count OPs would be harder, since most of them don't actually
store a reference count.

The appended documentation patch attempts to explain that one
shouldn't do what you tried to do.

-- Stephen

Index​: B.pm

--- B.pm (revision 18175)
+++ B.pm (working copy)
@​@​ -368,6 +368,10 @​@​
way to get an initial "handle" on an internal perl data structure
which can then be followed with the other access methods.

+The returned object will only be valid as long as the underlying OPs
+and SVs continue to exist. Do not attempt to use the object after the
+underlying structures are freed.
+
=item amagic_generation

Returns the SV object corresponding to the C variable C<amagic_generation>.
@​@​ -523,7 +527,11 @​@​
these structures.

Note that all access is read-only. You cannot modify the internals by
-using this module.
+using this module. Also, note that the B​::OP and B​::SV objects created
+by this module are only valid for as long as the underlying objects
+exist; their creation doesn't increase the reference counts of the
+underlying objects. Trying to access the fields of a freed object will
+give incomprehensible results, or worse.

=head2 SV-RELATED CLASSES

The RT System itself - Status changed from 'new' to 'open'

From at@altlinux.ru

On Tue, Dec 28, 2004 at 04​:01​:49PM -0800, Stephen McCamant wrote​:

AT> Recently I reported this bug on perl5-porters mailing list.
AT> As there seems to be no immediate interest to this bug,
AT> I file it for future revision. I assign severity=medium
AT> because reference counting mechanism seems to be affected.

AT> Brief description​: consequent svref_2object calls produce wrong
AT> B objects in certain cases. Detailed explanation and test cases
AT> are available via the following links​:

AT> http​://www.nntp.perl.org/group/perl.perl5.porters/96593
AT> http​://www.nntp.perl.org/group/perl.perl5.porters/96600

I think your diagnosis is correct. The B module in general, and
svref_2object in particular, does not increase the reference count of
the SV and OP objects the B​::SV and B​::OP proxies point to, so bad
things happen if the proxy is used after the underlying object is
freed. (I bet with more experimentation you could get a segfault, for
instance).

Okay, thanks a lot for this clarification. Unfortunately I don't quite
grok reference counting mechanism in Perl (for now). I just have a code
that has a kludge that apparently fixes the mechanism for perl-5.8.x.
The code is as follows​:

[...]
  use B qw(svref_2object);
  our $perlbug32967 = \$version;
  my $v = sv_version(svref_2object(\$version));
  # process $v
[...]
use B qw(class);
sub sv_version ($) {
  my $sv = shift;
  my $class = class($sv);
  if ($class eq "IV" or $class eq "PVIV") {
  return $sv->int_value;
  }
  if ($class eq "NV" or $class eq "PVNV") {
  return $sv->NV;
  }
  if ($class eq "PVMG") {
  for (my $mg = $sv->MAGIC; $mg; $mg = $mg->MOREMAGIC) {
  next if $mg->TYPE ne "V";
  my @​v = $mg->PTR =~ /(\d+)/g;
  return $v[0] + $v[1] / 1000 + $v[2] / 1000 / 1000;
  }
  }
  if ($sv->can("PV")) {
  my $v = $sv->PV;
  if ($v =~ /^\s*\.?\d/) {
  $v =~ s/_//g;
  return $v + 0;
  }
  }
  return undef;
}
__END__

You see, without referencing $version to global $perlbug32967 variable,
I get a very wrong result (wrong SV object), as of perl-5.8.6. And this
kludge has *no effect* on perl-5.6.1.

So I feel like there's still a subtle problem​: I can't understand why
$version and/or svref_2object(\$version) is freed upon subsequent
sv_version() calls. (Well, I only want to know how to fix my code.)

To examine the real code, you may want to take a look at
http​://search.cpan.org/dist/rpm-build-perl/ (only PVMG version strings
seems to break things). My original post has a very simple example, though.

However, I think the best solution is "don't do that, then". B is
intended for looking at the parse tree, which isn't generally in
danger of going away, rather than for examining transient values. It
wouldn't be out of the question to change B so that it always
SvREFCNT_inc'd the SVs it wrapped, but it would be a fairly major
change, with some tricky aspects (for instance, should it try to hide
the increased count from the B​::SV​:REFCNT method?). Trying to
reference count OPs would be harder, since most of them don't actually
store a reference count.

The appended documentation patch attempts to explain that one
shouldn't do what you tried to do.

-- Stephen

From @ysth

On Tue, Dec 28, 2004 at 04​:01​:49PM -0800, Stephen McCamant wrote​:

However, I think the best solution is "don't do that, then". B is
intended for looking at the parse tree, which isn't generally in
danger of going away, rather than for examining transient values. It
wouldn't be out of the question to change B so that it always
SvREFCNT_inc'd the SVs it wrapped, but it would be a fairly major
change, with some tricky aspects (for instance, should it try to hide
the increased count from the B​::SV​:REFCNT method?). Trying to
reference count OPs would be harder, since most of them don't actually
store a reference count.

So you probably also shouldn't try to do things like​:

[http​://perlmonks.org/index.pl?node_id=379395]

? :)

From smcc@mit.edu

"AT" == Alexey Tourbin <at@​altlinux.ru> writes​:

AT> Brief description​: consequent svref_2object calls produce wrong
AT> B objects in certain cases. Detailed explanation and test cases
AT> are available via the following links​:

AT> http​://www.nntp.perl.org/group/perl.perl5.porters/96593
AT> http​://www.nntp.perl.org/group/perl.perl5.porters/96600

SMcC> I think your diagnosis is correct. The B module in general, and
SMcC> svref_2object in particular, does not increase the reference
SMcC> count of the SV and OP objects the B​::SV and B​::OP proxies point
SMcC> to, so bad things happen if the proxy is used after the
SMcC> underlying object is freed. (I bet with more experimentation you
SMcC> could get a segfault, for instance).

AT> Okay, thanks a lot for this clarification. Unfortunately I don't quite
AT> grok reference counting mechanism in Perl (for now). I just have a code
AT> that has a kludge that apparently fixes the mechanism for perl-5.8.x.
AT> The code is as follows​:

AT> [...] [snip]

AT> To examine the real code, you may want to take a look at
AT> http​://search.cpan.org/dist/rpm-build-perl/ (only PVMG version
AT> strings seems to break things). My original post has a very
AT> simple example, though.

I think I can explain more easily what's going on in your simple
example. For the readers playing along at home, your short example
was​:

AT> my $v1 = 0.17;
AT> my $v2 = 0.1.7;
AT> sub get_sv {
AT> my $v = shift;
AT> my $sv = svref_2object(\$v);
AT> return $sv;
AT> }
AT> Dump get_sv($v1);
AT> get_sv($v2);
AT> Dump get_sv($v1);

The first Dump shows that its argument is a reference to a B​::NV
object, while the second shows its to be a reference to a B​::PVMG.

What I think may be confusing about this example is that get_sv
doesn't actually tell you about its argument SV. Rather, it returns a
B​::SV object that describes its local $v variable. When, as in the
unmodified example, no reference to $v escapes the subroutine (the
reference count of $v is 1 on leaving the subroutine), perl doesn't
bother to free the local; rather, it keeps it around to reuse the next
time the subroutine is called. This allows it to save time that would
otherwise be spent reallocating and "upgrading" the local on the next
sub call; it assumes that if you made an SV big and complicated on one
invocation, you likely will again.

Thus all three calls to get_sv return B​::SV objects referring to the
same SV. When a PVMG is passed in the second get_sv() call, $v is
upgraded to accommodate it, and it stays upgraded for the third call.

If you change the code to pass the variable by reference, i.e.

sub get_sv {
  my $vref = shift;
  my $sv = svref_2object($vref);
  return $sv;
}
Dump get_sv(\$v1);
  get_sv(\$v2);
Dump get_sv(\$v1);

you'll see that $v1 stays an NV.

To sharpen what I was saying above, I don't think your bug comes from
an SV being freed to soon; rather, the SV is being reused
unexpectedly. Making another reference to the variable is a workaround
because it disables the reuse optimization; perl allocates a new SV,
which your code handles better.

On to how you might fix your real bug (note I've added a bit more
context in the first sub)​:

AT> sub extract_version {
AT> [...]
AT> my $version = ....;
AT> [...]
AT> use B qw(svref_2object);
AT> our $perlbug32967 = \$version;
AT> my $v = sv_version(svref_2object(\$version));
AT> # process $v
AT> [...]
AT> use B qw(class);
AT> sub sv_version ($) {
AT> my $sv = shift;
AT> my $class = class($sv);
AT> if ($class eq "IV" or $class eq "PVIV") {
AT> return $sv->int_value;
AT> }
AT> if ($class eq "NV" or $class eq "PVNV") {
AT> return $sv->NV;
AT> }
AT> if ($class eq "PVMG") {
AT> for (my $mg = $sv->MAGIC; $mg; $mg = $mg->MOREMAGIC) {
AT> next if $mg->TYPE ne "V";
AT> my @​v = $mg->PTR =~ /(\d+)/g;
AT> return $v[0] + $v[1] / 1000 + $v[2] / 1000/1000;
AT> }
AT> }
AT> if ($sv->can("PV")) {
AT> my $v = $sv->PV;
AT> if ($v =~ /^\s*\.?\d/) {
AT> $v =~ s/_//g;
AT> return $v + 0;
AT> }
AT> }
AT> return undef;
AT> }

I think the real problem with sv_version is that its logic is wrong​:
it's trying to switch on the representation type of the SV, rather
than the contents. Just because an SV is a PVMG doesn't mean it has
magic now (in particular, because SVs can't be "downgraded").

For a single invocation that shows this problem, try​:

my $x = 3;
study $x;
$x = 4;

print sv_version(svref_2object(\$x)), "\n";

The first three lines make $x a PVMG with no string value or V magic
whose value is stored as an integer, but your code skips the branch
that would have called $sv->int_value because it's deciding based on
the SV's class.

I'd suggest testing in the following order​:

if (SV has "V" magic) {
  parse V-string
}
if (SV is IOK) {
  use int_value
}
if (SV is NOK) {
  use NV
}
if (SV is POK) {
  parse PV
}

Hope this helps,

-- Stephen

From @rgs

Stephen McCamant wrote​:

The appended documentation patch attempts to explain that one
shouldn't do what you tried to do.

My favourite ones :)

Index​: B.pm

--- B.pm (revision 18175)
+++ B.pm (working copy)

Thanks, applied as 23845 to bleadperl.

@smpeters - Status changed from 'open' to 'resolved'