MAX_RECURSE_EVAL_NOCHANGE_DEPTH

[hvds]

This was introduced as 50 by 6bda09f (Yves, 2006-10-04), made compile-time overridable by d56b301 (Rafael, 2007-02-14), bumped to 1000 by 4b196cd (Yves, 2007-02-15), and then dropped to 10 by ba6840f (Yves, 2016-03-06 as part of #14935). There are brief mentions in perlre under (?PARNO) and (??{ expr }):

Recursing deeply without consuming any input string will
also result in a fatal error.  The depth at which that happens is
compiled into perl, so it can be changed with a custom build.
[...]
Executing a postponed regular expression too many times without
consuming any input string will also result in a fatal error.

.. but once we hit a GOSUB it looks like every additional GOSUB or EVAL ("postponed" or not) bumps the count, so as of now we get:

% perl -wle '"foo" =~ m{(?&FOO)(?(DEFINE)(?<FOO>
    (?{1}) (?{1}) (?{1}) (?{1}) (?{1})
    (?{1}) (?{1}) (?{1}) (?{1}) (?{1})
    (?{1})
    foo
))}x'
EVAL without pos change exceeded limit in regex at -e line 1.
% 

This behaviour breaks Regexp::Debugger for quite trivial cases, which is a shame.

I consider it wrong that this recursion check is any more than a warning (as for sub recursion); if there's a good reason for it to be fatal, I think it really must be controllable without having to recompile perl. But the first question is whether it is actually checking the right thing - should it be treating normal (?{...}) evals as a candidate for bumping the recursion depth at all?

@demerphq can you clarify if this check was intended to kill patterns like the above?

Hugo

[demerphq]

On Sun, 26 Jan 2020, 19:43 Hugo van der Sanden, ***@***.***> wrote: This was introduced as 50 by 6bda09f <6bda09f> (Yves, 2006-10-04), made compile-time overridable by d56b301 <d56b301> (Rafael, 2007-02-14), bumped to 1000 by 4b196cd <4b196cd> (Yves, 2007-02-15), and then dropped to 10 by ba6840f <ba6840f> (Yves, 2016-03-06 as part of #14935 <#14935>). There are brief mentions in perlre under (?PARNO) and (??{ expr }): Recursing deeply without consuming any input string will also result in a fatal error. The depth at which that happens is compiled into perl, so it can be changed with a custom build. [...] Executing a postponed regular expression too many times without consuming any input string will also result in a fatal error. .. but once we hit a GOSUB it looks like every additional GOSUB or EVAL ("postponed" or not) bumps the count, so as of now we get: % perl -wle '"foo" =~ m{(?&FOO)(?(DEFINE)(?<FOO> (?{1}) (?{1}) (?{1}) (?{1}) (?{1}) (?{1}) (?{1}) (?{1}) (?{1}) (?{1}) (?{1}) foo ))}x' EVAL without pos change exceeded limit in regex at -e line 1. % This behaviour breaks Regexp::Debugger for quite trivial cases, which is a shame. I consider it wrong that this recursion check is any more than a warning (as for sub recursion); if there's a good reason for it to be fatal, I think it really must be controllable without having to recompile perl.

I seem to recall there are good reasons not to but off the top of my head I don't remember. I'll find some time in the next week to do so. But the first question is whether it is actually checking the right thing -

should it be treating normal (?{...}) evals as a candidate for bumping the recursion depth at all?

(?{...}) no. (??{...}) yes. @demerphq <https://github.com/demerphq> can you clarify if this check was

…

intended to kill patterns like the above? Hugo — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17490?email_source=notifications&email_token=AAAZ5R3TXKHX5NZOSCB3DXTQ7VZPLA5CNFSM4KLWNIAKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IIX53JA>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZ5RZ4EZVJ737GQRMO5TDQ7VZPLANCNFSM4KLWNIAA> .

[hvds]

But the first question is whether it is actually checking the right thing -
should it be treating normal (?{...}) evals as a candidate for bumping the recursion depth at all?

(?{...}) no. (??{...}) yes.

I think that may be as simple as:

--- a/regexec.c
+++ b/regexec.c
@@ -7411,3 +7411,3 @@ S_regmatch(pTHX_ regmatch_info *reginfo, char *startpos, regnode *prog)
         case EVAL:  /*   /(?{...})B/   /(??{A})B/  and  /(?(?{...})X|Y)B/   */
-            if (cur_eval && cur_eval->locinput==locinput) {
+            if (logical == 2 && cur_eval && cur_eval->locinput==locinput) {
                if ( ++nochange_depth > max_nochange_depth )

.. since the 3 places it is decremented all have comments saying they are specifically for (??{...}) constructs (EVAL_postponed_AB, EVAL_postponed_AB_fail and END/fake_end). I'll try running some tests.

Hugo

[hvds]

Created a PR for this change as a first step.

[demerphq]

Looks good to me Yves

…

On Mon, 27 Jan 2020, 08:31 Hugo van der Sanden, ***@***.***> wrote: Created a PR for this change as a first step. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17490?email_source=notifications&email_token=AAAZ5RYQBAJ245AQDR754VDQ7YTMXA5CNFSM4KLWNIAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ6B4XI#issuecomment-578559581>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZ5R435GHTLVYR4EZXIPTQ7YTMXANCNFSM4KLWNIAA> .

[hvds]

Ok, merged that.

For the remaining questions, I'm confused now precisely what constructs should trigger "Pattern subroutine nesting/EVAL without pos change exceeded limit in regex" that would not first be caught by the "infinite recursion" tests. I tried crafting a cascade of (?<A>(?&B))(?<B>(?&C))..., but that didn't trigger it.

I think it'd be useful to have a couple of tests for these errors. I'm happy to write those if I can understand what's supposed to trigger them.

[demerphq]

On Mon, 27 Jan 2020, 11:56 Hugo van der Sanden, ***@***.***> wrote: Ok, merged that.

Cool. One thought I had after reading the patch is that we should replace the 2 with a symbolic define for legibility. For the remaining questions, I'm confused now precisely what constructs

should trigger "Pattern subroutine nesting/EVAL without pos change exceeded limit in regex" that would not first be caught by the "infinite recursion" tests. I tried crafting a cascade of (?<A>(?&B))(?<B>(?&C))..., but that didn't trigger it.

I think one came before the other, and it is entirely possible that the former is redundant in the face of the latter /for GOSUB/, although I have a vague recollection that we dont necessarilly catch all forms of infinite recursion with those checks. But I think it safe to say those checks dont help for EVAL. Consider something like this: our $eval; $eval= qr/(??{ $eval })/; "Foo" =~/$eval/; Iirc without the second set of checks this will actually SEGV due to stack exhaustion. Yves I think it'd be useful to have a couple of tests for these errors. I'm

…

happy to write those if I can understand what's supposed to trigger them. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17490?email_source=notifications&email_token=AAAZ5R7HLNVTJDRMRHYWQFDQ7ZLPXA5CNFSM4KLWNIAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ6HWYQ#issuecomment-578583394>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZ5R657MJ4HKCIKBCYO7DQ7ZLPXANCNFSM4KLWNIAA> .

[hvds]

Right, the infinite recursion check is really aggressive now:

% perl -le '
  print +("foo" =~ m{(?&F)
    (?(DEFINE)(?<F>
      (??{ $x++ ? "(?!)" : "(?=)" })
      (?&F)
    |
      foo
    ))
  }x) ? "ok" : "not ok"
'
Infinite recursion in regex at -e line 1.
% 

I think that's a case that would be more ideally served by a (preferably overridable) "X without pos change exceeded limit" check.