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

fix(flux-core): infinite loop protection in the parser #5436

Merged
merged 3 commits into from
Oct 25, 2023

Conversation

mhilton
Copy link
Contributor

@mhilton mhilton commented Oct 25, 2023

Update all loops using self.more in the parser to detect if they get stuck attempting to process the same token multiple times. This has been observed to cause the parser to get into an infinite loop with some erroneus inputs.

The protection code was copied from the parse_array_items_rest and applied everywhere the parser could conceivably get stuck. It is not clear that it is possible to get stuck in all the places that the protection was added.

Checklist

Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.

  • ✏️ Write a PR description, regardless of triviality, to include the value of this PR
  • 🔗 Reference related issues
  • 🏃 Test cases are included to exercise the new code
  • 🧪 If new packages are being introduced to stdlib, link to Working Group discussion notes and ensure it lands under experimental/
  • 📖 If language features are changing, ensure docs/Spec.md has been updated

Dear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.

Update all loops using self.more in the parser to detect if they
get stuck attempting to process the same token multiple times. This
has been observed to cause the parser to get into an infinite loop
with some erroneus inputs.

The protection code was copied from the parse_array_items_rest and
applied everywhere the parser could conceivably get stuck. It is
not clear that it is possible to get stuck in all the places that
the protection was added.
@mhilton mhilton requested a review from a team as a code owner October 25, 2023 15:33
@mhilton mhilton self-assigned this Oct 25, 2023
Copy link
Contributor

@btasker btasker left a comment

Choose a reason for hiding this comment

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

LGTM - tested against the original testcase and it definitely resolves that.

Copy link
Contributor

@appletreeisyellow appletreeisyellow left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! The new test cases are very helpful for review!

Comment on lines +745 to +753

// If we parse the same token twice in a row,
// it means we've hit a parse error, and that
// we're now in an infinite loop.
let this = self.peek().start_offset;
if last == this {
break;
}
last = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the comment 👍

@mhilton mhilton merged commit 43ad07e into master Oct 25, 2023
6 of 7 checks passed
@jacobmarble jacobmarble deleted the mh-parameter-list-loop branch January 4, 2024 17:09
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