REFCNT wrong on tied hash object following assignment to keys()

p5pRT commented 22 years ago

Migrated from rt.perl.org#8902 (status was 'resolved')

Searchable as RT8902$

p5pRT commented 22 years ago

From @mjdominus

Created by @mjdominus

$^W = 1; sub X::TIEARRAY { my ($pack\, %h) = shift; keys %h = 64; # removing this line makes the problem go away bless \%h => $pack; }

my $x = tie @x\, 'X'; # Devel::Peek::dump shows that at this point\, %$x has REFCNT = 3 undef $x; untie @x;

When run\, this program (incorrectly) emits the warning

untie attempted while 1 inner references still exist at /tmp/untie.pl line 11.

(Line 11 is the 'untie' line.)

Commenting out the assignment to 'keys' at line 4 makes the warning go away; the REFCNT is correct in that case.

The problem persists in 5.7.3 and in this morning's bleadperl.

Perl Info

``` Flags: category=core severity=medium Site configuration information for perl v5.6.1: Configured by root at Sat Dec 29 11:54:59 EST 2001. Summary of my perl5 (revision 5.0 version 6 subversion 1) configuration: Platform: osname=linux, osvers=2.4.2-2, archname=i586-linux uname='linux plover.com 2.4.2-2 #1 sun apr 8 19:37:14 edt 2001 i586 unknown ' config_args='-des' hint=recommended, useposix=true, d_sigaction=define usethreads=undef use5005threads=undef useithreads=undef usemultiplicity=undef useperlio=undef d_sfio=undef uselargefiles=define usesocks=undef use64bitint=undef use64bitall=undef uselongdouble=undef Compiler: cc='cc', ccflags ='-fno-strict-aliasing -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64', optimize='-O2', cppflags='-fno-strict-aliasing' ccversion='', gccversion='2.96 20000731 (Red Hat Linux 7.1 2.96-81)', 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, usemymalloc=n, prototype=define Linker and Libraries: ld='cc', ldflags =' -L/usr/local/lib' libpth=/usr/local/lib /lib /usr/lib libs=-lnsl -lndbm -lgdbm -ldb -ldl -lm -lc -lcrypt -lutil perllibs=-lnsl -ldl -lm -lc -lcrypt -lutil libc=/lib/libc-2.2.2.so, so=so, useshrplib=false, libperl=libperl.a Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-rdynamic' cccdlflags='-fpic', lddlflags='-shared -L/usr/local/lib' Locally applied patches: @INC for perl v5.6.1: /usr/local/lib/perl5/5.6.1/i586-linux /usr/local/lib/perl5/5.6.1 /usr/local/lib/perl5/site_perl/5.6.1/i586-linux /usr/local/lib/perl5/site_perl/5.6.1 /usr/local/lib/perl5/site_perl/5.6.0/i586-linux /usr/local/lib/perl5/site_perl/5.6.0 /usr/local/lib/perl5/site_perl . Environment for perl v5.6.1: HOME=/home/mjd LANG=C LANGUAGE (unset) LD_LIBRARY_PATH=/lib:/usr/lib:/usr/X11R6/lib LOGDIR (unset) PATH=/home/mjd/bin:/usr/local/bin:/bin:/usr/bin:/usr/X11R6/bin:/usr/games:/sbin:/usr/sbin:/usr/local/bin/X11R6:/usr/local/bin/mh:/data/mysql/bin:/usr/local/bin/pbm:/usr/local/bin/ezmlm:/home/mjd/TPI/bin:/usr/local/teTeX/bin:/usr/local/mysql/bin PERL_BADLANG (unset) SHELL=/bin/bash ```

p5pRT commented 22 years ago

From @mjdominus

[Please enter your report here]
    $^W = 1;
    sub X&#8203;::TIEARRAY \{
      my $$pack\, %h$ = shift;
      keys %h = 64;  \# removing this line makes the problem go away
      bless \\%h => $pack;
    \}

    my $x = tie @&#8203;x\, 'X';
    \# Devel&#8203;::Peek&#8203;::dump shows that at this point\, %$x has REFCNT = 3
    undef $x;
    untie @&#8203;x;
When run\, this program (incorrectly) emits the warning
    untie attempted while 1 inner references still exist at /tmp/untie\.pl line 11\.
(Line 11 is the 'untie' line.)

Commenting out the assignment to 'keys' at line 4 makes the warning go away; the REFCNT is correct in that case.

The problem persists in 5.7.3 and in this morning's bleadperl.

Should I put a test in for this? Or do you want to deal with it after 5.8 is released?

p5pRT commented 22 years ago

From @jhi

On Tue\, Apr 02\, 2002 at 04:11:09PM -0500\, Mark-Jason Dominus wrote:

[Please enter your report here]
    $^W = 1;
    sub X&#8203;::TIEARRAY \{
      my $$pack\, %h$ = shift;
      keys %h = 64;  \# removing this line makes the problem go away
      bless \\%h => $pack;
    \}

    my $x = tie @&#8203;x\, 'X';
    \# Devel&#8203;::Peek&#8203;::dump shows that at this point\, %$x has REFCNT = 3
    undef $x;
    untie @&#8203;x;
When run\, this program (incorrectly) emits the warning
    untie attempted while 1 inner references still exist at /tmp/untie\.pl line 11\.
(Line 11 is the 'untie' line.)

Commenting out the assignment to 'keys' at line 4 makes the warning go away; the REFCNT is correct in that case.

The problem persists in 5.7.3 and in this morning's bleadperl.
Should I put a test in for this? Or do you want to deal with it after 5.8 is released?

More $TODO tiearray.t?

-- $jhi++; # http://www.iki.fi/jhi/ # There is this special biologist word we use for 'stable'. # It is 'dead'. -- Jack Cohen

p5pRT commented 22 years ago

From @mjdominus

It turns out that following lvalue 'keys'\, the REFCNT is wrong on hashes even when they are not tied or objects. For example:

sub D::new { bless [] => 'D' } sub D::DESTROY { print "Destroying $_[0]\n" }

{ my %h = (0 => D->new); # keys %h = 64;
} print "%h and D should have been destroyed by now.\n";

END { print "Start of global destruction phase\n" }

With 5.7.3\, the output here is

Destroying D=ARRAY(0x80f7b0c) %h and D should have been destroyed by now. Start of global destruction phase

as you would expect. But if you uncomment the 'keys' assignment\, you get different results:

%h and D should have been destroyed by now. Start of global destruction phase Destroying D=ARRAY(0x80f7b0c)

%h is not cleaned up at the end of the block as it should be; instead\, it lives on until the interpreter thread exits and performs a global destruction.

The reason is simple: Doing an lvalue 'keys' on a hash increments the reference count on the hash. Why? I don't know. I took out the REFCNT_inc:

--- doop.c 2002/04/03 17:22:42 1.1 +++ doop.c 2002/04/03 17:33:33 @@ -1314\,7 +1314\,7 @@ if (LvTARG(TARG) != (SV*)keys) { if (LvTARG(TARG)) SvREFCNT_dec(LvTARG(TARG)); - LvTARG(TARG) = SvREFCNT_inc(keys); + LvTARG(TARG) = keys; } PUSHs(TARG); RETURN;

and now the bug is gone and all the tests still pass.

But I'm worried. Clearly someone put that SvREFCNT_inc() in for a reason\, and maybe it's a reason I don't understand. I couldn't find the patch that did this; I only have patches back to @9671. The change was introduced sometime between 5.004_04 and 5.005_02. Can anyone think of what it's doing?

If not\, I suggest that it be removed; as I mentioned\, all the other tests still pass. Patch below.

--- t/op/ref.t 2002/04/03 17:50:46 1.1 +++ t/op/ref.t 2002/04/03 17:56:44 @@ -5\,7 +5\,7 @@ @INC = qw(. ../lib); }

-print "1..62\n"; +print "1..63\n";

require 'test.pl';

@@ -315\,9 +315\,19 @@ print "# expected \"$expect\"\, got \"$result\"\n"; }

+# test bug in lvalue 'keys' which incorrectly incremented the refcount +# in the hash [ID 20020402.001] REFCNT wrong on tied hash object +# following assignment to keys() +# 20020403 mjd-perl-patch+@plover.com +sub mjd::DESTROY {$mjd::ok = 1} +{ my %h = (0 => (bless [] => 'mjd')); + keys %h = 64; +} +print $mjd::ok ? "ok 60\n" : "not ok 60\n"; + # test global destruction

-my $test = 60; +my $test = 61; my $test1 = $test + 1; my $test2 = $test + 2;

--- doop.c 2002/04/03 17:22:42 1.1 +++ doop.c 2002/04/03 17:33:33 @@ -1314\,7 +1314\,7 @@ if (LvTARG(TARG) != (SV*)keys) { if (LvTARG(TARG)) SvREFCNT_dec(LvTARG(TARG)); - LvTARG(TARG) = SvREFCNT_inc(keys); + LvTARG(TARG) = keys; } PUSHs(TARG); RETURN;

p5pRT commented 22 years ago

From @schwern

On Wed\, Apr 03\, 2002 at 12:59:55PM -0500\, Mark-Jason Dominus wrote:

But I'm worried. Clearly someone put that SvREFCNT_inc() in for a reason\, and maybe it's a reason I don't understand. I couldn't find the patch that did this; I only have patches back to @9671. The change was introduced sometime between 5.004_04 and 5.005_02. Can anyone think of what it's doing?

http://www.rosat.mpe-garching.mpg.de/mailing-lists/perl5-porters/1998-04/msg00035.html

--

Michael G. Schwern \schwern@pobox\.com http://www.pobox.com/~schwern/ Perl Quality Assurance \perl\-qa@perl\.org Kwalitee Is Job One Now if you'll excuse me\, I've got about a hundred hours of memories to erase!! -- http://www.angryflower.com/allrigh.gif

p5pRT commented 22 years ago

From @mjdominus

http://www.rosat.mpe-garching.mpg.de/mailing-lists/perl5-porters/1998-04/msg00035.html

Thanks. With Sarathy's help\, I was able to find an example where my patch failed. I'm now working on a different approach.

p5pRT commented 18 years ago

From @smpeters

[RT_System - Wed Apr 03 02:00:26 2002]:

It turns out that following lvalue 'keys'\, the REFCNT is wrong on hashes even when they are not tied or objects. For example:
    sub D&#8203;::new \{ bless \[\] => 'D' \}
    sub D&#8203;::DESTROY \{ print "Destroying $\_\[0\]\\n" \}

    \{
            my %h = $0 => D\->new$;
    \#       keys %h = 64;         
    \}
    print "%h and D should have been destroyed by now\.\\n";

    END \{ print "Start of global destruction phase\\n" \}
With 5.7.3\, the output here is
    Destroying D=ARRAY$0x80f7b0c$
    %h and D should have been destroyed by now\.
    Start of global destruction phase
as you would expect. But if you uncomment the 'keys' assignment\, you get different results:
    %h and D should have been destroyed by now\.
    Start of global destruction phase
    Destroying D=ARRAY$0x80f7b0c$
%h is not cleaned up at the end of the block as it should be; instead\, it lives on until the interpreter thread exits and performs a global destruction.

The reason is simple: Doing an lvalue 'keys' on a hash increments the reference count on the hash. Why? I don't know. I took out the REFCNT_inc:
\-\-\- doop\.c    2002/04/03 17&#8203;:22&#8203;:42    1\.1
\+\+\+ doop\.c    2002/04/03 17&#8203;:33&#8203;:33
@&#8203;@&#8203; \-1314\,7 \+1314\,7 @&#8203;@&#8203;
            if $LvTARG\(TARG$ \!= $SV\*$keys\) \{
                if $LvTARG\(TARG$\)
                    SvREFCNT\_dec$LvTARG\(TARG$\);
\-        LvTARG$TARG$ = SvREFCNT\_inc$keys$;
\+        LvTARG$TARG$ = keys;
            \}
            PUSHs$TARG$;
            RETURN;
and now the bug is gone and all the tests still pass.

But I'm worried. Clearly someone put that SvREFCNT_inc() in for a reason\, and maybe it's a reason I don't understand. I couldn't find the patch that did this; I only have patches back to @9671. The change was introduced sometime between 5.004_04 and 5.005_02. Can anyone think of what it's doing?

If not\, I suggest that it be removed; as I mentioned\, all the other tests still pass. Patch below.

--- t/op/ref.t 2002/04/03 17:50:46 1.1 +++ t/op/ref.t 2002/04/03 17:56:44 @@ -5\,7 +5\,7 @@ @INC = qw(. ../lib); }

-print "1..62\n"; +print "1..63\n";

require 'test.pl';

@@ -315\,9 +315\,19 @@ print "# expected \"$expect\"\, got \"$result\"\n"; }

+# test bug in lvalue 'keys' which incorrectly incremented the refcount +# in the hash [ID 20020402.001] REFCNT wrong on tied hash object +# following assignment to keys() +# 20020403 mjd-perl-patch+@plover.com +sub mjd::DESTROY {$mjd::ok = 1} +{ my %h = (0 => (bless [] => 'mjd')); + keys %h = 64; +} +print $mjd::ok ? "ok 60\n" : "not ok 60\n"; + # test global destruction

-my $test = 60; +my $test = 61; my $test1 = $test + 1; my $test2 = $test + 2;

--- doop.c 2002/04/03 17:22:42 1.1 +++ doop.c 2002/04/03 17:33:33 @@ -1314\,7 +1314\,7 @@ if (LvTARG(TARG) != (SV*)keys) { if (LvTARG(TARG)) SvREFCNT_dec(LvTARG(TARG)); - LvTARG(TARG) = SvREFCNT_inc(keys); + LvTARG(TARG) = keys; } PUSHs(TARG); RETURN;

Following up later\, you wrote...

http://www.rosat.mpe-garching.mpg.de/mailing-lists/perl5-porters/1998-04/msg00035.html

Thanks. With Sarathy's help\, I was able to find an example where my patch failed. I'm now working on a different approach. OK\, this is way after the fact\, but do you know if somewhere you might please have an example where your patch failed? Adding this as a test case would help anyone who decided to follow up on this problem.

I know this is *way* after the fact\, but do you have examples of where your patch failed. These would be very helpful to someone wanting to take up where this ticket was left off.

p5pRT commented 16 years ago

From p5p@spam.wizbit.be

On Wed Apr 03 03:49:52 2002\, RT_System wrote:

On Wed\, Apr 03\, 2002 at 12:59:55PM -0500\, Mark-Jason Dominus wrote:

But I'm worried. Clearly someone put that SvREFCNT_inc() in for a reason\, and maybe it's a reason I don't understand. I couldn't find the patch that did this; I only have patches back to @9671. The change was introduced sometime between 5.004_04 and 5.005_02. Can anyone think of what it's doing?

http://www.rosat.mpe-garching.mpg.de/mailing-lists/perl5-porters/ 1998- 04/msg00035.html

Dead url.

http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/1998-04/ msg00035.html

p5pRT commented 14 years ago

From @cpansprout

This has been fixed\, probably by 0607bed5b8805b56401c29fc8c6d0f3737d5353f.

p5pRT commented 14 years ago

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

Perl / perl5