8299684: (bf) JNI direct buffer functions with large capacity behave unexpectedly

[bplb]

Remove cast in JNI::NewDirectByteBufferof 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.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires CSR request JDK-8299759 to be approved

Issues

• JDK-8299684: (bf) JNI direct buffer functions with large capacity behave unexpectedly
• JDK-8299759: (bf) JNI direct buffer functions with large capacity behave unexpectedly (CSR)

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11873/head:pull/11873
$ git checkout pull/11873

Update a local copy of the PR:
$ git checkout pull/11873
$ git pull https://git.openjdk.org/jdk pull/11873/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11873

View PR using the GUI difftool:
$ git pr show -t 11873

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11873.diff

[bridgekeeper]

👋 Welcome back bpb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@bplb The following labels will be automatically applied to this pull request:

• hotspot
• nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 16: Full - Incremental (3b1eef25)
• 15: Full - Incremental (040374bc)
• 14: Full - Incremental (f9350695)
• 13: Full - Incremental (3b64bd20)
• 12: Full - Incremental (abc6b586)
• 11: Full - Incremental (7d526352)
• 10: Full - Incremental (38ea7997)
• 09: Full - Incremental (2fe819ba)
• 08: Full - Incremental (5269cae1)
• 07: Full - Incremental (7f99faee)
• 06: Full - Incremental (95fd6652)
• 05: Full - Incremental (82d9e6f1)
• 04: Full - Incremental (d2386a4b)
• 03: Full - Incremental (d8e6c180)
• 02: Full - Incremental (a9e44d62)
• 01: Full - Incremental (db924a34)
• 00: Full (8baa0fbb)

[PaulSandoz]

Requires an update to the JNI spec and therefore a CSR?

[bplb]

Requires an update to the JNI spec and therefore a CSR?

Yes, I think both.

[dholmes-ora]

I think it will look strange to update the JNI spec to say that although the function takes a jlong as the requested capacity, it will throw IAE if the capacity is larger than a jint! The mismatch between the JNI and Java code made me dig into the history here to see what JSR-51 had to say about this, but unfortunately it didn't say much. What I did find in

https://bugs.openjdk.org/browse/JDK-4496703

was a very definitive comment about only supporting 32-bit (ie int) based direct buffers, but with a glimmer of hope

"A future revision of the specification is likely to address this problem".

So perhaps the JNI side was prepared for this unrealized future? But even so it should have specified what happens if the jlong capacity exceeds the value of a jint

I thought perhaps a special OutOfMemoryError (akin to the "Requested array size exceeds VM limit" OOME) could be thrown - and that would not require a JNI spec change - but it is a stretch. Though also note that if this requires a JNI spec change then it cannot be backported without jumping through some serious JCP Maintenance Release hoops.

[AlanBateman]

I checked ancient history and the issue goes back to JDK 1.4 (when JSR-51 defined this API). It's surprising that it hasn't been reported/noticed before now.

Yes, this will need an update to the JNI spec to say that it throws if capacity is negative or greater than Integer.MAX_VALUE. It will look a bit strange in the spec when the parameter is jlong but we can't change it. As regards what to throw then the current proposal to throw IAE seems okay as the function already throws it for negative values that are >= Integer.MIN_VALUE, e.g.

Exception in thread "main" java.lang.IllegalArgumentException: capacity < 0: (-1 < 0)
	at java.base/java.nio.Buffer.createCapacityException(Buffer.java:282)
	at java.base/java.nio.Buffer.<init>(Buffer.java:245)
	at java.base/java.nio.ByteBuffer.<init>(ByteBuffer.java:298)
	at java.base/java.nio.ByteBuffer.<init>(ByteBuffer.java:306)
	at java.base/java.nio.MappedByteBuffer.<init>(MappedByteBuffer.java:113)
	at java.base/java.nio.DirectByteBuffer.<init>(DirectByteBuffer.java:177)
	at Test.newDirectByteBuffer(Native Method)
	at Test.main(Test.java:7)

The compatibility impact of the change should be minimal, at least I can't imagine anyone depending on the current broken behavior.

For the change, clampCapacity only needs to be called once, not twice. The exception message is a bit strange. If cap is < Integer.MIN_VALUE then it would be better to have it consistent with the existing message above. Also "in upcall from JNI NewDirectByteBuffer" should be better if it didn't include "in upcall".

We will need to add a test for this and decide where to place it. It seems there aren't any tests for these JNI functions in hotspot//jtreg/runtime/jni, maybe we start a new locations for tests in test/jdk/java/nio/jni.

[AlanBateman]

/csr

[openjdk]

@AlanBateman has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@bplb please create a CSR request for issue JDK-8299684 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[AlanBateman]

I agree with David that it would be clearer and simpler to check if capacity is negative or greater than Integer.MAX_VALUE. There other helper methods called from this code and they are named checkXXX so it would be good to keep that consistent if you can.

We don't have other tests for this JNI function in the test tree so having the new tests be a complete unit test would be good. This means checking the position, limit, and capacity of the ByteBuffer and testing that it's a direct buffer. If you make it a JUnit test then you'll be able to paramatrized test with value sources, and use the assertions API when checking each of the buffer properties.

For the native code used by the test then it would be clearer if allocBigBuffer were renamed to newDirectByteBuffer. Also I think you'll need to check the value from malloc in case it fails. You'll need another native function to free the memory and have the tests invoke the free in a finally block so that memory doesn't accumulate as the test cycles through the different buffer sizes. Right now, I assume the tests don't run on 32-bit as it mallocs two 2Gb areas. Once the test is re-worded then you can get a sense as to how much memory it needs and whether you need a @requires in the test description to ensue that the test system has sufficient memory.

[bplb]

If you make it a JUnit test then you'll be able to paramatrized test with value sources, and use the assertions API when checking each of the buffer properties.

All points addressed in d2386a4 except converting to a JUnit test.

[dholmes-ora]

Updates look good - thanks. A few minor nits.

[AlanBateman]

The update looks quite good and maybe some day this can be converted from a psvm test to a junit test.

[bplb]

The update looks quite good and maybe some day this can be converted from a psvm test to a junit test.

JUnit or TestNG?

[LanceAndersen]

The update looks quite good and maybe some day this can be converted from a psvm test to a junit test.

JUnit or TestNG?

It sounds like the suggested way forward is to use junit for new tests and for extra credit convert existing testng tests when/if they need updated

[bplb]

If you make it a JUnit test then you'll be able to paramatrized test with value sources, and use the assertions API when checking each of the buffer properties.

Converted to a JUnit test in 7f99fae.

Once the test is re-worded then you can get a sense as to how much memory it needs and whether you need a @requires in the test description to ensue that the test system has sufficient memory.

Added @requires constraining to 64-bit arch and memory at least 8GB in 7f99fae.

[AlanBateman]

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

[AlanBateman]

I think this looks okay. If you are doing any further edits then the try-finally OOME can be removed from the illegalCapacities test.

[openjdk]

@bplb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8299684: (bf) JNI direct buffer functions with large capacity behave unexpectedly

Reviewed-by: dholmes, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 277 new commits pushed to the master branch:

• 030b071: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern
• 7ced08d: 8300638: Tier1 IR Test failure after JDK-8298632 on macosx-x64-debug
• 3ea4eac: 8300817: The build is broken after JDK-8294693
• cbfc069: 8300731: Avoid unnecessary array fill after creation in PaletteBuilder
• 67b1c89: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface
• c8dd758: 8300260: Remove metaprogramming/isSame.hpp
• a6c2a2a: 8300692: GCC 12 reports some compiler warnings in bundled freetype
• bb42e61: 8300493: Use ArraysSupport.vectorizedHashCode in j.u.zip.ZipCoder
• 06394ee: 8300590: [JVMCI] BytecodeFrame.equals is broken
• 5331a3e: 8298908: Instrument Metaspace for ASan
• ... and 267 more: https://git.openjdk.org/jdk/compare/a49ccb959b7e50ee67b1ab4feca47c34bdbc14fe...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[dholmes-ora]

Nothing further from me. Thanks

[bplb]

/integrate

[openjdk]

Going to push as commit a56598f.
Since your change was applied there have been 284 commits pushed to the master branch:

• 542bfe6: 8300587: (bf) Some covariant overrides are missing @SInCE tags
• 03a9a88: 8300265: Remove metaprogramming/isSigned.hpp
• 5a4945c: 8299975: Limit underflow protection CMoveINode in PhaseIdealLoop::do_unroll must also protect type from underflow
• f307e8c: 8299795: Relativize locals in interpreter frames
• 11aadc9: 8244400: MenuItem may cache the size and did not update it when the screen DPI is changed
• 836198a: 8300591: @SuppressWarnings option "lossy-conversions" missing from jdk.compiler module javadoc
• 45e4e00: 8300079: SIGSEGV in LibraryCallKit::inline_string_copy due to constant NULL src argument
• 030b071: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern
• 7ced08d: 8300638: Tier1 IR Test failure after JDK-8298632 on macosx-x64-debug
• 3ea4eac: 8300817: The build is broken after JDK-8294693
• ... and 274 more: https://git.openjdk.org/jdk/compare/a49ccb959b7e50ee67b1ab4feca47c34bdbc14fe...master

Your commit was automatically rebased without conflicts.

[openjdk]

@bplb Pushed as commit a56598f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[iklam]

This PR is causing causing x86 build failures in GitHub actions. I filed https://bugs.openjdk.org/browse/JDK-8300942

[bplb]

This PR is causing causing x86 build failures in GitHub actions. I filed https://bugs.openjdk.org/browse/JDK-8300942

PR #12157 has been created to address this problem.