On Wed, 29 Apr 2026 22:14:46 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 Changes requested by aivanov (Reviewer). src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 613: > 611: "(" + offset + " + " + i + ") = " + (offset + i)); > 612: } > 613: if (i >= size) { Does it make sense to add a similar comment here? // Don't need to include bank offset here since all constructors validated // the offset for each bank against the size. To avoid the confusion, that `offset` is missed. However, it could be not a problem any more because the exception message doesn't mention _`offset`_ now. src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 653: > 651: if (numBanks <= 0) { > 652: throw new IllegalArgumentException("Must have at least one > bank"); > 653: } Suggestion: if (numBanks < 1) { throw new IllegalArgumentException("Must have at least one bank"); } The condition will literally match the exception message. test/jdk/java/awt/image/DataBuffer/DataBufferConstructorTest.java line 41: > 39: > 40: static void testByteConstructor(int size, > 41: Class expectedExceptionType) { Suggestion: Class<?> expectedExceptionType) { To silence IDE warnings about raw use of parameterized class Class? Or add `@SuppressWarnings("rawtypes")` to the test class? test/jdk/java/awt/image/DataBuffer/DataBufferConstructorTest.java line 543: > (failed to retrieve contents of file, check the PR for context) If `nullOffsets` was unused, you had missed adding a test that passes `nullOffsets` as parameter to a constructor that accepts an array of offsets. For example, this block at lines 589–597: // DataBufferByte(byte[][] dataArray, int size, int[] offsets) Other types are also affected. test/jdk/java/awt/image/DataBuffer/DataBufferConstructorTest.java line 557: > (failed to retrieve contents of file, check the PR for context) Why is `nullByteSubArrays` removed? If it was unused, a test for the case where dataArrays[][] contains a bank that's null was missing. This applies for other types, for example, `nullShortSubArrays` is unused. ------------- PR Review: https://git.openjdk.org/jdk/pull/29766#pullrequestreview-4207781978 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3170242118 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3170284853 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3170314604 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3170221164 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r3170189450
