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

fix(regex): limit regex manager iterations to 10k to avoid OoM #22084

Merged
merged 2 commits into from May 11, 2023

Conversation

rarkins
Copy link
Collaborator

@rarkins rarkins commented May 11, 2023

Changes

Limits matchStrings matching to 10k to reduce change of hanging/crashing. The test completes in 16ms for me locally.

Context

Fixes #22083

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

Copy link
Collaborator

@secustor secustor left a comment

Choose a reason for hiding this comment

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

We should document and log this behaviour.

@rarkins
Copy link
Collaborator Author

rarkins commented May 11, 2023

Do we document to anything like that detail today? I'm not sure where to add it where it makes sense

@secustor
Copy link
Collaborator

secustor commented May 11, 2023

I would put a warning in the regex manager docs.

We document limitations as I'm aware.

@HonkingGoose Thoughts?

viceice
viceice previously approved these changes May 11, 2023
@viceice
Copy link
Member

viceice commented May 11, 2023

I would put a warning in the regex manager docs.

We document limitations as I'm aware.

@HonkingGoose Thoughts?

It's not only a regex manager limitation, it's an re2 bug. see my comment on the issue. I would open a bug on re2.

@secustor
Copy link
Collaborator

secustor commented May 11, 2023

I agree we should open an issue on RE2, but with this PR we are introducing a limitation and this should be documented or at least logged as it is happening.

@rarkins
Copy link
Collaborator Author

rarkins commented May 11, 2023

I'll go with a warn

Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

I would put a warning in the regex manager docs.

We should add a warning to the regexManager config option docs.

And maybe explain it in our "Known Limitations" page as well?

@rarkins rarkins added this pull request to the merge queue May 11, 2023
Merged via the queue into main with commit b5d87c6 May 11, 2023
12 checks passed
@rarkins rarkins deleted the fix/22083-lazy-regex-crash branch May 11, 2023 08:00
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 35.79.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lazy matchStrings regex can cause out of memory crash
5 participants