fix recursive code block regexes #23043
Open
+128
−22
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When doing a low-level dump of a pattern-match op (PMOP), it currently displays a stringified representation of the regex attached to the op. This commit makes it also display the regex SV's address and, on threaded builds, the offset within the regex pad where that SV is stored.
Convert the SET_reg_curpm() file-scoped macro into a static function, S_set_reg_curpm(). This makes debugging and any future enhancements easier.
In something like
my $pat = /... (?{ foo() if ...; }) .../;
sub foo { $string =~ $pat; }
foo();
perl would SEGV or produce the wrong values for $1 et al. This commit
fixes that.
Background:
For a compile-time pattern like $foo =~ /..../, the pattern is compiled
at compile time, and the resulting REGEXP SV is stored as a pointer in
the OP_MATCH op (or on threaded builds, stored in PL_regex_pad[],
indexed by the op_pmoffset field in the match op).
For a runtime pattern like
$foo =~ /abc$bar/;
or
$pat = qr/..../;
$foo =~ $pat;
the pattern is compiled by a regcomp op (or for a qr// object,
duplicated), and the resulting REGEX SV is stored by the regcomp op into
its related match (or subst or split etc) op.
This regex will likely have a refcount of 1: i.e. it is only being kept
alive by the link from the match op. Normally this is fine: the regex
lives for as long as the op (and hence the sub it lives in) exist. In
particular, the regex continues to live on after the match is complete,
so that $1 etc will work.
$1 etc work by perl setting PL_curpm to point to the match op which most
recently did a successful match. This is dynamically scoped: on scope
exit, the old value of PL_curpm is restored.
When $1 is accessed, its get-magic is called, which looks up PL_curpm,
gets the regex pointed to by that match op, and that regex contains the
char ranges and match string associated with the most recent match.
The Problem:
That all works well until the`
sub foo { $string =~ $pat; }
from the example above is called recursively from the /(?{...}/). When
foo() is first called, $pat is compiled and the resulting REGEXP is
stored in the OP_MATCH with a ref count of 1. OP_MATCH is then executed,
which calls the regex engine with that regex and string. Part of the
match is a (?{...}) which recursively calls foo(). foo() does an
OP_REGCOMP again, which overwrites the current regex in the OP_MATCH
with a new regex, freeing the old regex (the one we are in the middle of
executing). Cue SEGVs etc.
There is a further complication: PL_curpm points to the current
successful *match op* rather than the current *regex*. When the regex
engine was made accessible via an API, it was possible for the engine to
be running with no active OP_MATCH present. But the design of (?{...})
is such that any partial matches are accessible *during* the execution,
not just after the end of a successful match. So for example
"AB" =~ /^(.) (?{ say "$1$2" } (.)$/x;
will print out "A" and undef. The regex engine handles this by having a
fake global match PMOP structure, PL_reg_curpm, and every time code
within (?{...}) is about to be called, the current regex is pointed to
from PL_reg_curpm, and PL_curpm is set to point to PL_reg_curpm. Since
this is global, it suffers from the same problem as for the recursive
match op, in that the inner call to (?{...}) will overwrite the regex
pointer in PL_reg_curpm, potentially prematurely freeing the regex, and
even if not freed, meaning that on return to the outer pattern, $1 et al
will refer to the inner match, not the current match.
The Solution:
This commit makes use of an existing save/restore mechanism for patterns
involving (?{...}). At the start of the match, S_setup_eval_state() is
called, which saves some state in reginfo->info_aux_eval. On exit from
the match (either normally or via croak), S_cleanup_regmatch_info_aux()
is called to restore stuff.
This commit saves three new things in info_aux_eval.
1) The current REGEXP SV (ref counted). Formerly it stored ReANY(regex);
now it stores regex directly. This ensures the regex isn't freed
during matching (including calls out to code in (?{...}) blocks), but
it doesn't guarantee that it will live on after the end of the match,
to be accessible to $1 etc al.
2) It saves (ref counted) the current value of the regex pointer in
PL_reg_curpm and restores it on return. Thus on return from doing the
inner match, $1 et al will give the current value for any remaining
code within the code block, e.g. /(?{ foo(); print $1 })/
3) If PL_op happens to be a pattern match op (it might not if for
example the engine has been called via the API from XS) then its
regex is saved and restored similar to (2).
The combination of those three extra saves makes it likely that the
regex will not be prematurely freed, and $1 etc will have the right
values at all times.
Note that this commit doesn't fix the general problem of recursively
calling a match op; only the ones involving calls from within a
(?{...}). For example this still prints "BB" rather than "AB":
sub foo {
$_[0] =~ /(.)/;
foo('B') if $_[0] eq 'A';
print $1;
}
foo('A');
Note that the PM_GETRE() and PM_SETRE() macros, which I wanted to use to
save and restore the regex pointer in PL_reg_curpm, do some funny
business: PM_GETRE() returns NULL if the SV isn't a REGEX (e.g. if its
&PL_sv_undef),and PM_SETRE asserts that the regex isn't null. I got
round those side-effects by adding PM_GETRE_raw()/PM_SETRE_raw(), which
do nothing but get/set the regex from the PMOP.
Explain better in various places how RX_SUBBEG is set, saved and restored.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GH #22869
Fix crashes when code within a regex code block /(?{...})/ recursively executes the same pattern.
There is one main commit which fixes the issue, plus a few other commits which tidy code, tweak comments etc for things I noticed while trying to identify and fix the bug.