Re: [perl #97088] [PATCH] Prevent executing get-magic twice forderefing by makin a non-magic copy instead of using the OPpDEREFed flag.

On Tue, Aug 16, 2011 at 12:38:25AM -0700, Gerard Goossen wrote:
> Currently preventing get-magic to be executed twice (once by the op
> returning the value to do autovivication and once by the rv2xv op).
> is being done by setting setting OPpDEREFed on the rv2xv op in the
> peephole optimizer, but this doesn't always work, see the test cases
> added.
> Instead of setting the OPpDEREFed flag, make a non-magic copy of the
> SV in vivify_ref (the first time get-magic is executed).
[snip]
> -Perl_vivify_ref(pTHX_ SV *sv, U32 to_what)
> +Perl_vivify_ref(pTHX_ SV **svp, U32 to_what)
>  {
> +    SV* sv = *svp;
>      PERL_ARGS_ASSERT_VIVIFY_REF;
>  
>      SvGETMAGIC(sv);
> @@ -2876,6 +2873,12 @@ Perl_vivify_ref(pTHX_ SV *sv, U32 to_what)
>  	SvROK_on(sv);
>  	SvSETMAGIC(sv);
>      }
> +    if (SvGMAGICAL(sv)) {
> +	/* copy the sv without magic to prevent magic from being
> +	   executed twice */
> +	*svp = sv_newmortal();
> +	sv_setsv_nomg(*svp, sv);
> +    }
>  }

This is very wrong: consider for example the case where vivify_ref is
called from pp_aelem; in this case the original SV in the AV slot is
leaked, and is replaced with a mortal copy that is soon freed: so the AV
slot points to a freed scalar.

Here's a trivial demonstration: (I used taint just as simple way to give
a scalar get magic):

    $ cat /tmp/p
    use Devel::Peek;
    $a[0] = $^X;
    $a[0][0] = 1; # calls vivify_ref
    Dump $a[0];

    $ ./perl -T -Ilib /tmp/p
    SV = UNKNOWN(0xff) (0xfb6548) at 0xf96b98
      REFCNT = 0
      FLAGS = ()
    $


-- 
The Enterprise is captured by a vastly superior alien intelligence which
does not put them on trial.
    -- Things That Never Happen in "Star Trek" #10

• [perl #97088] [PATCH] Prevent executing get-magic twice for derefing by makin a non-magic copy instead of using the OPpDEREFed flag. by Gerard Goossen
• Re: [perl #97088] [PATCH] Prevent executing get-magic twice forderefing by makin a non-magic copy instead of using the OPpDEREFed flag. by Dave Mitchell