Skip to content

Add RegExp recursion depth limit #2543

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

Merged

Conversation

imiklos
Copy link
Contributor

@imiklos imiklos commented Sep 28, 2018

The regexp engine does not have any recursion depth check, thus it can cause problems with various regexps. Added a new build option --regexp-recursion-limit N whose
default value is 0, which is for unlimited recursion depth. Also added a build-option-test.

Fixes #2448
Fixes #2190

JerryScript-DCO-1.0-Signed-off-by: Istvan Miklos imiklos2@inf.u-szeged.hu

@LaszloLango
Copy link
Contributor

IMHO this limit should be applied to re_parse_alternative in re-compile.c also.

@imiklos imiklos force-pushed the regexp_recursion_limit branch from 79d9642 to 1c5ca4a Compare October 1, 2018 12:50
@imiklos imiklos force-pushed the regexp_recursion_limit branch 2 times, most recently from e6f4c2a to 0e8ff57 Compare October 15, 2018 12:37
@imiklos
Copy link
Contributor Author

imiklos commented Oct 16, 2018

@zherczeg @LaszloLango I've applied the first version of the patch and updated the PR.

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

Otherwise the patch is ok.

@imiklos imiklos force-pushed the regexp_recursion_limit branch from 0e8ff57 to 25de724 Compare October 17, 2018 13:33
@imiklos imiklos force-pushed the regexp_recursion_limit branch from 25de724 to da1149a Compare October 18, 2018 10:55
@imiklos
Copy link
Contributor Author

imiklos commented Oct 19, 2018

@LaszloLango @zherczeg @akosthekiss I've updated the PR.

@imiklos imiklos force-pushed the regexp_recursion_limit branch 6 times, most recently from d271448 to 5c82a89 Compare October 25, 2018 12:50
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

Only final touches.

@imiklos imiklos force-pushed the regexp_recursion_limit branch 3 times, most recently from 559750e to f8235f2 Compare October 26, 2018 12:01
@imiklos imiklos force-pushed the regexp_recursion_limit branch 2 times, most recently from 4b1a08c to 59659c0 Compare November 8, 2018 13:07
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

@akosthekiss
Copy link
Member

@imiklos ping. @rerobika has suggested some reasonable changes, please apply them (except where commented otherwise).

@imiklos imiklos force-pushed the regexp_recursion_limit branch 2 times, most recently from 9583c51 to fb04ecb Compare December 12, 2018 09:31
Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

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

Some quick style change requests.

@imiklos imiklos force-pushed the regexp_recursion_limit branch from fb04ecb to 355963f Compare December 12, 2018 10:53
@imiklos
Copy link
Contributor Author

imiklos commented Dec 12, 2018

@akosthekiss @rerobika Thank you! I've fixed the typos, style issues and updated the PR.

Copy link
Member

@rerobika rerobika left a comment

Choose a reason for hiding this comment

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

One more minor thing.

@imiklos imiklos force-pushed the regexp_recursion_limit branch 2 times, most recently from b62104e to f3a3d35 Compare December 17, 2018 08:40
@imiklos imiklos force-pushed the regexp_recursion_limit branch from f3a3d35 to 89f26c2 Compare December 19, 2018 10:51
@imiklos imiklos force-pushed the regexp_recursion_limit branch from 89f26c2 to 098afb5 Compare January 3, 2019 11:31
Copy link
Member

@rerobika rerobika left a comment

Choose a reason for hiding this comment

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

LGTM (informal)

@rerobika
Copy link
Member

@akosthekiss Do you have any more comments, remarks about this PR?

@akosthekiss
Copy link
Member

An additional question: does this PR deal only with #2448 or also with #2190 ? If it fixes both, please, update the commit message accordingly.

The regexp engine does not have any recursion depth check, thus it can cause problems with various regexps. Added a new build option `--regexp-recursion-limit N` whose
default value is 0, which is for unlimited recursion depth. Also added a build-option-test.

Fixes jerryscript-project#2448
Fixes jerryscript-project#2190

JerryScript-DCO-1.0-Signed-off-by: Istvan Miklos imiklos2@inf.u-szeged.hu
@imiklos imiklos force-pushed the regexp_recursion_limit branch from 098afb5 to ebf605f Compare January 17, 2019 14:09
@imiklos
Copy link
Contributor Author

imiklos commented Jan 17, 2019

@akosthekiss I updated the PR with the requested changes, also updated the commit message, as you mentioned, this PR also fixes the #2190.

Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

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

LGTM

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.

5 participants