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

Fixed possible regular expression catastrophic backtracking #105

Merged
merged 16 commits into from
Nov 10, 2020
Merged

Fixed possible regular expression catastrophic backtracking #105

merged 16 commits into from
Nov 10, 2020

Conversation

marekdedic
Copy link
Contributor

@marekdedic marekdedic commented Mar 26, 2019

Fixes #95

@marekdedic
Copy link
Contributor Author

I don't think the failing CI has anything to do with the PR - let me know if it does.

@marekdedic
Copy link
Contributor Author

marekdedic commented Jun 20, 2019

Hey, any progress on this? This is a blocking issue for me...

Thanks a lot,
Marek

@pkuczynski
Copy link
Collaborator

@marekdedic, apologies for the late reply. Could you provide some tests and changelog entry for this?

@marekdedic
Copy link
Contributor Author

Hi, thanks for the reply anyway 🙂

I've added a changelog entry, however, I'm a bit stuck with the tests, see #126.

@pkuczynski pkuczynski changed the title Fixed issue #95 Fix poor performance of a particular example on IE 6 Nov 2, 2020
@pkuczynski pkuczynski changed the title Fix poor performance of a particular example on IE 6 Fixed possible regular expression catastrophic backtracking Nov 2, 2020
@pkuczynski
Copy link
Collaborator

@marekdedic I had a look at the example you provided and after adding ? as you suggest, I am only getting a match on .:

https://regex101.com/r/MlbAhW/1 vs https://regex101.com/r/MlbAhW/2

I don't think this is correct?

@pkuczynski pkuczynski mentioned this pull request Nov 2, 2020
@marekdedic
Copy link
Contributor Author

Hmm, it's not correct. I don't really know how to fix it though, as I am not really sure what the mathOutsideOfBrackets function does...

@pkuczynski
Copy link
Collaborator

To be honest, I don't know either. I have not written this package and I am only maintaining it. Approving PR and releasing the new versions. The only person who knows is @anandthakker, but he is not responsive :(

Copy link
Collaborator

@pkuczynski pkuczynski left a comment

Choose a reason for hiding this comment

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

Does not seem to work...

@marekdedic
Copy link
Contributor Author

Hi,
I looked over it once more and it seems to me that the function matchOutsideOfBrackets(X) checks whether there is the characterX somewhere not inside [] or (). So the match on . would actually be correct. And expanding the regex constructed by mathcOutsideOfBrackets, the non-greedy approach doesn't change the meaning.

So I do actually think this PR is correct, however there is one more thing... We can actually undo #88 with this change and simplify it!

@marekdedic
Copy link
Contributor Author

(and undoing #88 would allow for testing for catastrophic regexes...)

@marekdedic
Copy link
Contributor Author

Hi, the PR is now working and with tests for unsafe regexes (not limited to the one in #95)

@marekdedic
Copy link
Contributor Author

Hmm, so the tests wouldn't catch #95. I've tried switching safe-regex for vuln-regex-detector but that seems to be abandoned... So at least an imperfect test...

Copy link
Collaborator

@pkuczynski pkuczynski left a comment

Choose a reason for hiding this comment

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

As I said before, I am not able to fairly judge the changes. If you confident this is correct, I am fine with merging it and releasing as next version.

@pkuczynski
Copy link
Collaborator

And thank you for putting effort into this! :)

@marekdedic
Copy link
Contributor Author

Hold your horses. I think I've found some more performance issues with this function. Going to go for a different solution to solve it once and for all.

@marekdedic
Copy link
Contributor Author

Hi, I've reworked it to not use such a horrendous regex but a simpler function instead. Unit tests pass, tried it on 2 of my projects, same output, no performance issues. I think you can merge this now.

@pkuczynski
Copy link
Collaborator

Let's give it a try and release it as next major version :)

@pkuczynski pkuczynski added the bug label Nov 10, 2020
@pkuczynski pkuczynski merged commit 9c31557 into anandthakker:master Nov 10, 2020
@marekdedic marekdedic deleted the fix-95 branch November 10, 2020 11:11
@marekdedic marekdedic mentioned this pull request Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poor performance of a particular example on ie 6
2 participants