Repository navigation
Conversation
…ing them NumberConverter parsed numeric strings of any length with BigInteger, BigDecimal and DecimalFormat, whose string parsing is quadratic in the length of the string. A request parameter value is not length-limited (only the parameter name is), so a long digit string bound to a boxed number, BigInteger or BigDecimal property cost seconds of CPU. Strings longer than 1000 characters are now rejected with a TypeConversionException, which is reported as the usual conversion error for the field. 1000 is well above any number the framework itself produces; surrounding whitespace is trimmed before the check.
This file contains hidden or 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
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.
Fixes WW-5834
NumberConverterparses numeric strings withBigInteger,BigDecimalandDecimalFormat, which are quadratic in the length of the string. A request parameter value has no length limit (ParametersInterceptorlimits only the name), so a long digit string bound to a boxed number,BigIntegerorBigDecimalproperty costs CPU out of proportion to its size: on a running Struts Boot app, one POST with anIntegerand aBigDecimalproperty given the same value took 0.31 s at 100,000 digits and 3.98 s at 400,000 digits onmain.Strings longer than 1000 characters are now rejected with a
TypeConversionExceptionat the top of theStringbranch ofconvertValue, before any parsing. That one place coversBigInteger(which bypassesisInRange),BigDecimal,Double,Float, boxed and primitive types.XWorkConverterreports the exception as the usual field conversion error (covered by a test).Why 1000:
StringConverternever produces more than 681 characters (grouping off, 340 integer + 340 fraction digits + separator), doubles need about 330, and the longest existing test value is about 636.XWorkBasicConvertertrims surrounding whitespace before callingNumberConverter, so padding does not count (covered by a test).How verified: with the production change reverted
NumberConverterTest.testExceptionWhenNumberIsTooLongandXWorkConverterTest.testFieldErrorMessageAddedWhenNumberIsTooLongfail; with it they pass.testStringToBigDecimalConversionWithMaxLength(exactly 1000 digits still converts) and the whitespace test pass both ways. Fullcoresuite: 3629 run, 0 failures, 2 skipped.On the running app with this change the same requests take 0.014 s (100,000 digits) and 0.016 s (400,000 digits), ordinary values (
count=12345&amount=12345.67) still convert, and a 1001-character value is reported as a conversion error.Not changed:
StringConverter,isInRange, any configuration option. Risk: numbers longer than 1000 characters can no longer be converted.