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

ReDoS: use the new codePointAt and codePointCount methods instead of regex hacks #14481

Merged
merged 3 commits into from Oct 13, 2023

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Oct 12, 2023

I wasn't sure what to put as the first word in the title of this PR.
It's shared code in the shared/ folder, but it's only used by the ReDoS libraries.

I've put the Python team as reviewers, because they motivated the work.

The codePointAt and codePointCount methods and very new, but they've been in the language for a few weeks by now.
(The PR was merged about a month ago).


I've added documentation for the new string methods to our language specification.

Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

  • Excellent comment in getCodePointAt it made all the mysterious components clear 💪 .
  • Good that you rewrote escapeUnicodeString to avoid ranking 👍
  • There are a number of calls in NfaUtils of the form getCodePointAt(..., _). It seems that exposing a predicate getACodePoint(string s) { result = s.codePointAt(_).toUnicode() } could avoid ranking for those cases.

@erik-krogh
Copy link
Contributor Author

Evaluations look good: Java, JavaScript, Python, Ruby.
Maybe a hint of a performance improvement?

@erik-krogh erik-krogh marked this pull request as ready for review October 12, 2023 12:49
@erik-krogh erik-krogh requested a review from a team as a code owner October 12, 2023 12:49
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

LGTM

@erik-krogh erik-krogh merged commit b1ad61e into github:main Oct 13, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants