B::PADOP->gv should not allow fetching values from a pad that's not the op's #10277
Comments
From @raflCreated by rafl@perldition.orgCommit f746176 broke Devel-Caller, at least on some perls. The tests segfault while calling B::PADOP->gv, which is trying to The code in question is this define in B.xs: #define PADOP_gv(o) ((o->op_padix \ PAD_SVl returns NULL, causing a segfault in SvTYPE. I suspect the GV Unfortunately I'm unable to to come up with a test case that involves Perl Info |
From @rafl0001-Make-sure-PADOP_gv-checks-that-the-pad-actually-cont.patchFrom 8a5fca807c0394d03ed7cb9268cc4a5564996868 Mon Sep 17 00:00:00 2001
From: Florian Ragwitz <rafl@debian.org>
Date: Sun, 4 Apr 2010 04:36:11 +0200
Subject: [PATCH] Make sure PADOP_gv checks that the pad actually contains a scalar before dereferencing it as such.
Since commit f7461760 the pads might contain NULL SVs for GVs that have been
downgraded.
---
ext/B/B.xs | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/ext/B/B.xs b/ext/B/B.xs
index 2b6fb8d..3586e76 100644
--- a/ext/B/B.xs
+++ b/ext/B/B.xs
@@ -1135,6 +1135,7 @@ SVOP_gv(o)
#define PADOP_padix(o) o->op_padix
#define PADOP_sv(o) (o->op_padix ? PAD_SVl(o->op_padix) : Nullsv)
#define PADOP_gv(o) ((o->op_padix \
+ && PAD_SVl(o->op_padix) \
&& SvTYPE(PAD_SVl(o->op_padix)) == SVt_PVGV) \
? (GV*)PAD_SVl(o->op_padix) : (GV *)NULL)
--
1.7.0.3
|
From @avarOn Sun, Apr 4, 2010 at 03:01, Florian Ragwitz <perlbug-followup@perl.org> wrote:
I've tested this both on the current blead and blead + rafl's patch. ok 62 - package called_assign(@T::flange) Program received signal SIGSEGV, Segmentation fault. But with rafl's patch all tests are OK: ok 65 - package called_assign(%T::quux, %T::bar) Devel::Caller is depended on by a lot of CPAN[1], in particular I'd say this warrants a RC4. 1. http://deps.cpantesters.org/depended-on-by.pl?dist=Devel-Caller-2.04 |
|
The RT System itself - Status changed from 'new' to 'open' |
From @nwc10On Sat, Apr 03, 2010 at 08:01:25PM -0700, Florian Ragwitz wrote:
Vincent is right - this smells bad. 0x00002ac24582620a in XS_B__PADOP_gv (my_perl=0xa0b010, cv=0xca25e8) Right. So the value at that offset *is* NULL. (gdb) p my_perl->Icomppad But that array only has 218 elements, offsets 0 to 217. So 258 is way out of Here's what the neighbouring non-NULL entries (that far out of range) look (gdb) call Perl_sv_dump(my_perl, my_perl->Icurpad[257]) Program received signal SIGSEGV, Segmentation fault. Ooopsy. I think, therefore, that the patch (unfortunately) is papering over symptoms, Nicholas Clark |
From @nwc10On Sun, Apr 04, 2010 at 10:15:24PM +0100, Nicholas Clark wrote:
This is a different run: (gdb) call Perl_get_cv(my_perl, "Devel::Caller::called_with", 0) Note that at the time of the call the current pad, as held in PL_comppad and If I'm right, I don't know how this ever worked. (Specifically, B::PADOP::sv and B::PADOP::gv, except when the current pad Nicholas Clark |
From @nwc10On Mon, Apr 05, 2010 at 08:57:02AM +0100, Nicholas Clark wrote:
Pure chance.
If I patch B.xs like this: Inline Patchdiff --git a/ext/B/B.xs b/ext/B/B.xs
index 2b6fb8d..3c05970 100644
--- a/ext/B/B.xs
+++ b/ext/B/B.xs
@@ -1151,6 +1151,14 @@ PADOP_sv(o)
B::GV
PADOP_gv(o)
B::PADOP o
+ CODE:
+ if (PADOP_padix(o) > AvFILL(PL_comppad)) {
+ PerlIO_printf(PerlIO_stderr(), "%d > %d\n",
+ (int)PADOP_padix(o), (int) AvFILL(PL_comppad));
+ }
+ RETVAL = PADOP_gv(o);
+ OUTPUT:
+ RETVAL
MODULE = B PACKAGE = B::PVOP PREFIX = PVOP_
ok 56 - package called_assign($T::bar) whereas this is what a run with 5.12.0-RC3 looks like ok 56 - package called_assign($T::bar) So there always was a problem. If you look at the actual code in Devel::Caller: my $consider = ($op->name eq "gvsv") ? $op : $prev; Note that as we're dealing with a PADOP, all it uses the return of ->gv for So that code will work as long as the (out of bounds) memory address that #define PADOP_gv(o) ((o->op_padix \ So how come B::Concise works? elsif ($h{class} eq "SVOP" or $h{class} eq "PADOP") { Because it do *doesn't use* the B::PADOP::gv in anger. It too does the lookup So, the simplest work around seems to be to proffer Richard a patch for The correct fix is to refactor B.xs so that its objects for OPs (or at least Nicholas Clark |
From @raflOn Sun, Apr 04, 2010 at 02:15:59PM -0700, Nicholas Clark via RT wrote:
It's an OP_GV, so it's a valid PADOP for threaded perls. The opcode is I still have no idea what's causing it to disappear later. Initially I Could it be related to the slightly changed order of op construction -- |
From @raflOn Mon, Apr 05, 2010 at 01:52:55AM -0700, Nicholas Clark via RT wrote:
You're absolutely right. The patch is at Richard has been notified. Thanks! -- |
From zefram@fysh.orgIt is impossible for B to determine which pad an op refers to, and -zefram |
|
@cpansprout - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#74040 (status was 'resolved')
Searchable as RT74040$
The text was updated successfully, but these errors were encountered: