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 bug in string split #2146

Merged
merged 5 commits into from
Aug 8, 2023
Merged

Conversation

arthurscchan
Copy link
Contributor

@arthurscchan arthurscchan commented Aug 3, 2023

This fixes an ArrayIndexOutOfBoundException in core/src/main/java/feign/template/Expressions.java.

If the String.split(regex) method is called without providing the limit parameter, it has the same behaviour as String.split(regex, 0) in which the limit is default to 0. As mentioned in the JDK javadoc, if the limit is set to 0, all trailing empty string in the split result will be discarded. For example, "A::A".split(":") will result in ["A", "", "A"] correctly, but A::".split(":") will result in ["A"] because the two trailing empty strings are discarded. If the string only contains the split character, it will even result in an empty array. For example, "::".split(":") will result in [] because the 3 empty strings are discarded. Because of this reason, if the provided variableName contains only ":" characters, it will result in ArrayIndexOutOfBoundException in the next line. Also, if the provided variableName contains only 1 ":" characters and end with it, parts[1] will also result in ArrayIndexOutOfBoundException.

This PR fixes the possible ArrayIndexOutOfBoundException by adding the limit 2 to the split method call. With the limit set, it is guaranteed to have a result String array with size equal to the limit if ":" split character does exist. This setting should avoid the possible ArrayIndexOutOfBoundException. If the string contains an undetermined number of ":" characters, using the limit = -1 can guarantee all the trailing empty strings are not discarded. The size of the resulting String array is guaranteed to be the number of ":" character + 1.

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 in details and also set up things so you can receive emails and detailed reports when bugs are found.

Signed-off-by: Arthur Chan <[email protected]>
@velo
Copy link
Member

velo commented Aug 4, 2023

Can we get a unit test covering the original issue?

@arthurscchan
Copy link
Contributor Author

@velo Thanks for the reply. I will prepare a unit testing.

Signed-off-by: Arthur Chan <[email protected]>
@arthurscchan
Copy link
Contributor Author

arthurscchan commented Aug 8, 2023

@velo Thanks again for your previous comment, the unit test for the bug is ready now.

@velo velo merged commit b46ad26 into OpenFeign:master Aug 8, 2023
velo pushed a commit that referenced this pull request Oct 7, 2024
* Fix empty split bug

Signed-off-by: Arthur Chan <[email protected]>

* Add unit testing

Signed-off-by: Arthur Chan <[email protected]>

* Fix formatting

Signed-off-by: Arthur Chan <[email protected]>

---------

Signed-off-by: Arthur Chan <[email protected]>
velo pushed a commit that referenced this pull request Oct 8, 2024
* Fix empty split bug

Signed-off-by: Arthur Chan <[email protected]>

* Add unit testing

Signed-off-by: Arthur Chan <[email protected]>

* Fix formatting

Signed-off-by: Arthur Chan <[email protected]>

---------

Signed-off-by: Arthur Chan <[email protected]>
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