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

test for out of memory problem with complex glob #533

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danez
Copy link

@danez danez commented Jun 7, 2023

I was running into an out-of-memory problem with one of our tools and traced it back to node-glob.

The problem arises when the glob is extremely complex or potentially invalid. I know this is a pretty heavy edge case and the glob in the test is CSS and not a glob :) but I was wondering if there is something that can be done about this and if the test might help to debug this.

This also only really happens if the folder where the glob is executed against is big. That is why I set it to the repo root in the test. In the fixtures directory it usually finishes after 10 to 15 seconds

Side note: fast-glob handles this without a problem and super fast. (obviously does not match anything)

@isaacs
Copy link
Owner

isaacs commented Jun 7, 2023

Hah, yeah, css is just close enough to being a valid glob pattern that I imagine it sends the parsing into a bit of a tailspin. I'll take a look, my first guess is that either it's just too long to reasonably handle, or it's resulting in some ReDOS-ish patterns somehow.

@isaacs
Copy link
Owner

isaacs commented Jun 7, 2023

The fact that fast-glob doesn't error on it makes me think it's down to the parser, since node-glob does a lot more up front optimization. Might be a good idea to look into more of a parse-on-demand approach like fast glob uses, but there are some correctness edge cases that do require consuming and validating the whole pattern up front unfortunately.

@dbssman
Copy link

dbssman commented Feb 21, 2024

Hey @isaacs is there any update on this ?
Thanks in advance!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants