Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAttempt to free unreferenced scalar: SV 0x5eb03f8 (memory corruption) #14595
Comments
From @geeknikThere is some memory corruption happening, but not sure if this is a use after free or a double free, I'll await an official explanation of what is happening here. Built v5.21.10 (v5.21.9-259-g88d9f32) with the following command line: ./Configure -des -Dusedevel -DDEBUGGING -Dcc=afl-gcc -Doptimize=-O2\ -g && AFL_HARDEN=1 make -j12 test-prep Bug found with AFL (http://lcamtuf.coredump.cx/afl) Valgrind: GDB: Program received signal SIGABRT, Aborted. Hexdump of the minimized 25-byte test case: System Info: Debian 7, Kernel 3.2.65-1+deb7u1 x86_64, GCC 4.9.2, libc 2.13-38+deb7u8 **NOTE** When I was minimizing the test case from 43-bytes to 25-bytes, glibc started popping off a bunch of messages such as these, figured it might relevant: *** glibc detected *** /home/geeknik/perl/perl: invalid fastbin entry (free): 0x0000000002790330 *** The original test case, test159, causes a SIGSEGV while test 159-min causes a SIGABRT. I've attached both test cases, but did not including any gdb or valgrind output from the original test case. |
From @geeknik |
From @geeknikAffects (v5.21.7 (v5.21.6-602-ge9d2bd8) as well. perl -e 't r sort{(0,*b=0)=~0}0..1' gdb-peda$ file perl Program received signal SIGABRT, Aborted. |
From @iabynOn Tue, Mar 17, 2015 at 03:24:58AM -0700, Brian Carpenter wrote:
on code basically like: @a = sort{ *a=0; 1} 0..1; basically its deleting the *a glob then trying to restore the old value of -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Tue Mar 17 09:50:02 2015, davem wrote:
Like the attached? Tony |
From @tonycoz0001-perl-124097-don-t-let-the-GPs-be-removed-out-from-un.patchFrom 472e56692ec732c054a96072a540725842ba6051 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 26 Nov 2015 15:23:23 +1100
Subject: [perl #124097] don't let the GPs be removed out from under pp_sort
---
pp_sort.c | 6 ++++++
t/op/sort.t | 5 ++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/pp_sort.c b/pp_sort.c
index 64a67d8..f1be44f 100644
--- a/pp_sort.c
+++ b/pp_sort.c
@@ -1649,6 +1649,7 @@ PP(pp_sort)
CATCH_SET(TRUE);
PUSHSTACKi(PERLSI_SORT);
if (!hasargs && !is_xsub) {
+ GV *first_copy, *second_copy;
SAVEGENERICSV(PL_firstgv);
SAVEGENERICSV(PL_secondgv);
PL_firstgv = MUTABLE_GV(SvREFCNT_inc(
@@ -1657,6 +1658,11 @@ PP(pp_sort)
PL_secondgv = MUTABLE_GV(SvREFCNT_inc(
gv_fetchpvs("b", GV_ADD|GV_NOTQUAL, SVt_PV)
));
+ /* make sure the GP isn't removed out from under us */
+ first_copy = (GV*)newSVsv(MUTABLE_SV(PL_firstgv));
+ second_copy = (GV*)newSVsv(MUTABLE_SV(PL_secondgv));
+ SAVEFREESV(first_copy);
+ SAVEFREESV(second_copy);
SAVESPTR(GvSV(PL_firstgv));
SAVESPTR(GvSV(PL_secondgv));
}
diff --git a/t/op/sort.t b/t/op/sort.t
index 3c76365..14a5aed 100644
--- a/t/op/sort.t
+++ b/t/op/sort.t
@@ -7,7 +7,7 @@ BEGIN {
set_up_inc('../lib');
}
use warnings;
-plan(tests => 193);
+plan(tests => 194);
# these shouldn't hang
{
@@ -1127,3 +1127,6 @@ pass "no crash when sort block deletes *a and *b";
::is (join('-', sort f2 3,1,2,4), '1-2-3-4', "Ret: f2");
::is (join('-', sort f3 3,1,2,4), '1-2-3-4', "Ret: f3");
}
+
+@a = sort{ *a=0; 1} 0..1;
+pass "No crash when GP deleted out from under us [perl 124097]";
--
2.1.4
|
From @iabynOn Wed, Nov 25, 2015 at 08:24:22PM -0800, Tony Cook via RT wrote:
Wouldn't save_gp(PL_firstgv, 0) be more efficient? -- |
From @tonycozOn Fri, Nov 27, 2015 at 01:42:30PM +0000, Dave Mitchell wrote:
It would, but it also restores the GP to the GV, and the user has With save_gp: $ ./perl -e 'sub a { print "hello\n" } a(); @x = sort { *a = sub { print "goodbye\n" }; 1 } 0 .. 1; a();' With the patch I supplied: $ ./perl -e 'sub a { print "hello\n" } a(); @x = sort { *a = sub { print "goodbye\n" }; 1 } 0 .. 1; a();' Tony |
From @iabynOn Mon, Nov 30, 2015 at 10:14:31AM +1100, Tony Cook wrote:
I'd expect sort {block} 1, 2 to have the same semantics as: { 'local *a = ...' does a combination of save_gp(gv,0) via rv2gv and SAVEGENERICSV(SvGV(gv)) via pp_sassign and gv_setref, so I'd expect some combination of those to Do The Right Thing. And local does seem to Do The Right Thing: sub a { print "hello\n" } prints: hello So maybe there's a way to achieve this with save_gp? -- |
From @tonycozOn Mon Nov 30 03:42:06 2015, davem wrote:
Do you mean like the attached? I was thinking about how to document GVf_INTRO, something like: "Localize the next GP slot set." and save_gp (part of the API) something like: Saves the current GP of gv on the save stack to be restored on scope exit. If empty is true, replace the GP with a new GP. If empty is false, mark gv with GVf_INTRO so the next reference assigned is localized, which is how C< local *foo = $someref; > works. Tony |
From @tonycoz0001-perl-124097-don-t-let-the-GPs-be-removed-out-from-un.patchFrom 9f5f69448338d796de8e443d5ed74c2614700fa5 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Wed, 2 Dec 2015 14:26:52 +1100
Subject: [perl #124097] don't let the GPs be removed out from under pp_sort
Needs tests for preserving modifications to slots in *a and *b
---
pp_sort.c | 7 +++++++
t/op/sort.t | 5 ++++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/pp_sort.c b/pp_sort.c
index 64a67d8..51742f6 100644
--- a/pp_sort.c
+++ b/pp_sort.c
@@ -1657,6 +1657,13 @@ PP(pp_sort)
PL_secondgv = MUTABLE_GV(SvREFCNT_inc(
gv_fetchpvs("b", GV_ADD|GV_NOTQUAL, SVt_PV)
));
+ /* make sure the GP isn't removed out from under us for
+ * the SAVESPTR() */
+ save_gp(PL_firstgv, 0);
+ save_gp(PL_secondgv, 0);
+ /* we don't want modifications localized */
+ GvINTRO_off(PL_firstgv);
+ GvINTRO_off(PL_secondgv);
SAVESPTR(GvSV(PL_firstgv));
SAVESPTR(GvSV(PL_secondgv));
}
diff --git a/t/op/sort.t b/t/op/sort.t
index 3c76365..14a5aed 100644
--- a/t/op/sort.t
+++ b/t/op/sort.t
@@ -7,7 +7,7 @@ BEGIN {
set_up_inc('../lib');
}
use warnings;
-plan(tests => 193);
+plan(tests => 194);
# these shouldn't hang
{
@@ -1127,3 +1127,6 @@ pass "no crash when sort block deletes *a and *b";
::is (join('-', sort f2 3,1,2,4), '1-2-3-4', "Ret: f2");
::is (join('-', sort f3 3,1,2,4), '1-2-3-4', "Ret: f3");
}
+
+@a = sort{ *a=0; 1} 0..1;
+pass "No crash when GP deleted out from under us [perl 124097]";
--
2.1.4
|
From @iabynOn Tue, Dec 01, 2015 at 08:05:03PM -0800, Tony Cook via RT wrote:
Looks good to me (as far as I understand these things, which isn't far).
How about: A one-shot flag which indicates that the next assignment of a reference
looks ok. -- |
|
@tonycoz - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for submitting this report. You have helped make Perl better. Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0 |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#124097 (status was 'resolved')
Searchable as RT124097$