Re: [PATCH] [RT #60508] Avoid stomping on regex ->offs when reentrant (e.g., utf8 swashes).

2009/6/21 George Greer <perl@greerga.m-l.org>:
> On Sun, 21 Jun 2009, Rafael Garcia-Suarez wrote:
>
>> I had a look at this, and this looks correct, (although the
>> Devel::PPPort part is not necessary)
>>
>> However, I have a concern : while this patch removes usage of the swap
>> entry of the regexps, it doesn't remove the swap entry from the regexp
>> structs, which look like they don't serve any purpose anymore --
>> although regcomp.c still references it (mostly to free its memory).
>
> My original cut of the patch did that but I created the second patch from a
> fresh repository and forgot to do that part again (I noted that omission on
> the RT ticket).  I've been traveling lately so I haven't had time for a 3rd
> cut of the patch to include that.  If you refer to the first patch on the RT
> ticket, you'll see there isn't much else that relies on the ->swap, at least
> in the base Perl.
>
>> But couldn't we use a broader approach and still make use of the swap
>> pointer, but save it and restore it more properly ? (I'm waving
>> hands.)
>
> I had planned to make an alternate patch using the Perl stack macros rather
> than leaving it on the C stack, but I have to research their usage more, as
> I'm new to the code, before I can write that.  My reasoning for doing the
> alternate patch as well was perhaps that would resolve the pre-existing
> comment wondering about how it works with "eval { die }".
>
>> I propose this also because this patch, if completed to remove swap,
>> would break binary compatibility, and couldn't go in 5.10.x, because
>> of the pluggable regex engines on CPAN. A completely backportable fix
>> would be nicer, if possible. If that's not realistic I guess we'll
>> keep swap in the struct for the benefit of other engines.
>
> For backportability, probably save_re_context() is a better place to do the
> saving than on the Perl stack inside the function since the struct member
> would still be there.  That would presumably be sufficient to prevent
> utf8::SWASHNEW from clobbering the original regexp offsets and still
> maintain the relative backreference offsets.  I'll be at YAPC Pittsburgh so
> the chances of doing the above soon are murky.

I need to consider this in detail, but i remember that save_re_context
doesnt solve the problem that swap does which is why the swap thing
was implemented. I dont remember the full details, but cases like qr//
objects and recursive patterns come to mind.

Really the correct solution for all of this is something I long wanted
to get implemented, that is, the capture data should be a distinct
data-structure from the regexp compiled data, and every time a match
is started that datastructure should be created privately, and it
should never be possible for two match attempts to write into the same
capture buffers.

See the deeper problem here is that when qr// and (??{ }) structures
were added they violated the original design premise that a regex
could only ever be used in one place, that being where it is declared
in code. That in turn cascaded through the code base in terms of
crufty solutions like save_re_context(), and the swap code, and the
shallow copy code.

It also may be that the "shallow copy" logic actually makes the swap
code no longer necessary.  However this patch doesnt seem to address
those concerns, at least not on a superficial review.

Im hoping we can discuss and review this in more detail when ive had
more time to study it, and you have gotten back from YAPC.

Yves



-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

• [PATCH] [RT #60508] Avoid stomping on regex ->offs when reentrant (e.g., utf8 swashes). by George Greer
• Re: [PATCH] [RT #60508] Avoid stomping on regex ->offs when reentrant (e.g., utf8 swashes). by Rafael Garcia-Suarez
• Re: [PATCH] [RT #60508] Avoid stomping on regex ->offs when reentrant(e.g., utf8 swashes). by George Greer
• Re: [PATCH] [RT #60508] Avoid stomping on regex ->offs when reentrant (e.g., utf8 swashes). by demerphq
• Re: [PATCH] [RT #60508] Avoid stomping on regex ->offs when reentrant(e.g., utf8 swashes). by George Greer