On Fri, 20 Feb 2026 01:52: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 I haven't viewed all the files, my comments are applicable to most of the classes. Some of the checks could be moved into more helper methods. The constructors for the specific data types are updated, however, some checks could be performed in the super class, and likely the constructors of the super class, `DataBuffer` need also document that an exception could be thrown. The `getElem` and `setElem` method in `DataBuffer` should specify an exception will occur if the parameters are invalid. Otherwise, it doesn't look right to me. All the specific implementations throw exception, but the super class doesn't specify them. src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 544: > 542: } > 543: > 544: void checkIndex(int i) { Should both `checkIndex(int i)` and `checkIndex(int bank, int i)` be declared as `final`? Neither is supposed to be overridden. src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 545: > 543: > 544: void checkIndex(int i) { > 545: if ((i + offset) >= size) { Is there ever a possibility for integer overflow? Should we care? src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 554: > 552: if ((i + offsets[bank]) >= size) { > 553: throw new ArrayIndexOutOfBoundsException("Invalid index > (bankOffset+i) is " + > 554: "(" + offsets[bank] + " + " + i + ") which is too large > for size : " + size); Now, the `if` statement is indented by 2 spaces instead of 4. src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 93: > 91: * @param numBanks The number of banks in the {@code DataBuffer}. > 92: * @throws IllegalArgumentException if {@code size} is less than or > equal to zero. > 93: * or {@code numBanks} is less than one. Suggestion: * @throws IllegalArgumentException if {@code size} is less than or equal to zero, * or {@code numBanks} is less than one. src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 134: > 132: } > 133: if (size <= 0 || size > dataArray.length) { > 134: throw new IllegalArgumentException("Bad size : " + size); Is there a reason for a space *before* the colon? src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 167: > 165: throw new NullPointerException("Null dataArray"); > 166: } > 167: if (size <= 0 || (size + offset) > dataArray.length) { Could it ever happen that `size + offset` overflows such that the condition becomes `false` and the parameters are accepted as valid? src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 193: > 191: * @throws IllegalArgumentException if {@code dataArray} does not > have at least one bank. > 192: * @throws NullPointerException if any bank of {@code dataArray} is > {@code null}. > 193: * or {@code (offset + size)} is greater than the length of > {@code dataArray} Suggestion: * @throws IllegalArgumentException if {@code dataArray} does not have at least one bank. * @throws NullPointerException if any bank of {@code dataArray} is {@code null}. It looks as if a copy-paste mistake as `(offset + size) > dataArray.length` doesn't apply. src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 195: > 193: * or {@code (offset + size)} is greater than the length of > {@code dataArray} > 194: * @throws IllegalArgumentException if the length of any bank of > {@code dataArray}. > 195: * is less than {@code size}. Suggestion: * @throws IllegalArgumentException if the length of any bank of {@code dataArray} * is less than {@code size}. It's one sentence, isn't it? src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 244: > 242: * @throws IllegalArgumentException if {@code dataArray} does not > have at least one bank. > 243: * @throws NullPointerException if {@code offsets} is {@code null}. > 244: * @throws ArrayIndexOutOfBoundsException if the lengths of {@code > dataArray} and {@code offsets} differ. The code throws `IllegalArgumentException` if `dataArray.length > offsets.length` instead of AIOOBE. src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 247: > 245: * @throws NullPointerException if any bank of {@code dataArray} is > {@code null}. > 246: * @throws IllegalArgumentException if the length of any bank of > {@code dataArray}. > 247: * is less than ({@code size} + offsets[bankIndex]). Suggestion: * @throws IllegalArgumentException if the length of any bank of {@code dataArray} * is less than {@code (size + offsets[bankIndex])}. src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 259: > 257: if (dataArray.length == 0) { > 258: throw new IllegalArgumentException("Must have at least one > bank"); > 259: } The first three conditions are the same as in the constructor above. Are they worth moving into a static helper method? src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 95: > 93: if (numBanks < 1) { > 94: throw new IllegalArgumentException("Must have at least one > bank"); > 95: } Why can't these checks be performed in the super class, `DataBuffer`? I see the same statements here, in `DataBufferDouble`, and in `DataBufferByte`: https://github.com/prrace/jdk/blob/f17761b4b0ced616734825d486d6a3250c81c24e/src/java.desktop/share/classes/java/awt/image/DataBufferByte.java#L95-L102 src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 126: > 124: if (dataArray == null) { > 125: throw new NullPointerException("Null dataArray"); > 126: } Can we drop the explicit `null`-check? If the `dataArray` parameter is `null`, getting the length of the array in the following statement will result in NPE. src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 129: > 127: if (size <= 0 || size > dataArray.length) { > 128: throw new IllegalArgumentException("Bad size : " + size); > 129: } A static helper method `checkArraySize` would avoid duplicating validation of the `dataArray` size between different data types: checkArraySize(size, dataArray.length); where static void checkArraySize(int size, int dataArrayLength) { if (size <= 0 || size > dataArrayLength) { throw new IllegalArgumentException("Bad size : " + size); } } is in the `DataBuffer` class. src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 332: > 330: */ > 331: public int getElem(int i) { > 332: checkIndex(i); Shouldn't the `DataBuffer.getElem` method also specify `@throws ArrayIndexOutOfBoundsException`? src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 365: > 363: */ > 364: public void setElem(int i, int val) { > 365: checkIndex(i); Shouldn't the `DataBuffer.setElem` method also specify `@throws ArrayIndexOutOfBoundsException`? ------------- Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/29766#pullrequestreview-3832237800 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834041275 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2833935983 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2833933747 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2833671481 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2833919036 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2833944794 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2833982271 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2833984032 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834014142 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834016112 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834008457 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834060976 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834173709 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834162048 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834208733 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834212839
