Fix possible regex matching stack overflow #2150
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Arthur Chan <arthur.chan@adalogics.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a possible StackOverflowError in regex parsing of long expressions in core/src/main/java/feign/template/Expressions.java.
Java regex matchers are known to be recursive. When it handles some complicated structure like repetitive alternative paths
(a|b)*on long string input, its recursive structure will fill up the stack fast. This will result in StackOverflowError. In Expressions.java, itsEXPRESSION_PATTERNandVARIABLE_LIST_PATTERNdo contain some complicated structure. Thus when a long enough string passed in for regex matching, it will fill up the stack and crash the JVM with a StackOverflowError.This PR reduces the possibility of the StackOverflowError by introducing a maximum length check for the expression string. Whenever the string is longer than the maximum length, the parser will not run and simply throw an IllegalArgumentException. This could avoid a long string regex matching with those complicated patterns resulting in StackOverflowException and crashing the JVM.
An additional unit test case has been added for testing the changed behaviour.
We found this bug using fuzzing by way of OSS-Fuzz, where we recently integrated Feign (google/oss-fuzz#10684). OSS-Fuzz is a free service run by Google for fuzzing important open source software. If you'd like to know more about this then I'm happy to go into detail and also set up things so you can receive emails and detailed reports when bugs are found.