git.shadowcat.co.uk Git - p5sagit/p5-mst-13.2.git/log

projects / p5sagit/p5-mst-13.2.git / log

David Mitchell [Tue, 23 Mar 2010 12:11:43 +0000]

Fix assorted bugs related to magic (such as pos) not "sticking" to
magical array and hash elements; e.g. the following looped infinitely:

    $h{tainted_element} =~ /..../g

There are two side-effects of this fix.
First, MGf_GSKIP has been extended to work on tied array
elements as well as hash elements. This is the mechanism that skips all
but the first tied element magic gets until after the next set.
Second, rvalue hash/array element access where the element has get magic,
now directly returns the element rather than a mortal copy.

The root cause of the bug was code similar to the following in pp_alem,
pp_aelemfast, pp_helem and pp_rv2av:

    if (!lval && SvGMAGICAL(sv)) /* see note in pp_helem() */
sv = sv_mortalcopy(sv);

According to the note, this was added in 1998 to make this work:

    local $tied{foo} = $tied{foo}

Since it returns a copy rather than the element, this make //g fail.
My first attempt, a few years ago,  to fix this, took the approach that
the LHS of the bind should be made an lvalue in the presence of //g, since
it now modifies its LHS; i.e.

    expr =~ //        expr is rvalue
    expr =~ s///      expr is lvalue
    expr =~ //g       expr was rvalue, I proposed to change it to lvalue

Unfortunately this fix broke too much stuff (stuff that was arguably
already broken, but it upset people). For example, f() ~= s////
correctly gives the error

    Can't modify non-lvalue subroutine call

My fix extended f() =~ //g to give the same error. Which is reasonable,
because the g isn't doing what you want. But plenty of people had code that
only needed to match once and the g had just been cargo-culted. So it
broke their working code. So lets not do this.

My new approach has been to remove the sv_mortalcopy(). It turns out
that this is no longer needed to fix the local $tied{foo} issue.
Presumably that went away as a side-effect of my container/value magic
localisation rationalisation of a few years ago, although I haven't
analysed it - just noted that the tests still pass (!). However, an issue
with removing it is that mg_get() no longer gets called. So a plain

    $tied_hash{elem};

in void context no longer calls FETCH(). Which broke some tests and might
break some code. Also, there's an issue with the delayed calling of magic
in @+[n] and %+{foo}; by the time the get magic is called, the original
pattern may have gone out of scope.

The solution is to simply replace the original

sv = sv_mortalcopy(sv);

with

mg_get(sv);

This then caused problems with tied array FETCH() getting called too much.
I fixed this by extending the MGf_GSKIP mechanism to tied arrays as well
as hashes. I don't understand why tied arrays have always been treated
differently than tied hashes, but unifying them didn't seem to break
anything (except for a Storable test, whose comment indicated that the
test's author thought FETCH() was being called to often anyway).