Corrupt cx stack in pp_caller threaded #12131
Comments
From @rurbanThis is a bug report for perl from rurban@cpanel.net, AddressSanitizer detected a corrupt cx context in pp_caller. e.g. with ./miniperl -w -Ilib -MExporter -e '<?>' READ of size 1 at 0x7fa73ab95388 thread T0 Note: "main" with len 9 is wrong #1 0x169920f in Perl_pp_caller Note: cx already corrupt here. #2 0xd0876e in Perl_runops_debug I added the following assertions to narrow it down: Inline Patch--- ../blead/perl-git/pp_ctl.c 2012-04-27 08:58:31.962299840 -0500
+++ pp_ctl.c 2012-05-23 10:40:42.009392113 -0500
@@ -1897,6 +1897,9 @@
RETURN;
}
+ DEBUG_CX("CALLER");
+ assert(CopSTASHPV(cx->blk_oldcop));
+ assert(SvOOK((HV*)CopSTASHPV(cx->blk_oldcop)));
stash_hek = HvNAME_HEK((HV*)CopSTASH(cx->blk_oldcop));
if (GIMME != G_ARRAY) {
EXTEND(SP, 1);
Without the assert only Carp in make -c cpan/Archive-Extract/ caused the Flags: Site configuration information for perl 5.16.0: Configured by rurban at Mon May 21 12:07:22 CDT 2012. Summary of my perl5 (revision 5 version 16 subversion 0) configuration: Locally applied patches: @INC for perl 5.16.0: Environment for perl 5.16.0: |
From @rurbanThere was an obvious mistake in the pp_caller patch: This patch was more interesting: Inline Patch--- ../blead/perl-git/gv.c 2012-04-27 08:58:31.934300119 -0500
+++ gv.c 2012-05-23 18:11:19.141188997 -0500
@@ -1332,6 +1332,11 @@
tmpbuf = smallbuf;
else
Newx(tmpbuf, tmplen, char);
+ DEBUG_v(PerlIO_printf(Perl_debug_log, "gv_stashpvn
Which led to the wrong main namelen culprit. ../../miniperl -I../../lib Makefile.PL INSTALLDIRS=perl => heap overflow in gv_stashpvn, reading past name. So cx does not seem to be corrupt at all, it was only corrupt in |
From [Unknown Contact. See original ticket]There was an obvious mistake in the pp_caller patch: This patch was more interesting: Inline Patch--- ../blead/perl-git/gv.c 2012-04-27 08:58:31.934300119 -0500
+++ gv.c 2012-05-23 18:11:19.141188997 -0500
@@ -1332,6 +1332,11 @@
tmpbuf = smallbuf;
else
Newx(tmpbuf, tmplen, char);
+ DEBUG_v(PerlIO_printf(Perl_debug_log, "gv_stashpvn
Which led to the wrong main namelen culprit. ../../miniperl -I../../lib Makefile.PL INSTALLDIRS=perl => heap overflow in gv_stashpvn, reading past name. So cx does not seem to be corrupt at all, it was only corrupt in |
From @rurbanNow this is something for Dave Mitchell On Wed, May 23, 2012 at 6:30 PM, Reini Urban via RT
I finally found the culprit who writes the wrong cx->blk_oldcop->cop_stashlen. I used this gdb trick: Perl_sv_compile_2op_is_broken() does at line 3354 a LEAVE_with_name("eval"), SAVECOPSTASH does SAVEI32(CopSTASH_len(c)) though, but I found no call How to repro: (gdb) bt The missing cop_stashlen write: Old value = 0x9c7340 "\360É" -- |
From @iabynOn Tue, May 29, 2012 at 01:05:59PM -0500, Reini Urban wrote:
In my re_eval branch, Perl_sv_compile_2op_is_broken() and the way it does Is it likely that what you're seeing is related to the bad code around -- |
From @cpansproutOn Tue May 29 12:10:26 2012, davem wrote:
I think this is a regression in 5.16.0, due to one of my fumbling commits. So fixing it for backport to 5.16.1 would be good. Please don’t merge -- Father Chrysostomos |
|
The RT System itself - Status changed from 'new' to 'open' |
From @rurbanOn Tue, May 29, 2012 at 2:36 PM, Father Chrysostomos via RT
Found the proper patch finally (see attachment), for 5.16.1 and blead. This patch is independent of sv_compile_2op_is_broken and dave's branch. |
From @rurban0001-perl-113060-Save-cop_stashlen-threaded-even-with-sha.patchFrom 515d7310f01ebe2ca3af7bc3186a8a7504be12cd Mon Sep 17 00:00:00 2001
From: Reini Urban <[email protected]>
Date: Tue, 29 May 2012 15:46:13 -0500
Subject: [PATCH] [perl #113060] Save cop_stashlen threaded even with shared
cop pv
Perl_sv_compile_2op_is_broken() does at line 3354 a LEAVE_with_name("eval"),
a SSPOPSTR via SAVEt_SHARED_PVREF for the localized cop_stashpv, but
not for the cop_stashlen.
The cop in question is PL_compiling, which was "AutoSplit" before with
len=9 and restores it back to "main" but keeps len 9. Thus leading to a
heap-overflow in gv_stashpvn.
---
scope.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scope.h b/scope.h
index aa04a79..38da244 100644
--- a/scope.h
+++ b/scope.h
@@ -237,7 +237,8 @@ scope has the given name. Name must be a literal string.
#ifdef USE_ITHREADS
# define SAVECOPSTASH(c) (SAVEPPTR(CopSTASHPV(c)), \
SAVEI32(CopSTASH_len(c)))
-# define SAVECOPSTASH_FREE(c) SAVESHAREDPV(CopSTASHPV(c))
+# define SAVECOPSTASH_FREE(c) (SAVESHAREDPV(CopSTASHPV(c)), \
+ SAVEI32(CopSTASH_len(c)))
# define SAVECOPFILE(c) SAVEPPTR(CopFILE(c))
# define SAVECOPFILE_FREE(c) SAVESHAREDPV(CopFILE(c))
#else
--
1.7.10
|
From @rurbanThe attached 2nd patch also adds some useful assertions I needed while |
From @rurban0002-related-to-perl-113060-assert-CopSTASH-cx-blk_oldcop.patchFrom 14be56261fc9be8a1ef3b5dff74fce98a0b747b4 Mon Sep 17 00:00:00 2001
From: Reini Urban <[email protected]>
Date: Tue, 29 May 2012 16:06:00 -0500
Subject: [PATCH] related to [perl #113060] assert CopSTASH(cx->blk_oldcop)
HvNAME_HEK((HV*)CopSTASH(cx->blk_oldcop)) does some implicit assumptions, which should
better be asserted. Also record CX pp_caller reads besides PUSH and POP.
---
pp_ctl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/pp_ctl.c b/pp_ctl.c
index 4e54bd3..34fe990 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -1897,6 +1897,9 @@ PP(pp_caller)
RETURN;
}
+ DEBUG_CX("CALLER");
+ assert(CopSTASHPV(cx->blk_oldcop));
+ assert(SvOOK((HV*)CopSTASH(cx->blk_oldcop)));
stash_hek = HvNAME_HEK((HV*)CopSTASH(cx->blk_oldcop));
if (GIMME != G_ARRAY) {
EXTEND(SP, 1);
--
1.7.10
|
From @cpansproutOn Tue May 29 13:53:41 2012, rurban wrote:
Thank you. I’ve applied this as 014243a and your second patch as -- Father Chrysostomos |
|
@cpansprout - Status changed from 'open' to 'resolved' |
From @cpansproutOn Tue May 29 13:53:41 2012, rurban wrote:
I’ve been thinking about moving CopSTASH into the pad for threaded One thing that concerns me, though, is PL_compiling. Would it be Another difficulty is SAVECOPSTASH(_FREE). Currently only newCONSTSUB Dave, can you merge your re_eval branch asap? :-) -- Father Chrysostomos |
From @rurbanOn Fri, Jun 1, 2012 at 1:12 AM, Father Chrysostomos via RT
I was thinking something else. Since stashes are now always stored shared as hek, we can just use the I"ll investigate. |
From @cpansproutOn Mon Jun 04 16:21:22 2012, rurban wrote:
With commit d4d0394 I’ve changed the way CopSTASH is stored under threads. I’ve just realised I stopped B::COP::stashlen from working. But what you say suggests that it is not needed. B::COP::stashpv doesn’t currently work with UTF8 and embedded nulls, but -- Father Chrysostomos |
From @cpansproutOn Mon Jun 04 20:32:06 2012, sprout wrote:
Which has now happened with commit 9922583. -- Father Chrysostomos |
Migrated from rt.perl.org#113060 (status was 'resolved')
Searchable as RT113060$
The text was updated successfully, but these errors were encountered: