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.c        2009-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:[email protected]

"... getting out of a sonnet is much more
  difficult than getting in."
                  Brad Leithauser