On Mon, 23 Mar 2026 22:15:56 GMT, Phil Race <[email protected]> wrote:
>> This fix updates DataBuffer subclasses to actually adhere to their stated >> specifications by rejecting certain invalid parameters for constructors and >> getters and setters. >> A new egression test for each of the constructor and getter/setter cases is >> supplied. >> >> No existing regression tests fail with this change, and standard demos work. >> >> Problems caused by these changes are most likely to occur if the client has >> a bug such that >> - a client uses the constructors that accept an array and then supplies a >> "size" that is greater than the array. >> - a client uses the constructors that accept an array and then supplies a >> "size" that is less than the array and then uses getter/setters that are >> within the array but outside the range specified by size. >> >> Since very few clients (and just one case in the JDK that I found) even use >> these array constructors the changes are unlikely to make a difference to >> clients. >> >> The CSR is ready for review https://bugs.openjdk.org/browse/JDK-8378116 > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8377568 Changes requested by aivanov (Reviewer). src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 546: > 544: final void checkBank(int bank) { > 545: if (bank < 0 || bank >= banks) { > 546: throw new ArrayIndexOutOfBoundsException("Bank index out of > range " + bank); Suggestion: throw new ArrayIndexOutOfBoundsException("Bank index out of range : " + bank); For consistency with other messages. src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 606: > 604: ((offset + size) <= 0) || > 605: ((offset + size) > arrayLen) || > 606: ((offset > 0) && ((offset + size ) < size)); The last condition—`((offset > 0) && ((offset + size ) < size))`— can never be true because `((offset + size) <= 0)` would yield true if the sum overflows. By the time this third condition is reached, `offset >= 0` and `size > 0`, if the third condition yields false, then the last condition is bound to yield false too. src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 122: > 120: * @throws NullPointerException if {@code dataArray} is {@code null}. > 121: * @throws IllegalArgumentException if {@code size} is less than or > equal > 122: * to zero, or is greater than the length of {@code > dataArray}. The other subclasses except for `-Byte`, for example [`-Double`](https://github.com/openjdk/jdk/pull/29766/changes#diff-04543d1d9a796a3ba88b6fe9db5d132fac5c049d20e942c8431706811dbfa5cbR116) and [`-Float`](https://github.com/openjdk/jdk/pull/29766/changes#diff-8c62d99e15db8b04beec405481ad27a2c0d724eb5c4a0b9ffbaa49c97ed96d48R117), omit _“is”_: https://github.com/openjdk/jdk/blob/a4d19dde137d5d9019acdc30bcb863c28b77fc19/src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java#L116-L117 The other classes also use slightly different formatting. I prefer the version with “is”, it's clearer, as well as formatting here in the `-Byte` class. src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 152: > 150: * @throws IllegalArgumentException if {@code size} or {@code offset} > 151: * is less than or equal to zero, or {@code (offset + size)} > 152: * is greater than the length of {@code dataArray}. This description is confusing because it implies `IllegalArgumentException` is thrown if * `size <= 0`, or * **`offset <= 0`**, but `offset == 0` is valid. src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 117: > 115: * @throws NullPointerException if {@code dataArray} is {@code null}. > 116: * @throws IllegalArgumentException if {@code size} is less than or > equal to zero, > 117: * or greater than the length of {@code dataArray}. Suggestion: * @throws IllegalArgumentException if {@code size} is less than or equal * to zero, or is greater than the length of {@code dataArray}. To align with the version in `DataBufferByte.java`. https://github.com/openjdk/jdk/pull/29766/changes#r3066076500 src/java.desktop/share/classes/java/awt/image/DataBufferFloat.java line 375: > 373: * @return The data entry as a {@code float}. > 374: * @throws ArrayIndexOutOfBoundsException if {@code bank} is not a > valid bank index, > 375: * or {@code (i + getOffsets(bank)}} is not a valid index > into the bank. Suggestion: * @throws ArrayIndexOutOfBoundsException if {@code bank} is not a valid bank index, * or {@code (i + getOffsets()[bank])} is not a valid index into the bank. src/java.desktop/share/classes/java/awt/image/DataBufferShort.java line 216: > 214: * @throws NullPointerException if {@code offsets} is {@code null}. > 215: * @throws IllegalArgumentException if any element of {@code > offsets} is less than zero. > 216: * @throws IllegalArgumentException if the lengths of {@code > dataArray} and {@code offsets} differ. Suggestion: * @throws ArrayIndexOutOfBoundsException if the lengths of {@code dataArray} and {@code offsets} differ. The superclass throws `ArrayIndexOutOfBoundsException` in this case. src/java.desktop/share/classes/java/awt/image/DataBufferUShort.java line 83: > 81: if (size <= 0) { > 82: throw new IllegalArgumentException("Size must be > 0"); > 83: } The `if` condition is redundant. ------------- PR Review: https://git.openjdk.org/jdk/pull/29766#pullrequestreview-4091650147 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3066105493 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3066007831 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3066076500 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3066166472 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3066082920 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3066267397 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3066092291 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3066310059
