RFR: JDK-8309550: jdk.jfr.internal.Utils::formatDataAmount method should gracefully handle amounts equal to Long.MIN_VALUE [v2]

[Frederic Thevenet]

On Wed, 7 Jun 2023 10:07:18 GMT, Frederic Thevenet <fthevenet at openjdk.org> wrote:

>> Please review this simple fix to JDK-8309550.
>> 
>> NB: The problem originally reported only concerned `src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java`, but since this code was duplicated in `src/jdk.jfr/share/classes/jdk/jfr/internal/util/ValueFormatter.java` with, as far as I understood from the bug description, with an intent to refactor it a a later time, I opted to change both occurrences of the method.
>
> Frederic Thevenet has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Put brackets around the ? condition

I actually took a closer look at one those call site, namely the one that shows up in the stack you can reproduce from the elements in the JBS bug:
``` java
java.lang.StringIndexOutOfBoundsException: Index -1 out of bounds for length 6
        at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:55)
        at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:52)
        at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:213)
        at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:210)
        at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:98)
        at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:106)
        at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:302)
        at java.base/java.lang.String.checkIndex(String.java:4828)
        at java.base/java.lang.StringLatin1.charAt(StringLatin1.java:46)
        at java.base/java.lang.String.charAt(String.java:1555)
        at jdk.jfr/jdk.jfr.internal.Utils.formatDataAmount(Utils.java:127)
        at jdk.jfr/jdk.jfr.internal.Utils.formatBytes(Utils.java:155)
        at jdk.jfr/jdk.jfr.internal.tool.PrettyWriter.printFormatted(PrettyWriter.java:595)
        at jdk.jfr/jdk.jfr.internal.tool.PrettyWriter.printValue(PrettyWriter.java:357)
        at jdk.jfr/jdk.jfr.internal.tool.PrettyWriter.printFieldValue(PrettyWriter.java:276)
        at jdk.jfr/jdk.jfr.internal.tool.PrettyWriter.print(PrettyWriter.java:214)
        at jdk.jfr/jdk.jfr.internal.tool.PrettyWriter.print(PrettyWriter.java:75)
        at jdk.jfr/jdk.jfr.internal.tool.EventPrintWriter.print(EventPrintWriter.java:82)
        at jdk.jfr/jdk.jfr.internal.tool.Print.execute(Print.java:165)
        at jdk.jfr/jdk.jfr.internal.tool.Main.main(Main.java:92) 

That initially got me very confused as I couldn't understand why `PrettyWriter.printFormatted` from `PrettyWritter.printValue` would get called from `PrettyWritter.printValue` as it should have been cut short by the [check line 343](https://github.com/openjdk/jdk/blob/master/src/jdk.jfr/share/classes/jdk/jfr/internal/tool/PrettyWriter.java#L343-L348).

It turns out that this is because in that specific case at least, the value passed to printValue is actually a `BigDecimal` instance, which jumps over the check. 
Latter on in `printFormatted`, that same argument is cast as a `Number` instance, and a `long` value derived from it, triggering the issue: https://github.com/openjdk/jdk/blob/b5b5b7ce7220df650f6142c40d6e89c0462877ce/src/jdk.jfr/share/classes/jdk/jfr/internal/tool/PrettyWriter.java#L582

All this to say that a follow-up is indeed needed to more it more robust, but that for now the check directly in the `formatDataAmount ` method is useful as an immediate from of protection. 
However, because this may involve changes in several places, it might be wise to only undertake that after the large refactor initiated by [JDK-8306703](https://bugs.openjdk.org/browse/JDK-8306703) is finished? Wdyt?

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

PR Comment: https://git.openjdk.org/jdk/pull/14341#issuecomment-1589809268