Re: Data overrun in Perl_magic_get '?' (Was: Access violation in SV.C new_body_inline())
On Wed, May 20, 2009 at 07:23:32PM -0500, Craig A. Berry wrote:
On May 19, 2009, at 8:31 AM, Nicholas Clark wrote:
That code rings a bell. The only thing I can find that I did near it
was:
http://perl5.git.perl.org/perl.git/commit/35f998ddd1e1665f7d0899ae3e50f9262c59d848
However I had a suspicion that I also did something that restricted
the upgrade
to the minimal case.
Maybe you were thinking of this:
http://www.nntp.perl.org/group/perl.perl5.changes/2008/09/msg22279.html
Yes, it was exactly that one. Thanks.
If this is the cause, I'm not sure whether the correct fix is to make
mg_localize generally upgrade the new scalar to the type of the
existing
scalar, or special case it for $?.
Thanks for the analysis. I don't really know the implications of the
alternatives you propose. I assume a general change is more risky and
potentially adds unnecessary processing to hot code. To me the least
risky thing would be to add the following by analogy with what's
already going on in gv.c:
--- mg.c;-0 2009-04-27 02:42:10 -0500
+++ mg.c2009-05-20 19:11:11 -0500
@@ -974,6 +974,7 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg)
{
sv_setiv(sv, (IV)STATUS_CURRENT);
#ifdef COMPLEX_STATUS
+ SvUPGRADE(sv, SVt_PVLV);
LvTARGOFF(sv) = PL_statusvalue;
LvTARGLEN(sv) = PL_statusvalue_vms;
#endif
[end]
Does that make sense?
This does fix the memory error that started this thread. I'm now
starting a complete test run to make sure nothing else goes pear-shaped.
Presumably an upgrade check is also needed at any point that tries to read
the complex status, as right now it could be using LvTARGOFF() or LvTARGLEN()
on a localised $? that is only PVMG, not PVLV.
Nicholas Clark
Re: Data overrun in Perl_magic_get '?' (Was: Access violation in SV.C new_body_inline())
On May 21, 2009, at 1:01 AM, Nicholas Clark wrote: Presumably an upgrade check is also needed at any point that tries to read the complex status, as right now it could be using LvTARGOFF() or LvTARGLEN() on a localised $? that is only PVMG, not PVLV. Thanks. I only found one such place (Perl_magic_set) and have committed: http://perl5.git.perl.org/perl.git/commitdiff/41cb7b2b Whether there is something more general that ought to be done to lvalues is well beyond my skill level. It took me a long time just to sort out that the L in PVLV stands for lvalue, not lexical, local, long, or any of the other l-words that came to mind first. Craig A. Berry mailto:craigbe...@mac.com ... getting out of a sonnet is much more difficult than getting in. Brad Leithauser
Re: Data overrun in Perl_magic_get '?' (Was: Access violation in SV.C new_body_inline())
Craig A. Berry wrote:
On May 18, 2009, at 11:54 PM, John E. Malmberg wrote:
John E. Malmberg wrote:
The trail has taken me to the following lines just below the comment
with the tag AMS 20010810.
while(mg) {
const MGVTBL * const vtbl = mg-mg_virtual;
if (!(mg-mg_flags MGf_GSKIP) vtbl vtbl-svt_get) {
CALL_FTPR(vtbl-sv_get)(aTHX_ sv, mg);
On the return from this call, the Ibody_roots[7] is corrupted.
*vtbl-sv_get at this point is loaded with the function Perl_magic_get,
and the value it is looking for is the '?'.
And looking there, I find VMS specific code where the
my_perl-Istatusvalue_vms is put into sv-sv_any-slv_targlen.
Istatusvalue_vms contains 44, the same bad pointer value.
The body size for type SVt_PVMG (7) appears to be 32, which means that
the LvTARGLEN(sv) writing at offset 32 is the culprit, corrupting the
linked list.
I am not sure how to fix this, but now that I have found out this
much, maybe someone else here can?
So you're saying that these lines in Perl_magic_get in mg.c:
case '?':
{
sv_setiv(sv, (IV)STATUS_CURRENT);
#ifdef COMPLEX_STATUS
LvTARGOFF(sv) = PL_statusvalue;
LvTARGLEN(sv) = PL_statusvalue_vms;
#endif
}
are where the damage occurs? So it looks like the SV in question does
not even have the relevant slots (xlv_targlen) we're trying to update
here.
Yes.
I wonder if it's because IPC::Cmd declares $? as local. Maybe we
are assuming $? is always lexical but it's not?
I do not know.
-John
wb8...@qsl.net
Personal Opinion Only
Re: Data overrun in Perl_magic_get '?' (Was: Access violation in SV.C new_body_inline())
On May 19, 2009, at 8:31 AM, Nicholas Clark wrote:
On Tue, May 19, 2009 at 08:18:17AM -0500, John E. Malmberg wrote:
Craig A. Berry wrote:
So you're saying that these lines in Perl_magic_get in mg.c:
case '?':
{
sv_setiv(sv, (IV)STATUS_CURRENT);
#ifdef COMPLEX_STATUS
LvTARGOFF(sv) = PL_statusvalue;
LvTARGLEN(sv) = PL_statusvalue_vms;
#endif
}
are where the damage occurs? So it looks like the SV in question
does
not even have the relevant slots (xlv_targlen) we're trying to
update
here.
Yes.
I wonder if it's because IPC::Cmd declares $? as local. Maybe we
are assuming $? is always lexical but it's not?
I do not know.
That code rings a bell. The only thing I can find that I did near it
was:
http://perl5.git.perl.org/perl.git/commit/35f998ddd1e1665f7d0899ae3e50f9262c59d848
However I had a suspicion that I also did something that restricted
the upgrade
to the minimal case.
Maybe you were thinking of this:
http://www.nntp.perl.org/group/perl.perl5.changes/2008/09/msg22279.html
?
I don't think that lexical has anything to do with it - I suspect
that the
bug is because IPC::Cmd localises $?, and the new scalar is created
like this:
STATIC SV *
S_save_scalar_at(pTHX_ SV **sptr, const U32 flags)
{
dVAR;
SV * const osv = *sptr;
register SV * const sv = *sptr = newSV(0);
PERL_ARGS_ASSERT_SAVE_SCALAR_AT;
if (SvTYPE(osv) = SVt_PVMG SvMAGIC(osv) SvTYPE(osv) !=
SVt_PVGV) {
if (SvGMAGICAL(osv)) {
const bool oldtainted = PL_tainted;
SvFLAGS(osv) |= (SvFLAGS(osv)
(SVp_IOK|SVp_NOK|SVp_POK)) PRIVSHIFT;
PL_tainted = oldtainted;
}
mg_localize(osv, sv, (flags SAVEf_SETMAGIC) != 0);
}
return sv;
}
where that call to mg_localize() will upgrade it to PVMG, but not
PVLV.
If this is the cause, I'm not sure whether the correct fix is to make
mg_localize generally upgrade the new scalar to the type of the
existing
scalar, or special case it for $?.
Thanks for the analysis. I don't really know the implications of the
alternatives you propose. I assume a general change is more risky and
potentially adds unnecessary processing to hot code. To me the least
risky thing would be to add the following by analogy with what's
already going on in gv.c:
--- mg.c;-0 2009-04-27 02:42:10 -0500
+++ mg.c2009-05-20 19:11:11 -0500
@@ -974,6 +974,7 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg)
{
sv_setiv(sv, (IV)STATUS_CURRENT);
#ifdef COMPLEX_STATUS
+ SvUPGRADE(sv, SVt_PVLV);
LvTARGOFF(sv) = PL_statusvalue;
LvTARGLEN(sv) = PL_statusvalue_vms;
#endif
[end]
Does that make sense?
This does fix the memory error that started this thread. I'm now
starting a complete test run to make sure nothing else goes pear-shaped.
Craig A. Berry
mailto:craigbe...@mac.com
... getting out of a sonnet is much more
difficult than getting in.
Brad Leithauser
Data overrun in Perl_magic_get '?' (Was: Access violation in SV.C new_body_inline())
John E. Malmberg wrote:
The trail has taken me to the following lines just below the comment
with the tag AMS 20010810.
while(mg) {
const MGVTBL * const vtbl = mg-mg_virtual;
if (!(mg-mg_flags MGf_GSKIP) vtbl vtbl-svt_get) {
CALL_FTPR(vtbl-sv_get)(aTHX_ sv, mg);
On the return from this call, the Ibody_roots[7] is corrupted.
*vtbl-sv_get at this point is loaded with the function Perl_magic_get,
and the value it is looking for is the '?'.
And looking there, I find VMS specific code where the
my_perl-Istatusvalue_vms is put into sv-sv_any-slv_targlen.
Istatusvalue_vms contains 44, the same bad pointer value.
The body size for type SVt_PVMG (7) appears to be 32, which means that
the LvTARGLEN(sv) writing at offset 32 is the culprit, corrupting the
linked list.
I am not sure how to fix this, but now that I have found out this much,
maybe someone else here can?
-John
wb8...@qsl.net
Personal Opinion Only
Re: Data overrun in Perl_magic_get '?' (Was: Access violation in SV.C new_body_inline())
On May 18, 2009, at 11:54 PM, John E. Malmberg wrote:
John E. Malmberg wrote:
The trail has taken me to the following lines just below the comment
with the tag AMS 20010810.
while(mg) {
const MGVTBL * const vtbl = mg-mg_virtual;
if (!(mg-mg_flags MGf_GSKIP) vtbl vtbl-svt_get) {
CALL_FTPR(vtbl-sv_get)(aTHX_ sv, mg);
On the return from this call, the Ibody_roots[7] is corrupted.
*vtbl-sv_get at this point is loaded with the function
Perl_magic_get, and the value it is looking for is the '?'.
And looking there, I find VMS specific code where the my_perl-
Istatusvalue_vms is put into sv-sv_any-slv_targlen.
Istatusvalue_vms contains 44, the same bad pointer value.
The body size for type SVt_PVMG (7) appears to be 32, which means
that the LvTARGLEN(sv) writing at offset 32 is the culprit,
corrupting the linked list.
I am not sure how to fix this, but now that I have found out this
much, maybe someone else here can?
So you're saying that these lines in Perl_magic_get in mg.c:
case '?':
{
sv_setiv(sv, (IV)STATUS_CURRENT);
#ifdef COMPLEX_STATUS
LvTARGOFF(sv) = PL_statusvalue;
LvTARGLEN(sv) = PL_statusvalue_vms;
#endif
}
are where the damage occurs? So it looks like the SV in question does
not even have the relevant slots (xlv_targlen) we're trying to update
here. I wonder if it's because IPC::Cmd declares $? as local. Maybe
we are assuming $? is always lexical but it's not?
Craig A. Berry
mailto:craigbe...@mac.com
... getting out of a sonnet is much more
difficult than getting in.
Brad Leithauser
Re: Data overrun in Perl_magic_get '?' (Was: Access violation in SV.C new_body_inline())
On Tue, May 19, 2009 at 08:18:17AM -0500, John E. Malmberg wrote:
Craig A. Berry wrote:
So you're saying that these lines in Perl_magic_get in mg.c:
case '?':
{
sv_setiv(sv, (IV)STATUS_CURRENT);
#ifdef COMPLEX_STATUS
LvTARGOFF(sv) = PL_statusvalue;
LvTARGLEN(sv) = PL_statusvalue_vms;
#endif
}
are where the damage occurs? So it looks like the SV in question does
not even have the relevant slots (xlv_targlen) we're trying to update
here.
Yes.
I wonder if it's because IPC::Cmd declares $? as local. Maybe we
are assuming $? is always lexical but it's not?
I do not know.
That code rings a bell. The only thing I can find that I did near it was:
http://perl5.git.perl.org/perl.git/commit/35f998ddd1e1665f7d0899ae3e50f9262c59d848
However I had a suspicion that I also did something that restricted the upgrade
to the minimal case.
I don't think that lexical has anything to do with it - I suspect that the
bug is because IPC::Cmd localises $?, and the new scalar is created like this:
STATIC SV *
S_save_scalar_at(pTHX_ SV **sptr, const U32 flags)
{
dVAR;
SV * const osv = *sptr;
register SV * const sv = *sptr = newSV(0);
PERL_ARGS_ASSERT_SAVE_SCALAR_AT;
if (SvTYPE(osv) = SVt_PVMG SvMAGIC(osv) SvTYPE(osv) != SVt_PVGV) {
if (SvGMAGICAL(osv)) {
const bool oldtainted = PL_tainted;
SvFLAGS(osv) |= (SvFLAGS(osv)
(SVp_IOK|SVp_NOK|SVp_POK)) PRIVSHIFT;
PL_tainted = oldtainted;
}
mg_localize(osv, sv, (flags SAVEf_SETMAGIC) != 0);
}
return sv;
}
where that call to mg_localize() will upgrade it to PVMG, but not PVLV.
If this is the cause, I'm not sure whether the correct fix is to make
mg_localize generally upgrade the new scalar to the type of the existing
scalar, or special case it for $?.
Nicholas Clark
Re: Data overrun in Perl_magic_get '?' (Was: Access violation in SV.C new_body_inline())
Nicholas Clark wrote:
On Tue, May 19, 2009 at 08:18:17AM -0500, John E. Malmberg wrote:
Craig A. Berry wrote:
So you're saying that these lines in Perl_magic_get in mg.c:
case '?':
{
sv_setiv(sv, (IV)STATUS_CURRENT);
#ifdef COMPLEX_STATUS
LvTARGOFF(sv) = PL_statusvalue;
LvTARGLEN(sv) = PL_statusvalue_vms;
#endif
}
are where the damage occurs? So it looks like the SV in question does
not even have the relevant slots (xlv_targlen) we're trying to update
here.
Yes.
I wonder if it's because IPC::Cmd declares $? as local. Maybe we
are assuming $? is always lexical but it's not?
I do not know.
That code rings a bell. The only thing I can find that I did near it was:
http://perl5.git.perl.org/perl.git/commit/35f998ddd1e1665f7d0899ae3e50f9262c59d848
However I had a suspicion that I also did something that restricted the upgrade
to the minimal case.
I don't think that lexical has anything to do with it - I suspect that the
bug is because IPC::Cmd localises $?, and the new scalar is created like this:
STATIC SV *
S_save_scalar_at(pTHX_ SV **sptr, const U32 flags)
{
dVAR;
SV * const osv = *sptr;
register SV * const sv = *sptr = newSV(0);
PERL_ARGS_ASSERT_SAVE_SCALAR_AT;
if (SvTYPE(osv) = SVt_PVMG SvMAGIC(osv) SvTYPE(osv) != SVt_PVGV) {
if (SvGMAGICAL(osv)) {
const bool oldtainted = PL_tainted;
SvFLAGS(osv) |= (SvFLAGS(osv)
(SVp_IOK|SVp_NOK|SVp_POK)) PRIVSHIFT;
PL_tainted = oldtainted;
}
mg_localize(osv, sv, (flags SAVEf_SETMAGIC) != 0);
}
return sv;
}
where that call to mg_localize() will upgrade it to PVMG, but not PVLV.
If this is the cause, I'm not sure whether the correct fix is to make
mg_localize generally upgrade the new scalar to the type of the existing
scalar, or special case it for $?.
My vote would be that the mg_localize code should always want to make
sure that the new scalar could contain any value that the existing scalar.
-John
wb8...@qsl.net
Personal Opinion Only
