8296024: Usage of DirectBuffer::address should be guarded #11260

minborg · 2022-11-21T10:53:07Z

This PR proposes the introduction of guarding of the use of DirectBuffer::address within the JDK. With the introduction of the Foreign Function and Memory access, it is possible to derive Buffer instances that are backed by native memory that, in turn, can be closed asynchronously by the user (and not only via a Cleaner when there is no other reference to the Buffer object). If another thread is invoking MemorySession::close while a thread is doing work using raw addresses, the outcome is undefined. This means the JVM might crash or even worse, silent modification of unrelated memory might occur.

Design choices in this PR:

There is already a method MemorySession::whileAlive that takes a runnable and that will perform the provided action while acquiring the underlying MemorySession (if any). However, using this method entailed relatively large changes while converting larger/nested code segments into lambdas. This would also risk introducing lambda capturing. So, instead, a ~~try-with-resources~~ try-finally friendly access method was added. This made is more easy to add guarding and did not risk lambda capturing. Also, introducing lambdas in certain fundamental JDK classes might incur bootstrap problems.

The aforementioned ~~TwR~~ TF is using a ~~"session acquisition" that is not used explicitly in the try block itself~~ session used in the finally block. ~~This raises a warning that is suppressed using @SuppressWarnings("try"). In the future, we might be able to remove these suppressions by using the reserved variable name _.~~

In some cases, where is is guaranteed that the backing memory session is non-closeable, we do not have to guard the use of DirectBuffer::address. ~~These cases have been documented in the code.~~

On some occasions, a plurality of acquisitions are made. This would never lead to deadlocks as acquisitions are fundamentally concurrent counters and not resources that only one thread can "own".

I have added comments (and not javadocs) before the declaration of the non-public-api DirectBuffer::address method, that the use of the returned address needs to be guarded. It can be discussed if this is preferable or not.

This PR spawns several areas of responsibility and so, I expect more than one reviewer before promoting the PR.

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-8296024: Usage of DirectBuffer::address should be guarded

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to 619473ee
Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 619473ee
Paul Sandoz (@PaulSandoz - Reviewer) ⚠️ Review applies to 619473ee
Brian Burkhalter (@bplb - Reviewer) ⚠️ Review applies to 619473ee

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11260

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

Using diff file

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

bridgekeeper · 2022-11-21T10:54:22Z

👋 Welcome back pminborg! 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 · 2022-11-21T10:56:43Z

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

core-libs
net
nio
security

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 · 2022-11-21T11:00:12Z

Webrevs

AlanBateman · 2022-11-21T11:23:54Z

src/java.base/share/classes/java/nio/Buffer.java

+                // We are not using a lambda here because this would create a circular dependency with lambda factories.
+                static final JavaNioAccess.SessionAcquisition NO_OP_CLOSE = new JavaNioAccess.SessionAcquisition() {
+                    @Override
+                    public void close() throws RuntimeException {}


The throws RuntimeException is not needed here. Also would it be possible to reflow the comment to avoid the wildly long line.

AlanBateman · 2022-11-21T11:25:13Z

src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java

+    interface SessionAcquisition extends AutoCloseable {
+
+        @Override
+        void close() throws RuntimeException;


You can drop throws RuntimeEXcepton here too.

AlanBateman · 2022-11-21T11:26:01Z

src/java.base/share/classes/sun/nio/ch/DirectBuffer.java

+    // An example of a guarded use of a memory address is shown here:
+    // try (var sessionAcquisition = NIO_ACCESS.acquireSessionAsAutoCloseable(bb)) {
+    //     performOperation(bb.address());
+    // }


This comment is very messy and needs to be cleaned up.

AlanBateman · 2022-11-21T11:29:07Z

src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java

@@ -248,6 +248,8 @@ private int tryRead(FileDescriptor fd, byte[] b, int off, int len)
        ByteBuffer dst = Util.getTemporaryDirectBuffer(len);
        assert dst.position() == 0;
        try {
+            // 'dst' is guaranteed not to be associated with a closeable session.
+            // Hence, there is no need for acquiring any session.


This comment is will be confusing to anyone reading this code. Is this really needed?

The reason for the comment is to make it clear why DirectBuffer::address can be used directly without guarding. This will also reduce the probability of unnecessary guarding being added in the future. However, if the consensus is that these comments just adds confusion, I am happy to remove them.

I'd prefer to see this comment removed from all places that are obviously interacting with the direct buffer cache. These usages are try-finally to acquire and return the temporary direct buffer cache back to the cache. Talking about closable sessions here is definitely confusing.

Thanks for persisting with it, latest version looks okay to me and the naming issue can be sorted out after the JEP is integrated.

IMHO the renaming is not super important - the underlying abstraction managing the segment lifetime is still called MemorySession, even after https://git.openjdk.org/jdk/pull/10872. And, acquire/release are methods on MemorySession - so I think the current name might be more precise even after we integrate the latest API changes.

AlanBateman · 2022-11-21T11:32:35Z

src/java.base/share/classes/java/util/zip/Adler32.java

@@ -96,7 +99,7 @@ public void update(ByteBuffer buffer) {
        if (rem <= 0)
            return;
        if (buffer.isDirect()) {
-            try {
+            try (var sessionAcquisition = NIO_ACCESS.acquireSessionAsAutoCloseable(buffer)) {


We need to find something better than "sessionAcquisition", it looks very messy at all these use sites.

Eventually, I hope most of them will be named _.

maybe just bufferLock and and just acquireBuffer ?

Would it be possible to re-examine acquireSession returning Runnable and
acquireSessionAsAutoCloseable returning AutoCloseable. The naming is bit awkward at most of the use sites and maybe we can do better.

I think it would be confusing to have overloads with the same name.

The aquireSession(Buffer, boolean) method is only used once and is used in IOUtil so we could rename it to aquireSessionOrNull and then rename acquireSessionAsAutoCloseable to acquireSession.

IIRC, Runnable was chosen back then because of the issues with using TWR which was generating warnings (because resource not accessed in body of TWR). I don't have strong opinions on this, other than we should only have one way to do things, rather than 2-3. Since the JDK has capabilities to acquire and release a session w/o a TWR and/or Runnable, one might also consider whether exposing acquire/release separately, and just use those.

src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java

src/java.base/share/classes/sun/nio/ch/DirectBuffer.java

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

…java Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

AlanBateman · 2022-11-21T14:07:38Z

src/java.base/share/classes/java/util/zip/Adler32.java

@@ -96,7 +99,7 @@ public void update(ByteBuffer buffer) {
        if (rem <= 0)
            return;
        if (buffer.isDirect()) {
-            try {
+            try (var guard = NIO_ACCESS.acquireSession(buffer)) {
                adler = updateByteBuffer(adler, ((DirectBuffer)buffer).address(), pos, rem);
            } finally {
                Reference.reachabilityFence(buffer);


The updated naming is a bit better but there are it still feels that that there are too many things going on at the use sites ("guard", "session", "reachability fence"). Ideally the acquire would return something with an accessor for the direct buffer address but that may not be possible. I wonder if changing NOP_CLOSE.close to do reachabilityFence(this) would work as expected and improve many of these use sites?

This can certainly be done. We could, for example, add a new method to the JavaNioAccess interface:

AddressAcquisition acquireSession2(Buffer buffer); // to be renamed

This would allow us to go from:

try (var guard = NIO_ACCESS.acquireSession(buffer)) { adler = updateByteBuffer(adler, ((DirectBuffer)buffer).address(), pos, rem); } finally { Reference.reachabilityFence(buffer); }

to

try (var guard = NIO_ACCESS.acquireSession2(buffer)) { adler = updateByteBuffer(adler, guard.address(), pos, rem); }

Although this looks much simpler and concise, it means "a new object is created for each invocation" (*). This also allows us to remove the @SupressWarnings("try") annotations.

Here is how the undercarriage might look like:

@Override public AddressAcquisition acquireSession2(Buffer buffer) { var session = buffer.session(); if (session == null) { return AddressAcquisition.create(buffer, () -> {}); } session.acquire0(); return AddressAcquisition.create(buffer, session::release0); }

and

sealed interface AddressAcquisition extends AutoCloseable { long address(); @Override void close(); static AddressAcquisition create(Buffer buffer, Runnable closeActions) { return new AddressAcquisitionImpl(buffer, closeActions); } final class AddressAcquisitionImpl implements AddressAcquisition { private final DirectBuffer directBuffer; private final Runnable closeAction; public AddressAcquisitionImpl(Buffer buffer, Runnable closeAction) { this.directBuffer = (DirectBuffer) buffer; this.closeAction = closeAction; } @Override public long address() { return directBuffer.address(); } @Override public void close() { try { closeAction.run(); } finally { Reference.reachabilityFence(directBuffer); } } } }

(*) In reality, a representation of the object might be allocated on the stack instead.

Although this looks much simpler and concise, it means "a new object is created for each invocation"

My comment was actually to see if DirectBuffer could extend AutoCloseable so that the acquire returns "this" for the non-session case. Doing the equivalent for the session case might leak into MemorySessionImpl implementing DirectBuffer which you probably don't want to do. If NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would at least improve some of the use-sites.

Although this looks much simpler and concise, it means "a new object is created for each invocation"

My comment was actually to see if DirectBuffer could extend AutoCloseable so that the acquire returns "this" for the non-session case. Doing the equivalent for the session case might leak into MemorySessionImpl implementing DirectBuffer which you probably don't want to do. If NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would at least improve some of the use-sites.

Not sure that is simpler. ByteBuffer <: AutoCloseable doesn't seem to make sense to me. I'm also not sure how much object allocation (all this stuff will become value types) should be the driving factor in these code paths.

Right, I didn't mean BB <: AC but to see if we avoid needing to wrap it because this PR is touching several low level and performance critical code paths. For the faraway places then having the close do a Reference.reachabilityFence(this) should avoid the surprise that the buffer has to kept alive even though it appears that the try-with-resources is doing it already.

I have reworked Acquisition. I think we could merge the old and new way in a separate PR.

AlanBateman · 2022-11-21T14:14:14Z

src/java.base/share/classes/sun/nio/fs/NativeBuffers.java

@@ -88,6 +88,8 @@ static NativeBuffer getNativeBufferFromCache(int size) {
     * Returns a native buffer, of at least the given size. The native buffer
     * is taken from the thread local cache if possible; otherwise it is
     * allocated from the heap.
+     * <p>
+     * The returned NativeBuffer is guaranteed not to be asynchronously closeable.


This class, and the temporary buffer cache in sun.nio.ch.Util, are intended to be used with try-finally. There isn't any notion of asynchronously close so maybe it would best to not add this comment to these sources.

That is clear to me but I am trying to prevent future redundant guarding. Anyway, I will remove the comments.

That is clear to me but I am trying to prevent future redundant guarding. Anyway, I will remove the comments.

The faraway use sites look much better now. The performance sensitive usages need close attention. Can you summarise the changes to DatagramChannel, are you creating a NoOpScopeAcquisition for every send/receive? This is low level code where we do do something different to avoid the try-with-resources.

…rg/jdk into guard-directbuffer-address

openjdk · 2022-11-21T15:33:42Z

⚠️ @minborg This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

ijuma · 2022-11-22T09:34:49Z

because this PR is touching several low level and performance critical code paths

Indeed. Have we verified that performance doesn't regress with these changes?

mcimadamore · 2022-11-22T09:23:40Z

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java

@@ -907,44 +911,49 @@ int doLastBlock(GCMOperation op, ByteBuffer buffer, ByteBuffer src,
         * and if dst will overwrite src data before src can be processed.
         * If so, make a copy to put the dst data in.
         */
+        @SuppressWarnings("try")


After looking at the implementation some more, I'm not sure this need fixing? E.g. this method is just using the address to compute some overlap - and return a buffer sliced accordingly. There's no access to the buffer data (except for the last part which does a put). The access will fail if the session is closed from underneath. I don't think this can crash the VM (in fact this code did not have a reachability fence to begin with).

Well spotted. I will remove the guarding here.

This @SuppressWarnings annotation is no longer needed:

Suggested change

@SuppressWarnings("try")

mcimadamore · 2022-11-22T09:29:14Z

src/java.base/share/classes/java/nio/Buffer.java

@@ -823,7 +824,7 @@ public MemorySegment bufferSegment(Buffer buffer) {
                }

                @Override
-                public Runnable acquireSession(Buffer buffer, boolean async) {
+                public Runnable acquireSessionOrNull(Buffer buffer, boolean async) {


If allocation/performance is an issue, a relatively straightforward way to adjust the code would be to let go of the TWR entirely. E.g. we have code that does this:

Buffer b = ... try { // use buffer.address(); } finally { Reference.reachabilityFence(b); }

We could transform to this:

Buffer b = ... acquire(b); // calls MemorySessionImpl.acquire0 if session is not null try { // use buffer.address(); } finally { release(b); // calls MemorySessionImpl.release0 if session is not null (and maybe adds a reachability fence, just in case) }

This leads to a simpler patch that is probably easier to validate. The remaining IOUtil code will be using a different idiom, and perhaps we can, at some point, go back and make that consistent too.

The AutoCloseable experiment was interesting to try but I think you are right, acquire try { .. } finally release would be simpler and also remove the concerns about the performance critical code paths.

mcimadamore · 2022-11-22T09:32:32Z

src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java

-        if (n > 0)
-            bb.position(pos + n);
-        return n;
+        try (var guard = NIO_ACCESS.acquireScope(bb)) {


Why was the old code not using reachability fences? Bug or feature?

I see that there's a subsequent buffer call if n > 0, so that's probably why the fence was skipped? (I also assume that the code calling this method will access the buffer before/after, so reachability is never truly an issue - but for session-backed buffers this needs fixing).

Also, stepping back, I note how, if receive0 was a native call using Linker, perhaps we wouldn't need all this manual address computation - we'd just get a memory segment slice from the buffer and pass that to the handle (which will perform the correct liveness check). E.g. maybe a better long term solution would be to panama-ize this code?

Yes, once the memory/linker APIs are permanent then the SCTP implementation would be a good candidate to redo.

ExE-Boss · 2022-11-22T10:18:58Z

src/java.base/share/classes/sun/nio/ch/DirectBuffer.java

+    // try (var guard = NIO_ACCESS.acquireSession(bb)) {
+    //     performOperation(bb.address());
+    // }


Again, updating this comment was missed:

Suggested change

// try (var guard = NIO_ACCESS.acquireSession(bb)) {

// performOperation(bb.address());

// }

// try (var guard = NIO_ACCESS.acquireScope(bb)) {

// performOperation(guard.address());

// }

minborg · 2022-11-30T08:01:06Z

/integrate

openjdk · 2022-11-30T08:02:50Z

@minborg
Your change (at version 619473e) is now ready to be sponsored by a Committer.

minborg · 2022-11-30T17:32:11Z

I've converted this PR to a draft because we want to wait for the PR_20 branch being merged JEP 434

openjdk · 2022-12-06T09:19:23Z

@minborg this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout guard-directbuffer-address
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

minborg · 2022-12-06T10:32:04Z

/integrate

openjdk · 2022-12-06T10:34:41Z

@minborg
Your change (at version cd95bc8) is now ready to be sponsored by a Committer.

AlanBateman · 2022-12-06T10:41:49Z

/sponsor

openjdk · 2022-12-06T10:43:18Z

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

a9e6c62: 8297186: G1 triggers unnecessary full GCs when heap utilization is low
4458de9: 8297172: Fix some issues of auto-vectorization of Long.bitCount/numberOfTrailingZeros/numberOfLeadingZeros()

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-06T10:43:42Z

@AlanBateman @minborg Pushed as commit 84b927a.

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

Guard usages of DirectBuffer::address

23a6fd1

openjdk bot added the rfr Pull request is ready for review label Nov 21, 2022

openjdk bot added security security-dev@openjdk.org nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org net net-dev@openjdk.org labels Nov 21, 2022

AlanBateman reviewed Nov 21, 2022

View reviewed changes

minborg added 3 commits November 21, 2022 13:15

Cleanup after comments

9fafafa

Remove comments

9fcf2bb

Rename methods

c081b4a

ExE-Boss reviewed Nov 21, 2022

View reviewed changes

src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java Outdated Show resolved Hide resolved

src/java.base/share/classes/sun/nio/ch/DirectBuffer.java Outdated Show resolved Hide resolved

minborg and others added 3 commits November 21, 2022 14:38

Cleanup static declarations

eca7c38

Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java

17a72e9

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.…

a0de3bc

…java Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

AlanBateman reviewed Nov 21, 2022

View reviewed changes

minborg added 2 commits November 21, 2022 16:29

Remove comments

7097934

Merge branch 'guard-directbuffer-address' of https://github.com/minbo…

88ed3aa

…rg/jdk into guard-directbuffer-address

Rework Acquisition

0526e3d

mcimadamore reviewed Nov 22, 2022

View reviewed changes

ExE-Boss reviewed Nov 22, 2022

View reviewed changes

AlanBateman approved these changes Nov 29, 2022

View reviewed changes

PaulSandoz approved these changes Nov 29, 2022

View reviewed changes

bplb approved these changes Nov 29, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 30, 2022

minborg marked this pull request as draft November 30, 2022 17:29

openjdk bot removed the rfr Pull request is ready for review label Nov 30, 2022

minborg changed the title ~~8296024: Usage of DIrectBuffer::address should be guarded~~ 8296024: Usage of DirectBuffer::address should be guarded Dec 6, 2022

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Dec 6, 2022

Merge master

cd95bc8

openjdk bot added ready Pull request is ready to be integrated and removed sponsor Pull request is ready to be sponsored merge-conflict Pull request has merge conflict with target branch labels Dec 6, 2022

minborg marked this pull request as ready for review December 6, 2022 10:20

openjdk bot added the rfr Pull request is ready for review label Dec 6, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 6, 2022

openjdk bot added the integrated Pull request has been integrated label Dec 6, 2022

openjdk bot closed this Dec 6, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 6, 2022

8296024: Usage of DirectBuffer::address should be guarded #11260

8296024: Usage of DirectBuffer::address should be guarded #11260

Conversation

minborg commented Nov 21, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 21, 2022

openjdk bot commented Nov 21, 2022

mlbridge bot commented Nov 21, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanBateman Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanBateman Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Nov 21, 2022

ijuma commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minborg commented Nov 30, 2022

openjdk bot commented Nov 30, 2022

minborg commented Nov 30, 2022 • edited Loading

openjdk bot commented Dec 6, 2022

minborg commented Dec 6, 2022

openjdk bot commented Dec 6, 2022

AlanBateman commented Dec 6, 2022

openjdk bot commented Dec 6, 2022

openjdk bot commented Dec 6, 2022

minborg commented Nov 21, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 21, 2022 •

edited

Loading

AlanBateman Nov 21, 2022 •

edited

Loading

AlanBateman Nov 21, 2022 •

edited

Loading

minborg commented Nov 30, 2022 •

edited

Loading