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
[email protected]
Personal Opinion Only