RFR: 8299684: (bf) JNI direct buffer functions with large capacity behave unexpectedly [v9]

[Alan Bateman]

On Thu, 12 Jan 2023 00:15:45 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

>> Remove cast in `JNI::NewDirectByteBuffer`of `long` capacity to `int`, modify the constructor in question to accept a `long` capacity, and verify in the constructor that the  capacity does not overflow `int` range, throwing IAE If it does.
>
> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8299684: Remove spurious file

The test looks much better now but I think can be improved a bit more.

test/jdk/java/nio/jni/NewDirectByteBuffer.java line 28:

> 26: import org.junit.jupiter.api.Assertions;
> 27: import org.junit.jupiter.params.ParameterizedTest;
> 28: import org.junit.jupiter.params.provider.ValueSource;

Up to you but in other tests, we import static org.junit.jupiter.api.Assertions.* so that the asserts are simple assertXXX calls.

test/jdk/java/nio/jni/NewDirectByteBuffer.java line 36:

> 34:  * IllegalArgumentException if the capacity is negative or greater than
> 35:  * Integer::MAX_VALUE
> 36:  * @requires (sun.arch.data.model == "64" & os.maxMemory >= 8g)

The @summary should probably be updated to say that it's a unit test for JNI NewDirectBufferBuffer.

test/jdk/java/nio/jni/NewDirectByteBuffer.java line 53:

> 51:             "Buffer::position returned unexpected value");
> 52:         Assertions.assertEquals(capacity, buf.limit(),
> 53:             "Buffer::limit returned unexpected value");

For completeness, I think checkBuffer should also set/get some elements.

test/jdk/java/nio/jni/NewDirectByteBuffer.java line 77:

> 75:             }
> 76:         } catch (OutOfMemoryError ignored) {
> 77:             // Ignore the error

If I read this correctly, this frees the backing memory before it tests the buffer methods. This may be safe because checkBuffer doesn't access the buffer contents but if the checks are expanded then this will crash.

I think you should be able to reduce this method down to this:


ByteBuffer buf = newDirectByteBuffer(capacity);
try {
    checkBuffer(buf, capacity);
} finally {
    freeDirectByteBufferMemory(buf);
}

test/jdk/java/nio/jni/NewDirectByteBuffer.java line 83:

> 81:     @ParameterizedTest
> 82:     @ValueSource(longs = {Long.MIN_VALUE, (long)Integer.MIN_VALUE - 1L, -1L,
> 83:         (long)Integer.MAX_VALUE + 1L, 3_000_000_000L, 5_000_000_000L,

Just curious why 3_000_000_000L and 5_000_000_000L are in the values to test.

test/jdk/java/nio/jni/NewDirectByteBuffer.java line 93:

> 91:                 }
> 92:             });
> 93:         } catch (OutOfMemoryError ignored) {

Why is there is catch of OOME here?

-------------

PR: https://git.openjdk.org/jdk/pull/11873