pjfanning opened a new pull request, #97: URL: https://github.com/apache/poi-xmlbeans/pull/97
Follow-up to #96. That PR fixed one instance of a pattern that recurs across the 5.4.0 `MathUtil` work: a silently-truncating `BigDecimal` conversion was replaced by one that throws, on a path that isn't a validation path and has no way to report a failure. Four more call sites in that family. All are reachable through the public API and verified against a build of trunk. ### 1. `hashCode()` throws — `JavaDecimalHolder.value_hash_code()` ``` XmlDecimal(2000-digit).valueHashCode() -> IllegalArgumentException: BigDecimal magnitude too large ... (limit 1024) XmlDecimal(1E+2000).valueHashCode() -> IllegalArgumentException: ... ``` `XmlObjectBase.hashCode()` calls this, so an oversized decimal poisons every `HashMap`/`HashSet` it goes into. Such values are legitimately reachable — `set_BigDecimal` is unguarded, so `setBigDecimalValue(...)` stores them and `getBigDecimalValue()` returns them; only hashing fails. The max-number-chars limit can't be applied here anyway. `JavaIntegerHolder.value_hash_code` has no limit (it already holds the `BigInteger`), an `XmlInteger` holding the same value is `valueEquals` to the decimal, and the method's contract is that they hash alike — capping one side breaks that. The limit also bounds *parse-time* work; by hash time the value is already in memory, so it no longer caps anything an attacker controls. What does amplify at hash time is a large negative scale, where a 13-character `BigDecimal` expands to a two-billion-digit `BigInteger`. This guards that directly, and falls back to a hash of the canonical form. The branch is taken on integer-digit count (`precision() - scale()`), which is representation-invariant, so `1E+200000` and the same number written out in full still hash alike. ### 2. + 3. Out-of-range reported inconsistently The two magnitude regimes diverged: ``` XmlInt.setBigDecimalValue(1E+20) -> XmlValueOutOfRangeException XmlInt.setBigDecimalValue(2000-digit) -> IllegalArgumentException (leaked from MathUtil) ``` Same for `XmlLong`, `XmlInteger`, and `XmlObjectBase.getBigIntegerValue()`. The lexical path already reports `XmlValueOutOfRangeException` for these (see `MaxNumberOfCharsTest`); only the programmatic `setBigDecimalValue`/`getBigIntegerValue` path leaked the raw type. Translated once in `XmlObjectBase.to_BigInteger()`. Since `XmlValueOutOfRangeException extends IllegalArgumentException`, existing assertions are unaffected. ### 4. `GDurationBuilder` narrower than `GDateBuilder` — `GDurationBuilder.normalize()` Identical whole-second carry computation, but `GDurationBuilder` used `toInt` while `GDateBuilder._normalizeTime` uses `toLong` — into a `long carry` in both. `setFraction` is unvalidated, so: ``` GDurationBuilder.normalize(), fraction=1E+10 -> IllegalArgumentException: Value can't be converted to int GDurationBuilder.normalize(), fraction=1E+3 -> ok ``` `GDateBuilder` accepts that magnitude. Now both use `toLong`. ### Not addressed here - Several call sites still hardcode `DEFAULT_MAX_NUMBER_CHARS` instead of the configured limit, so `XmlOptions.setMaxNumberOfCharsForNumbers` doesn't reach them: `JavaIntHolderEx:160`, `JavaLongHolderEx:160`, `JavaIntegerHolderEx:163`, `GDate:278`, `GDuration:136`, `StscTranslator:1515`, `SampleXmlUtil:412,442`. - `MathUtil.parseAsInt` is the only parse method with no `maxNumberOfChars` overload (and its 1024-char guard is moot, since `Integer.parseInt` rejects anything past ~11 chars). - An alternative to the fix in 1 would be to apply the limit in `JavaDecimalHolder.set_BigDecimal`/`JavaIntegerHolder.set_BigInteger` so an oversized value can never be stored, letting `value_hash_code` keep the cap. That's a broader change to `setBigDecimalValue` semantics, so it's left out of this PR. ### Tests Four in `MaxNumberOfCharsTest` (hash doesn't throw and stays aligned with `XmlInteger`; hash is scale-independent across the expansion threshold; a huge exponent isn't expanded; the integral setters report `XmlValueOutOfRangeException`) and one in `GDateTests` for the duration carry. Full suite: 3080 tests, 0 failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
