Skip to content

Fix AIOOBE in ParsedDecimal.shouldRoundUp when rounding to zero retained digits - #791

Open
Mengxi-Xu wants to merge 1 commit into
apache:masterfrom
Mengxi-Xu:fix-parsed-decimal-should-round-up-zero-count
Open

Mengxi-Xu wants to merge 1 commit into
apache:masterfrom
Mengxi-Xu:fix-parsed-decimal-should-round-up-zero-count

Conversation

@Mengxi-Xu

Copy link
Copy Markdown

Description

When round() runs with roundExponent == max, digitCount == 1 and the single digit equals ROUND_CENTER (5), shouldRoundUp(0) evaluates digits[count - 1] with count == 0, throwing ArrayIndexOutOfBoundsException: Index -1.

This PR guards the half-even tie-break with count > 0 so a zero-retained-digit tie resolves to zero (the implicit retained digit is 0, which is even) instead of reading digits[-1]. Values with a single digit greater than 5 still round up correctly.

Root cause

ParsedDecimal.round() (line 546) calls shouldRoundUp(0) in the roundExponent == max branch:

} else if (roundExponent == max && shouldRoundUp(0)) {
    setSingleDigitValue(1, roundExponent);
}

shouldRoundUp(int count) (lines 634-635) evaluates:

return digitAfterLast > ROUND_CENTER || digitAfterLast == ROUND_CENTER
        && (count < digitCount - 1 || digits[count - 1] % 2 != 0);

When count == 0 and digitCount == 1:

  • digitAfterLast = digits[0] = 5 = ROUND_CENTER, so digitAfterLast > ROUND_CENTER is false.
  • digitAfterLast == ROUND_CENTER is true, so the right-hand side is evaluated.
  • count < digitCount - 1 is 0 < 0 = false.
  • Short-circuit therefore evaluates digits[count - 1] = digits[-1] -> ArrayIndexOutOfBoundsException.

The trigger is a value of the form ±5 x 10^k with minDecimalExponent = k+1, i.e. a single significant digit of exactly 5 with all significant digits rounded away (a half-even tie with zero retained digits).

Builder.minDecimalExponent(int) documents no constraint on its argument and explicitly promises that "If the number being formatted does not contain any such digits, then zero is returned" — rounding all significant digits away is a documented, supported scenario, so the AIOOBE violates the documented contract.

Fix

Add a count > 0 guard to the half-even tie-break in shouldRoundUp:

return digitAfterLast > ROUND_CENTER || digitAfterLast == ROUND_CENTER
        && (count < digitCount - 1 || (count > 0 && digits[count - 1] % 2 != 0));

When count == 0 and digitCount == 1, the tie now resolves to zero (the implicit retained digit is 0, which is even) instead of reading digits[-1]. A single digit greater than 5 still rounds up via the digitAfterLast > ROUND_CENTER branch, and the count < digitCount - 1 short-circuit for digitCount > 1 is unchanged.

Note: an alternative fix of gating round() with digitCount > 1 && shouldRoundUp(0) was considered and rejected, because it would also send single-digit values greater than 5 (e.g. 0.7 rounded to the units place, which should produce 1) to the zero branch.

Reproducer (fails without this fix)

import org.apache.commons.text.numbers.DoubleFormat;

public class Repro {
    public static void main(String[] args) {
        // Each of these throws ArrayIndexOutOfBoundsException: Index -1
        // at ParsedDecimal.shouldRoundUp(ParsedDecimal.java:634) without the fix.
        DoubleFormat.PLAIN.builder().minDecimalExponent(1).build().apply(5.0);
        DoubleFormat.PLAIN.builder().minDecimalExponent(-39).build().apply(5e-40);
        DoubleFormat.PLAIN.builder().minDecimalExponent(0).build().apply(0.5);
    }
}

With this fix, each call returns normally (the tie resolves to zero).

…ned digits

When round() runs with roundExponent == max, digitCount == 1 and the single
digit equals ROUND_CENTER (5), shouldRoundUp(0) evaluates digits[count - 1]
with count == 0, throwing ArrayIndexOutOfBoundsException: Index -1.

Guard the half-even tie-break with count > 0 so a zero-retained-digit tie
resolves to zero (the implicit retained digit is 0, which is even) instead
of reading digits[-1]. Values with a single digit greater than 5 still round
up correctly.
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.

1 participant