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

[Backport 2.x] Avoid infinite loop in flat_object parsing (#15985) #16026

Merged

Conversation

msfroh
Copy link
Collaborator

@msfroh msfroh commented Sep 20, 2024

Description

Avoid infinite loop in flat_object parsing

We had logic in flat_object parsing that would:

  1. Try parsing a flat object field that is not an object or null.
  2. Would see an END_ARRAY token, ignore it, and not advance the parser.

Combined, this would create a scenario where passing an array of strings for a flat_object would parse the string values, then loop infinitely on the END_ARRAY token.

Manual backport of #15985

Related Issues

N/A

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

CHANGELOG.md Outdated Show resolved Hide resolved
* Avoid infinite loop in flat_object parsing

We had logic in flat_object parsing that would:

1. Try parsing a flat object field that is not an object or null.
2. Would see an END_ARRAY token, ignore it, and not advance the parser.

Combined, this would create a scenario where passing an array of
strings for a flat_object would parse the string values, then loop
infinitely on the END_ARRAY token.

Signed-off-by: Michael Froh <[email protected]>

* Remove some unused code and add more tests

The removed code does not actually seem to affect the logic. Also, I
want to be 100% sure that every call to parseToken is guaranteed to
call parser.nextToken() at some point.

Signed-off-by: Michael Froh <[email protected]>

* Remove unused parameter from parseToken

Thanks for the reminder, @kkewwei!

Signed-off-by: Michael Froh <[email protected]>

* Add skip for newly-added test

The test fails on MixedClusterClientYamlTestSuiteIT because 2.x still
has the infinite loop until backport.

Signed-off-by: Michael Froh <[email protected]>

---------

Signed-off-by: Michael Froh <[email protected]>
(cherry picked from commit 05dab3b)
@msfroh msfroh force-pushed the backport/backport-15985-to-2.x branch from 5094b95 to c7da98f Compare September 20, 2024 22:36
Copy link
Contributor

✅ Gradle check result for c7da98f: SUCCESS

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 73.91304% with 6 lines in your changes missing coverage. Please review.

Project coverage is 71.76%. Comparing base (0870adb) to head (c7da98f).
Report is 2 commits behind head on 2.x.

Files with missing lines Patch % Lines
...opensearch/index/mapper/FlatObjectFieldMapper.java 70.58% 4 Missing and 1 partial ⚠️
...ch/common/xcontent/JsonToStringXContentParser.java 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                2.x   #16026      +/-   ##
============================================
+ Coverage     71.71%   71.76%   +0.05%     
- Complexity    64635    64707      +72     
============================================
  Files          5276     5276              
  Lines        302341   302333       -8     
  Branches      43993    43989       -4     
============================================
+ Hits         216819   216980     +161     
+ Misses        67407    67226     -181     
- Partials      18115    18127      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@msfroh msfroh merged commit 234c4da into opensearch-project:2.x Sep 20, 2024
35 checks passed
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