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

[Brian Burkhalter]

On Thu, 12 Jan 2023 10:17:04 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8299684: Remove spurious file
>
> 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.

Fixed in 2fe819ba1fd929eaaa6cc31caee38e217006c3f0.

> 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.

So changed in 2fe819ba1fd929eaaa6cc31caee38e217006c3f0.

> 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.

Tests added in 2fe819ba1fd929eaaa6cc31caee38e217006c3f0.

> 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);
> }

So changed in 2fe819ba1fd929eaaa6cc31caee38e217006c3f0.

> 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.

They were in the initial bug report.

> test/jdk/java/nio/jni/NewDirectByteBuffer.java line 93:
> 
>> 91:                 }
>> 92:             });
>> 93:         } catch (OutOfMemoryError ignored) {
> 
> Why is there is catch of OOME here?

Apparently it is not needed so removed in 38ea7997be2be59e2cea97c7dabf929210c0edfa.

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

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