Skip to content

Improve the regularization term #288

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

Closed
wants to merge 3 commits into from
Closed

Conversation

ShiyuBanzhou
Copy link

To resolve this #286 issue, it is recommended to replace the original code containing the inefficient regular expression with the following optimized version:

return url.replace(/^[\u0000-\u001F\u0020]+$/ug, "");

image-1
This alternative simplifies the regular expression by removing the redundant | (OR) operator, effectively resolving the inefficient backtracking behavior. This optimization prevents potential performance degradation when handling specific inputs, ensuring more efficient processing.

@ShiyuBanzhou ShiyuBanzhou reopened this Jan 29, 2025
@ShiyuBanzhou
Copy link
Author

Hello, sorry to bother you. Now I have fixed the regular item vulnerability to ensure that its function is consistent with the original business logic. Thank you for your reminder.

domenic added a commit that referenced this pull request Feb 9, 2025
Due to a V8 bug (https://issues.chromium.org/issues/42204424), negative end-of-string matches are slow, as shown in #286. We can avoid triggering this slowdown for certain inputs by just implementing the trimming using loops.

Closes #286. Closes #288 by superseding it.
@domenic domenic closed this in #289 Feb 9, 2025
@domenic domenic closed this in 834cef8 Feb 9, 2025
@domenic
Copy link
Member

domenic commented Feb 9, 2025

Thanks for identifying this issue. As shown in #289, I decided to go with a different solution, instead of the complicated regexp with negative lookahead. It appears to be faster than the regexp based on the benchmarks I added as well.

@ShiyuBanzhou
Copy link
Author

Thank you for your reply, I think you are right, we do need to keep the code clean to ensure its readability.

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.

2 participants