Bug 626411 - Perl gives the error 'Attempt to free unreferenced scalar' with nested foreach loops using the same variable
Summary: Perl gives the error 'Attempt to free unreferenced scalar' with nested foreac...
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: perl
Version: 12
Hardware: All
OS: Linux
low
low
Target Milestone: ---
Assignee: Petr Pisar
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords: Reopened
Depends On: 625746
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-08-23 13:37 UTC by Petr Pisar
Modified: 2010-10-19 07:14 UTC (History)
11 users (show)

(edit)
Clone Of: 625746
(edit)
Last Closed: 2010-10-19 07:14:33 UTC


Attachments (Terms of Use)
Fix (14.10 KB, patch)
2010-08-24 13:50 UTC, Petr Pisar
no flags Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
CPAN 70660 None None None Never

Description Petr Pisar 2010-08-23 13:37:16 UTC
+++ This bug was initially created as a clone of Bug #625746 +++
--- Additional comment from tao@redhat.com on 2010-08-20 10:36:51 GMT ---

Event posted on 2010-08-19 16:33 BST by spoyarek

Problem Description:

Perl gives the error "Attempt to free unreferenced scalar" with nested foreach loops using the same variable.

How Reproducible:

Always

Steps to Reproduce:

1) Execute the following perl script:

#! /usr/bin/perl
use strict;
my $element;
foreach $element ("a", "b") {
foreach $element ("c", "d") {1;}
}

Actual Result:

Attempt to free unreferenced scalar: SV 0x986b90c, Perl interpreter: 0x986a008.
Attempt to free unreferenced scalar: SV 0x986b918, Perl interpreter: 0x986a008.
----------

The same problem is in perl-5.10.0-91.fc12.x86_64. perl-5.10.1-118.fc13.x86_64 is not affected.

So wee need to bisect 5.10.0..5.10.1.

Comment 1 Petr Pisar 2010-08-23 14:24:31 UTC
Minimal test case as proposed in CPAN bug #70660:

my $_;
for (0) {
  for (0) {}
}

Comment 2 Petr Pisar 2010-08-23 16:03:55 UTC
This commit as first fixed tree has been found by bisecting git repository:

commit b99874c7926b2f5919256cf4bcee8eb7bf3dff22
Author: Nicholas Clark <nick@ccl4.org>
Date:   Sat Jan 26 15:14:25 2008 +0000

    In POPLOOP, if CxITERVAR(cx) is non-NULL, then so is itersave, and
    itersave is a less complex expression for the C compiler.
    
    p4raw-id: //depot/perl@33074

diff --git a/cop.h b/cop.h
index b9ca6c3..48f9d5b 100644
--- a/cop.h
+++ b/cop.h
@@ -514,15 +514,12 @@ struct block_loop {
 	    SvREFCNT_dec(cx->blk_loop.state_u.lazysv.cur);		\
 	    SvREFCNT_dec(cx->blk_loop.state_u.lazysv.end);		\
 	}								\
-	if (CxITERVAR(cx)) {						\
+	if (cx->blk_loop.itersave) {					\
             if (SvPADMY(cx->blk_loop.itersave)) {			\
 		SV ** const s_v_p = CxITERVAR(cx);			\
 		sv_2mortal(*s_v_p);					\
 		*s_v_p = cx->blk_loop.itersave;				\
 	    }								\
-	    else {							\
-		SvREFCNT_dec(cx->blk_loop.itersave);			\
-	    }								\
 	}								\
 	if (CxTYPE(cx) == CXt_LOOP_FOR)					\
 	    SvREFCNT_dec(cx->blk_loop.state_u.ary.ary);

Comment 3 Petr Pisar 2010-08-23 16:41:36 UTC
The second change of the patch was just an accident. Correct patch is:

commit c84a25c741881ec8b62eb5c2c67b2bd4fe4c977c
Author: Nicholas Clark <nick@ccl4.org>
Date:   Thu Oct 16 18:08:52 2008 +0000

    Integrate:
    [ 33074]
    In POPLOOP, if CxITERVAR(cx) is non-NULL, then so is itersave, and
    itersave is a less complex expression for the C compiler.
    
    [ 33075]
    Restore the else block accidently eaten by change 33074.
    p4raw-link: @33075 on //depot/perl: 9a98d8a1ee5dd7f1caa95f2ec5ab0341e0b4f8d1
    p4raw-link: @33074 on //depot/perl: b99874c7926b2f5919256cf4bcee8eb7bf3dff22
    
    p4raw-id: //depot/maint-5.10/perl@34492
    p4raw-integrated: from //depot/perl@33075 'edit in' cop.h (@33074..)

diff --git a/cop.h b/cop.h
index 10f8a04..572d9ec 100644
--- a/cop.h
+++ b/cop.h
@@ -501,7 +501,7 @@ struct block_loop {
 
 #define POPLOOP(cx)							\
 	SvREFCNT_dec(cx->blk_loop.iterlval);				\
-	if (CxITERVAR(cx)) {						\
+	if (cx->blk_loop.itersave) {					\
             if (SvPADMY(cx->blk_loop.itersave)) {			\
 		SV ** const s_v_p = CxITERVAR(cx);			\
 		sv_2mortal(*s_v_p);					\



However it does not fix it. (Bad bisect test?)

Comment 4 Petr Pisar 2010-08-23 16:56:11 UTC
Git log:

* Here it works.

commit e846cb9248fdbc78eb6a883f17c20c3785e7f2de
Author: Nicholas Clark <nick@ccl4.org>
Date:   Sat Jan 26 21:55:51 2008 +0000

    The layout for struct block_loop under ithreads can be simplified.
    Instead of wedging the pad offset into a void* iterdata, and always
    storing PL_comppad even when it isn't used, instead do this:
    
    PAD        *oldcomppad; /* Also used for the GV, if targoffset is 0 */
    /* This is also accesible via cx->blk_loop.my_op->op_targ */
    PADOFFSET  targoffset;
    
    and store the GV pointer in oldcompad. Pointers to pointers seems
    cleaner. This also allows us to eliminate the flag bit CXp_PADVAR.
    
    p4raw-id: //depot/perl@33081

commit 09edbca0f5c7caf9dd4acef80d8e6275e5a95ea1
Author: Nicholas Clark <nick@ccl4.org>
Date:   Sat Jan 26 17:54:29 2008 +0000

    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.
    
    p4raw-id: //depot/perl@33080

commit dceb5f6256f15f796b36d673cf34f57b7feb1127
Author: Nicholas Clark <nick@ccl4.org>
Date:   Sat Jan 26 17:31:34 2008 +0000

    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 ...).
    
    p4raw-id: //depot/perl@33079

commit f32d39a24d025c50d0bd64a7aabe80c983be93ca
Author: Nicholas Clark <nick@ccl4.org>
Date:   Sat Jan 26 16:46:22 2008 +0000

    POPLOOP is actually doing all the work of Perl_save_padsv() already!
    
    p4raw-id: //depot/perl@33078

commit 7fe44985589724dcec9673580abb16118ba49068
Author: Nicholas Clark <nick@ccl4.org>
Date:   Sat Jan 26 16:44:43 2008 +0000

    Standardise the conditional compilation protection of ({}) from
    #if defined(__GNUC__) && !defined(__STRICT_ANSI__) && !defined(PERL_GCC_PEDANTIC)
    to
    #if defined(__GNUC__) && !defined(PERL_GCC_BRACE_GROUPS_FORBIDDEN)
    because the ({}) construction can be used under __STRICT_ANSI__
    (and should be, because it avoids temporary use of PL_Sv).
    
    p4raw-id: //depot/perl@33077

commit d28d780685cbd952928da9c691e2b5bae129457a
Author: Nicholas Clark <nick@ccl4.org>
Date:   Sat Jan 26 16:03:03 2008 +0000

    As itersave points to the initial CxITERVAR(), and the state of
    SvPADMY() does not change over the duration of the scope, we can
    perform conditional actions at loop push time. For the non-pad case,
    a reference to the initial CxITERVAR() is already held on the scope
    stack thanks to SAVEGENERICSV(*svp); in pp_enteriter. So there is no
    need to save another reference to it in itersave - it's not going away.
    
    p4raw-id: //depot/perl@33076

commit 9a98d8a1ee5dd7f1caa95f2ec5ab0341e0b4f8d1
Author: Nicholas Clark <nick@ccl4.org>
Date:   Sat Jan 26 15:17:09 2008 +0000

    Restore the else block accidently eaten by change 33074.
    
    p4raw-id: //depot/perl@33075

* Here it does not work.

commit b99874c7926b2f5919256cf4bcee8eb7bf3dff22
Author: Nicholas Clark <nick@ccl4.org>
Date:   Sat Jan 26 15:14:25 2008 +0000

    In POPLOOP, if CxITERVAR(cx) is non-NULL, then so is itersave, and
    itersave is a less complex expression for the C compiler.

    p4raw-id: //depot/perl@33074

Comment 5 Petr Pisar 2010-08-24 08:56:00 UTC
09edbca0f5c7caf9dd4acef80d8e6275e5a95ea1 commit is first working one.

Comment 6 Petr Pisar 2010-08-24 13:50:36 UTC
Created attachment 440658 [details]
Fix

This is 73072c95a77d4a9e353efb3fdb279b9650e31e8b commit backported from perl-git tree. It should fix the problem.

Comment 7 Fedora Update System 2010-10-13 07:58:09 UTC
perl-5.10.0-96.fc12 has been submitted as an update for Fedora 12.
https://admin.fedoraproject.org/updates/perl-5.10.0-96.fc12

Comment 8 Fedora Update System 2010-10-14 06:23:24 UTC
perl-5.10.0-96.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update perl'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/perl-5.10.0-96.fc12

Comment 9 Fedora Update System 2010-10-19 07:14:13 UTC
perl-5.10.0-96.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.