Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regexec.c: Rmv obsolete recursion checks: GH #8369 #19636

Merged
merged 1 commit into from May 28, 2022

Conversation

khwilliamson
Copy link
Contributor

There is no recursion to exceed limits of.

This fixes #8369

There is no recursion to exceed limits of.

This fixes Perl#8369
Comment on lines -8807 to -8821
if (cur_curlyx->u.curlyx.count >= /*max*/ARG2(cur_curlyx->u.curlyx.me)) {
/* Maximum greed exceeded */
if (cur_curlyx->u.curlyx.count >= REG_INFTY
&& ckWARN(WARN_REGEXP)
&& !reginfo->warned)
{
reginfo->warned = TRUE;
Perl_warner(aTHX_ packWARN(WARN_REGEXP),
"Complex regular subexpression recursion "
"limit (%d) exceeded",
REG_INFTY - 1);
}
cur_curlyx->u.curlyx.count--;
CACHEsayNO;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this removes too much: the outer block looks to me like it is checking whether we've reached the max allowed for this (minimal) repetition. I can have a dig to verify, but I expect @demerphq will know.

If I'm correct, then we should really have a test that fails when you remove this. (I'm a bit surprised we don't have any tests of the warning, for that matter.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I believe you are correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khwilliamson I think this got merged without fixing the above over-removal, please could you verify?

@khwilliamson
Copy link
Contributor Author

The test suite only exercises this code via the fold_grind tests, and only then because it is using a ?? quantifier

@khwilliamson khwilliamson added the defer-next-dev This PR should not be merged yet, but await the next development cycle label Apr 25, 2022
@khwilliamson khwilliamson merged commit fd1cd0c into Perl:blead May 28, 2022
@khwilliamson khwilliamson deleted the 8369 branch May 28, 2022 17:31
khwilliamson added a commit that referenced this pull request May 28, 2022
I fixed this locally as a result of Hugo van der Sanden's comment in
 #19636, but neglected to commit it.
@khwilliamson
Copy link
Contributor Author

Thanks for finding this. Fixed by bdadb24

I'll change the issue title to indicate we need tests for this

@khwilliamson
Copy link
Contributor Author

I instead created a new ticket #19781

scottchiefbaker pushed a commit to scottchiefbaker/perl5 that referenced this pull request Nov 3, 2022
I fixed this locally as a result of Hugo van der Sanden's comment in
 Perl#19636, but neglected to commit it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defer-next-dev This PR should not be merged yet, but await the next development cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can exceed regex complex recursion limit
2 participants