RFR: 8264161: BigDecimal#stripTrailingZeros can throw undocumented ArithmeticException

[Joe Darcy]

On Thu, 25 Mar 2021 08:19:24 GMT, Fabian Meumertzheim <github.com+4312191+fmeum at openjdk.org> wrote:

> Adds missing @throws declarations for ArithmeticException to the public
> function
> java.math.BigDecimal#stripTrailingZeros
> as well as the private helper functions
> java.math.BigDecimal#createAndStripZerosToMatchScale(long, int, long)
> java.math.BigDecimal#createAndStripZerosToMatchScale(BigInteger, int, long)
> 
> stripTrailingZeros calls one of the two helper functions, both of which
> can repeatedly decrease the scale by 1 until it underflows. If it does,
> the call to checkScale will result in an ArithmeticException (overflow).

> 
> 
> > Just was we don't think it is helpful to put an explicit
> > @throws NullPointerException if a argument is null
> > on every method that throws a NPE for a null argument, I don't think it
> > is helpful or necessary to explicitly note in every BigDecimal method
> > with rounding that it could throw ArithmeticException. The general
> > class-level statement
> > ?* As a 32-bit integer, the set of values for the scale is large,
> > ?* but bounded. If the scale of a result would exceed the range of a
> > ?* 32-bit integer, either by overflow or underflow, the operation may
> > ?* throw an {@code ArithmeticException}.
> > in meant to capture the needed information.
> > -Joe
> 
> I do agree with this in general, but I think that the situation at hand is a bit different from your example for two reasons:
> 
>     * The `BigDecimal` class already contains many explicit `@throws` annotations for `RuntimeException`s. The absence of such an annotation from a particular method would thus naturally be interpreted as saying that the method does not throw.
> 
>     * For someone not intimately familiar with the internal representation of `BigDecimal`s, it is probably quite unexpected that a function called `stripTrailingZeros` would perform rounding and thus be liable to scale overflows. (This is what happened to the maintainers of jackson-core, where this lead to an unexpected RuntimeException that was only discovered via fuzzing. They simply took this function to perform some kind of "harmless" canonicalization.)
> 
> 
> That is why I do see value in adding these annotations in this particular case.

Thanks for the additional context. Fuzzers would be likely to run into an exception from stripTrailingZeros as they would be likely to start with extreme exponent values.

Looking over BigDecimal's throws clauses more closely, I've filed

    JDK-8265700: Regularize throws clauses in BigDecimal

and will send out a PR shortly. This change will add a throws clause for the unexpected exception from stripTrailingZeros and remove several "throws ArithmeticException when rounding occurs and the rounding mode is UNNECESSARY" clauses.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3189