8358537: (bf) JavaNioAccess::getBufferAddress bypasses direct buffer MemorySession state check

[bplb]

Add package-scope long java.nio.Buffer::address() to simply return the value of the address instance variable; use this method in JavaNioAccess::getBufferAddress.

---

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

Issue

• JDK-8358537: (bf) JavaNioAccess::getBufferAddress bypasses direct buffer MemorySession state check (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25631

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bplb]

Buffer::address is overridden for heap buffers to throw an UnsupportedOperationException. The MemorySegments test is modified to remove the use of shared Arenas which cause UnsupportedOperationExceptions to be thrown. The jdk_core tests have no failures attributable to these changes.

[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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

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

• nio

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

[mlbridge]

Webrevs

• 01: Full - Incremental (7e6c05ad)
• 00: Full (7c2f0130)

[JornVernee]

Discussed this with the rest of the Panama team. The way it works currently (without this PR) is how it's intended to work.

The issue is that JDK code can take the address of a direct byte buffer and pass it to some native method, but if the buffer was backed by a shared arena, the memory can be freed in the middle of that. That's why the 'default' implementation in DirectBuffer::address throws an exception. Code that wants to use the address of a direct buffer backed by a shared arena first has to acquire the arena, then get the address, and after it is done using the address, release the arena again. The second step requires a way to bypass the check. i.e. the shared secrets implementation intentionally accesses the field directly.

As for heap buffers: The base object and address form an Unsafe addressing pair. i.e. these are passed to Unsafe accessor methods. For heap buffers this would be the array base offset + any offset into the array (in bytes). The implementation of e.g. MemorySegment::ofBuffer uses both to construct a memory segment, so again, the address needs to be accessible. But, since the implementation uses shared secrets to access the address, as long as that method keeps bypassing the address() accessor method, I think this will continue to work.

[bplb]

Discussed this with the rest of the Panama team. The way it works currently (without this PR) is how it's intended to work.

Thank you for the elucidation: that is very helpful.

Perhaps some comments need to be added in strategic places to clarify the situation to avoid similar confusion in the future? This PR arose out of #25531, in particular when it was suggested to access the address of the direct buffer via IOUtil.bufferAddress which calls JavaNioAccess.getBufferAddress. A failure was expected for buffers created from shared Arenas but none occurred, and it looked like there had been an oversight in the buffer address access protocol.

[bplb]

Perhaps some comments need to be added in strategic places to clarify the situation to avoid similar confusion in the future?

It would be all right to repurpose this PR and its issue simply to add such comments.

[AlanBateman]

Perhaps some comments need to be added in strategic places to clarify the situation to avoid similar confusion in the future?

I think we should start by reviewing the test coverage. The channels/etc/MemorySegments.java test that we added will exercise all the methods on the synchronous channels that take a ByteBuffer as a parameter. We need to check what tests we have for AsynchronousSocketChannel and AsynchronousFileChannel to ensure that we are testing them with buffers that are views of memory segments allocated from an auto or the global arena. We need good tests before changing anything.

[bplb]

The test in #25531 covers AsynchronousFileChannel.

[bplb]

The corresponding issue was closed as Not an Issue so this request is being withdrawn.