On Mon, 2 Mar 2026 22:59:11 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 looked through `DataBuffer` and `DataBufferByte` only…
src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 559:
> 557:
> 558: // Checks used by subclass constructors.
> 559: static final void checkSize(int size) {
Do we really need the `final` modifier for a static method? Static methods
aren't inherited.
src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 579:
> 577: static final void checkArraySize(int size, int offset, int arrayLen)
> {
> 578: if (size <= 0 || (size + offset) > arrayLen ||
> 579: (offset > 0) && ((size + offset) < size)) {
Should the condition `(size + offset) > arrayLen` also ensure `offset > 0`?
Suggestion:
if (size <= 0 || offset < 0 || (size + offset) > arrayLen ||
(size + offset) < size) {
src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 587:
> 585: static final void checkBankSize(int bank, int size, int offset, int
> arrayLen) {
> 586: if ((arrayLen < (size + offset)) ||
> 587: ((offset > 0) && ((size + offset) < size))) {
If `offset` is negative, the condition `(arrayLen < (size + offset))` will give
unexpected result.
src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 75:
> 73: *
> 74: * @param size The size of the {@code DataBuffer}.
> 75: * @throws IllegalArgumentException if {@code size} is less than or
> equal to zero.
Why doesn't `DataBuffer(int size)` declare the same `IllegalArgumentException`?
It would be great to use `{@inheritDoc}` to pull in the description from super
constructors, but it doesn't work.
src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 79:
> 77: public DataBufferByte(int size) {
> 78: super(STABLE, TYPE_BYTE, size);
> 79: checkSize(size);
Why not move the call to `checkSize` into the superclass constructor of
`DataBuffer`?
Any data buffer of `size <= 0` doesn't make sense, then you won't have to call
`checkSize` explicitly in all the classes.
src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 97:
> 95: super(STABLE, TYPE_BYTE, size, numBanks);
> 96: checkSize(size);
> 97: checkNumBanks(numBanks);
The same comment: both `checkSize` and `checkNumBanks` could be called in the
superclass constructor and will handle all the data types automatically.
src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 188:
> 186: checkNumBanks(dataArray.length);
> 187: for (int b = 0; b < dataArray.length; b++) {
> 188: Objects.requireNonNull(dataArray, "bank must not be null");
Suggestion:
Objects.requireNonNull(dataArray[b], "bank must not be null");
Does it make sense to indicate the index of the bank in the error message?
src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 220:
> 218: * @throws NullPointerException if any bank of {@code dataArray} is
> {@code null}.
> 219: * @throws IllegalArgumentException if the length of any bank of
> {@code dataArray}
> 220: * is less than ({@code size} + offsets[bankIndex]).
Suggestion:
* is less than {@code (size + offsets[bankIndex])}.
src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 232:
> 230: }
> 231: for (int b = 0; b < dataArray.length; b++) {
> 232: Objects.requireNonNull(dataArray, "bank must not be null");
Suggestion:
Objects.requireNonNull(dataArray[b], "bank must not be null");
src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 269:
> 267: theTrackable.setUntrackable();
> 268: return bankdata[bank];
> 269: }
No custom message here? AIOOBE is produced by JVM.
src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 297:
> 295: public int getElem(int i) {
> 296: checkIndex(i);
> 297: return (int)(data[i+offset]) & 0xff;
It will thrown AIOOBE without explicit `checkIndex`, you want a custom error
message, right?
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/29766#pullrequestreview-3884114868
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880016990
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2879629676
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2879644926
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2879710595
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2879681310
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2879691531
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880695866
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880711857
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880714962
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880727887
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880733019
