Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refcounting bug with reused implicit lexicals as nested loop iterators #9440

Closed
p5pRT opened this issue Aug 3, 2008 · 7 comments
Closed

Comments

@p5pRT
Copy link

p5pRT commented Aug 3, 2008

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

Searchable as RT57564$

@p5pRT
Copy link
Author

p5pRT commented Aug 3, 2008

From fish-perlbug@uc.org

This is a bug report for perl from fish-perlbug@​uc.org,
generated with the help of perlbug 1.35 running under perl 5.10.0.

Re-using the same implicitly lexical iterator variable in nested loops
decrements recount on wrong aliased target.

Terrible, terrible coding style, I know, but still, a bug.

Self-contained example​:

--start--

my @​list1 = ( qw{ foo } );
my @​list2 = ( qw{ bar } );
my $i;

for $i ( @​list1 ) {
  for $i ( @​list2 ) {
  }
}

--cut--

Result​:
Attempt to free unreferenced scalar​: SV 0x1233ad8, Perl interpreter​: 0x1220240.

This happens consistently on perl 5.10 but not on 5.8.* nor does it appear to
happen in blead.

It is specific to the nesting of the foreach-style loops with implicit lexical
loop variables with the same name. What appears to be happening here is as
follows​:

The entry of the first loop aliases $i to the first SV of the first list. This
increases the refcount of that item. Similarly the second loop entry aliases
the inner $i to the first SV of the second list.

However, on exit of the inner list, the refcount of the first item of the first
list is decremented, rather than the correct one. Thus, the "foo" SV is
decremented twice and freed, but the "bar" is not decremented and thus leaks​:

--start--

use Devel​::Peek;

my @​list1 = ( qw{ foo } );
my @​list2 = ( qw{ foo } );
my $i;

for $i ( @​list1 ) {
  for $i ( @​list2 ) {
  }
}

print Dump \@​list1;
print Dump \@​list2;

--cut--

SV = RV(0x1233938) at 0x1233928
  REFCNT = 1
  FLAGS = (TEMP,ROK)
  RV = 0x125f650
  SV = PVAV(0x1235300) at 0x125f650
  REFCNT = 2
  FLAGS = (PADMY)
  ARRAY = 0x1270490
  FILL = 0
  MAX = 3
  ARYLEN = 0x0
  FLAGS = (REAL)
  Elt No. 0
  SV = PV(0x1231040) at 0x1233ad8
  REFCNT = 1 # refcount is 1, but SVPV has simply been reused
  FLAGS = (POK,pPOK)
  PV = 0x12e8040 "(0x1233938) at 0x1233928\n REFCNT = 1\n FLAGS = (TEMP,ROK)"\0
  CUR = 58
  LEN = 64
SV = RV(0x1233938) at 0x1233928
  REFCNT = 1
  FLAGS = (TEMP,ROK)
  RV = 0x125f590
  SV = PVAV(0x1235318) at 0x125f590
  REFCNT = 2
  FLAGS = (PADMY)
  ARRAY = 0x123a9b0
  FILL = 0
  MAX = 3
  ARYLEN = 0x0
  FLAGS = (REAL)
  Elt No. 0
  SV = PV(0x12310a8) at 0x1233ce8
  REFCNT = 2 # ref count too high
  FLAGS = (POK,pPOK)
  PV = 0x12327f0 "foo"\0
  CUR = 3
  LEN = 4

Comparing that with bleadperl​:

SV = IV(0x1423928) at 0x1423928
  REFCNT = 1
  FLAGS = (TEMP,ROK)
  RV = 0x144f608
  SV = PVAV(0x14252e8) at 0x144f608
  REFCNT = 2
  FLAGS = (PADMY)
  ARRAY = 0x142a9a0
  FILL = 0
  MAX = 3
  ARYLEN = 0x0
  FLAGS = (REAL)
  Elt No. 0
  SV = PV(0x1421048) at 0x1423ad8
  REFCNT = 1
  FLAGS = (POK,pPOK)
  PV = 0x142202c "foo"\0
  CUR = 3
  LEN = 4
SV = IV(0x1423928) at 0x1423928
  REFCNT = 1
  FLAGS = (TEMP,ROK)
  RV = 0x144f590
  SV = PVAV(0x1425300) at 0x144f590
  REFCNT = 2
  FLAGS = (PADMY)
  ARRAY = 0x142a830
  FILL = 0
  MAX = 3
  ARYLEN = 0x0
  FLAGS = (REAL)
  Elt No. 0
  SV = PV(0x1421090) at 0x1423c88
  REFCNT = 1
  FLAGS = (POK,pPOK)
  PV = 0x14226d4 "foo"\0
  CUR = 3
  LEN = 4

Dumping \$i after the exit of the inner loop, the refcount of the SV is 2,
compared with 3 if I explicitly lexicalize the inner loop variable.

If the $i is a package global in the declaration, this issue does not manifest.
And similarly, if the lexicals have different names, or explicit mys, there is
no issue.

Not sure if this is a known issue - it doesn't seem to happen in bleadperl, but
I don't see a resolved bug that looks like this, and I suspect the issue is
somewhere in cop.h, which is very different in blead than in 5.10.

The effect of this seems to vary a fair bit. In the case I was seeing this in
a production system, the outer loop array was simply being emptied without
error, leading to surprizing action at a distance.

Not cygwin specific, seen on linux as well.


Flags​:
  category=core
  severity=medium


This perlbug was built using Perl 5.10.0 - Tue Mar 18 00​:26​:31 EDT 2008
It is being executed now by Perl 5.10.0 - Mon Jun 30 16​:03​:19 GMT 2008.

Site configuration information for perl 5.10.0​:

Configured by rurban at Mon Jun 30 16​:03​:19 GMT 2008.

Summary of my perl5 (revision 5 version 10 subversion 0 patch 34065) configuration​:
  Platform​:
  osname=cygwin, osvers=1.5.25(0.15642), archname=cygwin-thread-multi-64int
  uname='cygwin_nt-5.1 reini 1.5.25(0.15642) 2008-06-12 19​:34 i686 cygwin '
  config_args='-de -Dmksymlinks -Dusethreads -Dmad=y -Dusedevel'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=define, usemultiplicity=define
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=define, use64bitall=undef, uselongdouble=undef
  usemymalloc=y, bincompat5005=undef
  Compiler​:
  cc='gcc', ccflags ='-DPERL_USE_SAFE_PUTENV -U__STRICT_ANSI__ -fno-strict-aliasing -pipe -I/usr/local/include',
  optimize='-O3',
  cppflags='-DPERL_USE_SAFE_PUTENV -U__STRICT_ANSI__ -fno-strict-aliasing -pipe -I/usr/local/include'
  ccversion='', gccversion='3.4.4 (cygming special, gdc 0.12, using dmd 0.125)', gccosandvers=''
  intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
  ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='g++', ldflags =' -Wl,--enable-auto-import -Wl,--export-all-symbols -Wl,--stack,8388608 -Wl,--enable-auto-image-base -L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib /lib
  libs=-lgdbm -ldb -ldl -lcrypt -lgdbm_compat
  perllibs=-ldl -lcrypt
  libc=/usr/lib/libc.a, so=dll, useshrplib=true, libperl=libperl.a
  gnulibc_version=''
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
  cccdlflags=' ', lddlflags=' --shared -Wl,--enable-auto-import -Wl,--export-all-symbols -Wl,--stack,8388608 -Wl,--enable-auto-image-base -L/usr/local/lib'

Locally applied patches​:
 


@​INC for perl 5.10.0​:
  /usr/lib/perl5/5.10/i686-cygwin
  /usr/lib/perl5/5.10
  /usr/lib/perl5/site_perl/5.10/i686-cygwin
  /usr/lib/perl5/site_perl/5.10
  /usr/lib/perl5/vendor_perl/5.10/i686-cygwin
  /usr/lib/perl5/vendor_perl/5.10
  /usr/lib/perl5/vendor_perl/5.10
  /usr/lib/perl5/site_perl/5.8
  /usr/lib/perl5/vendor_perl/5.8
  .


Environment for perl 5.10.0​:
  HOME=/home/fishy
  LANG (unset)
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/usr/local/bin​:/usr/bin​:/bin​:/usr/X11R6/bin​:/cygdrive/c/WINDOWS/system32​:/cygdrive/c/WINDOWS​:/cygdrive/c/WINDOWS/System32/Wbem​:/cygdrive/c/Program Files/putty​:/cygdrive/c/Program Files/Common Files/Adobe/AGL​:/usr/bin​:/usr/local/bin​:/cygdrive/c/Program Files/Perforce​:/cygdrive/c/PROGRA~1/ATT/Graphviz/bin​:/cygdrive/c/Program Files/ActiveState Perl Dev Kit 7.0/bin​:/cygdrive/c/Program Files/ActiveState Komodo IDE 4.2/​:/usr/bin​:/home/fishy/bin
  PERL_BADLANG (unset)
  SHELL (unset)

@p5pRT
Copy link
Author

p5pRT commented Aug 4, 2008

From p5p@perl.wizbit.be

Citeren "fish-perlbug@​uc.org (via RT)" <perlbug-followup@​perl.org>​:

# New Ticket Created by fish-perlbug@​uc.org
# Please include the string​: [perl #57564]
# in the subject line of all future correspondence about this issue.
# <URL​: http​://rt.perl.org/rt3/Ticket/Display.html?id=57564 >

This is a bug report for perl from fish-perlbug@​uc.org,
generated with the help of perlbug 1.35 running under perl 5.10.0.

Re-using the same implicitly lexical iterator variable in nested loops
decrements recount on wrong aliased target.

Terrible, terrible coding style, I know, but still, a bug.

Self-contained example​:

--start--

my @​list1 = ( qw{ foo } );
my @​list2 = ( qw{ bar } );
my $i;

for $i ( @​list1 ) {
for $i ( @​list2 ) {
}
}

--cut--

Result​:
Attempt to free unreferenced scalar​: SV 0x1233ad8, Perl interpreter​:
0x1220240.

Thanks for the bug report and the small test.

Binary search to see which Change broke it​:

----Program----
#!/usr/bin/perl

my $out = qx/$^X test_57564.pl 2>&1/;
if ($out =~ m/^\s*$/) {
  print "ok\n";
}
else {
  print "not ok\n";
}

----Output of .../pdnRSb4/perl-5.9.2@​26026/bin/perl----
ok

----EOF ($?='0')----
----Output of .../peJubii/perl-5.9.2@​26027/bin/perl----
not ok

----EOF ($?='0')----

http​://public.activestate.com/cgi-bin/perlbrowse/p/26027
Change 26027 by rgs@​bloom on 2005/11/07 09​:58​:26

  Subject​: [perl #24254] Attempt to free unreferenced scalar
  From​: "Chris Heath via RT" <perlbug-followup@​perl.org>
  Date​: Sun, 06 Nov 2005 20​:08​:05 -0800
  Message-ID​: <rt-3.0.11-24254-123984.4.44134155985068@​perl.org>

http​://rt.perl.org/rt3/Public/Bug/Display.html?id=24254

Binary search to see which Change fixed it​:

----Program----
#!/usr/bin/perl

my $out = qx/$^X test_57564.pl 2>&1/;
if ($out =~ m/^\s*$/) {
  print "ok\n";
}
else {
  print "not ok\n";
}

----Output of ...p0B4D4a/perl-5.10.0@​33077/bin/perl----
not ok

----EOF ($?='0')----
----Output of ...pEbKr4g/perl-5.10.0@​33080/bin/perl----
ok

----EOF ($?='0')----

perl-p-5.10.0\@​33079/miniperl test_57564.pl
panic​: leave_scope inconsistency at test_57564.pl line 6.
panic​: leave_scope inconsistency at test_57564.pl line 6.

perl-p-5.10.0\@​33078/miniperl test_57564.pl
panic​: leave_scope inconsistency at test_57564.pl line 6.
panic​: leave_scope inconsistency at test_57564.pl line 6.

So it got fixed with Change 33078 or Change 33079, or Change 33080.

http​://public.activestate.com/cgi-bin/perlbrowse/p/33078
Change 33078 by nicholas@​nicholas-saigo on 2008/01/26 16​:46​:22

  POPLOOP is actually doing all the work of Perl_save_padsv() already!

http​://public.activestate.com/cgi-bin/perlbrowse/p/33079
Change 33079 by nicholas@​nicholas-saigo on 2008/01/26 17​:31​:34

  Change 33072 missed embed.h. I wasn't aware that it was affected by
  opcode.pl. You live and learn (and should run p4 diff -se ...).

http​://public.activestate.com/cgi-bin/perlbrowse/p/33080
Change 33080 by nicholas@​nicholas-bouvard on 2008/01/26 17​:54​:29

  Investigation reveals that the work of restoring the iterator to the
  pad is shared between POPLOOP, using itersave, and the end of scope
  restore action requested by Perl_save_padsv(). In fact, the only user
  of SAVEt_PADSV is pp_enteriter, and it already provides enough
  information to allow it to perform the sv_2mortal() in POPLOOP.
  So make it do so. Rather than creating a new routine, use the existing
  routine because nothing else (at least nothing else known to Google's
  codesearch) uses it. But rename it just in case something we can't see
  is being naughty and using our private functions - they will get
  link errors against 5.12.

  All this means that itersave is now redundant. So remove it.
  This makes struct context 48 bytes on ILP32 platforms with 32bit IVs,
  down from 64 bytes in 5.10. 33% more context stack in the same memory

None of the Changes (the one that broke it and the one that fixed it)
appear to include tests so I'll take a deeper look to see if tests
were added.
That is, reverting the patches and running make test.
If I see no test failures then a patch with tests can be expected.

Kind regards,

Bram

@p5pRT
Copy link
Author

p5pRT commented Aug 4, 2008

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

@p5pRT
Copy link
Author

p5pRT commented Aug 5, 2008

From p5p@perl.wizbit.be

Citeren Bram <p5p@​perl.wizbit.be>​:

None of the Changes (the one that broke it and the one that fixed it)
appear to include tests so I'll take a deeper look to see if tests were
added.
That is, reverting the patches and running make test.
If I see no test failures then a patch with tests can be expected.

Patch with tests attached.
Added the tests in t/op/ref.t (as Merijn suggested).

Will the Changes 33078, 33079, and 33080 be intergrated into 5.10.x?

If not then the last test needs to be marked todo/skip in 5.10.x.
(The problem was created by Change 26027)

Kind regards,

Bram

@p5pRT
Copy link
Author

p5pRT commented Aug 5, 2008

From p5p@perl.wizbit.be

Inline Patch
--- old/t/op/ref.t	2008-08-05 18:48:45.000000000 +0200
+++ new/t/op/ref.t	2008-08-05 19:48:08.000000000 +0200
@@ -8,7 +8,7 @@
 require 'test.pl';
 use strict qw(refs subs);
 
-plan(182);
+plan(189);
 
 # Test glob operations.
 
@@ -584,6 +584,18 @@
 ok (!eval { $pvbm->() }, 'PVBM is not a CODE ref');
 ok (!eval { $rpvbm->foo }, 'PVBM is not an object');
 
+# bug 24254
+is( runperl(stderr => 1, prog => 'map eval qq(exit),1 for 1'), "");
+is( runperl(stderr => 1, prog => 'eval { for (1) { map { die } 2 } };'), "");
+is( runperl(stderr => 1, prog => 'for (125) { map { exit } (213)}'), "");
+is( runperl(stderr => 1, prog => 'map die,4 for 3'), "Died at -e line 1.\n");
+is( runperl(stderr => 1, prog => 'grep die,4 for 3'), "Died at -e line 1.\n");
+is( runperl(stderr => 1, prog => 'for $a (3) {@b=sort {die} 4,5}'), "Died at -e line 1.\n");
+
+# bug 57564
+is( runperl(stderr => 1, prog => 'my $i;for $i (1) { for $i (2) { } }'), "");
+
+
 # Bit of a hack to make test.pl happy. There are 3 more tests after it leaves.
 $test = curr_test();
 curr_test($test + 3);

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2008

From @Tux

On Tue, 05 Aug 2008 19​:58​:00 +0200, Bram <p5p@​perl.wizbit.be> wrote​:

Citeren Bram <p5p@​perl.wizbit.be>​:

None of the Changes (the one that broke it and the one that fixed it)
appear to include tests so I'll take a deeper look to see if tests were
added.
That is, reverting the patches and running make test.
If I see no test failures then a patch with tests can be expected.

Patch with tests attached.
Added the tests in t/op/ref.t (as Merijn suggested).

Will the Changes 33078, 33079, and 33080 be intergrated into 5.10.x?

If not then the last test needs to be marked todo/skip in 5.10.x.
(The problem was created by Change 26027)

Thanks, applied in change #34171

--
H.Merijn Brand Amsterdam Perl Mongers http​://amsterdam.pm.org/
using & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,
11.11, 11.23, and 11.31, SuSE 10.1, 10.2, and 10.3, AIX 5.2, and Cygwin.
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

@p5pRT
Copy link
Author

p5pRT commented Aug 10, 2008

p5p@spam.wizbit.be - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant