On Wed, 22 Apr 2026 21:52:52 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 >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8377568 src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 615: > 613: if (i >= size) { > 614: throw new ArrayIndexOutOfBoundsException("Invalid index > (offset+i) is " + > 615: "(" + offset + " + " + i + ") which is too large for > size : " + size); The message is out of sync with a check ` i>=size?` src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 623: > 621: throw new ArrayIndexOutOfBoundsException("Index cannot be > negative : " + i); > 622: } > 623: if ((offsets[bank] + i) < i) { Should the bank be validated first? or it is assumed always correct in this place? src/java.desktop/share/classes/java/awt/image/DataBufferFloat.java line 255: > 253: * @throws ArrayIndexOutOfBoundsException if {@code bank} is not a > valid bank index. > 254: */ > 255: public float[] getData(int bank) { It looks like getData(int bank) in all classes are not covered by the tests? src/java.desktop/share/classes/java/awt/image/DataBufferFloat.java line 369: > 367: * @return The data entry as a {@code float}. > 368: * @throws ArrayIndexOutOfBoundsException if {@code bank} is not a > valid bank index, > 369: * or {@code (i + getOffsets(bank))} is not a valid index > into the bank. For `public void setElem(int bank, int i, int val) { ` above it is documented as: `or {@code (i + getOffsets()[bank])} is not a valid index into the bank` test/jdk/java/awt/image/DataBuffer/DataBufferConstructorTest.java line 543: > 541: static boolean failed = false; > 542: > 543: static final int[] nullOffsets = null; nullOffsets is unsed? test/jdk/java/awt/image/DataBuffer/DataBufferConstructorTest.java line 659: > 657: final int[] zeroIntArray = { }; > 658: final int[] oneIntArray = { 0 } ; > 659: final int[] tenIntArray = new int[100]; is it expected "tenXX" vs "100"? test/jdk/java/awt/image/DataBuffer/DataBufferConstructorTest.java line 662: > 660: final int[] oneKIntArray = new int[1000]; > 661: final int[][] nullIntArrays = null; > 662: final int[][] nullIntSubArrays = { nullIntArray } ; Is "nullIntSubArrays"/nullxxx.. truly unused or some checks are missed in the test? test/jdk/java/awt/image/DataBuffer/DataBufferConstructorTest.java line 682: > 680: testIntConstructor(oneIntArray, 2, > IllegalArgumentException.class); > 681: > 682: // DataBufferInt(byte[] dataArray, int size, int offset) Copy paste error? should be int[]? test/jdk/java/awt/image/DataBuffer/DataBufferGetSetElemTest.java line 66: > 64: test(dbu, size); > 65: test(dbu, -1); > 66: dbu = new DataBufferShort(buf, size-1, 1); Should be "DataBufferUShort"? test/jdk/java/awt/image/DataBuffer/DataBufferGetSetElemTest.java line 117: > 115: } > 116: try { > 117: db.getElem(1, index); should we cover invalid/valid bank together with valid/invalid index? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3150048813 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3149986086 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3149950727 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3149980222 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3150001278 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3150077221 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3149937270 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3150072366 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3149944078 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3149969221
