8365203: defineClass with direct buffer can cause use-after-free #26724
Conversation
|
👋 Welcome back pminborg! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
| final class GuardByteBuffer { | ||
|
|
||
| @Test | ||
| void guardCrash() throws InterruptedException { |
There was a problem hiding this comment.
I was not able to reproduce the crash using this test on a Mac. The original reproducer worked on a Windows machine.
| postDefineClass(c, protectionDomain); | ||
| return c; | ||
|
|
||
| SharedSecrets.getJavaNioAccess().acquireSession(b); |
There was a problem hiding this comment.
Now that the fields in SharedSecrets are @Stable, we do not have to make a local copy in a static final field.
| SharedSecrets.getJavaNioAccess().acquireSession(b); | ||
| try { | ||
| Class<?> c = defineClass2(this, name, b, b.position(), len, protectionDomain, source); | ||
| postDefineClass(c, protectionDomain); |
There was a problem hiding this comment.
Should we leave postDefineClass out of this acquire-release scope? I don't see any reason including this.
There was a problem hiding this comment.
I don't think it matters here because something looking to close the arena around the time that it wants to defineClass with memory allocated from that arena is broken.
| } | ||
| }; | ||
| final List<Thread> threads = new ArrayList<>(); | ||
| for (int i = 0; i < Runtime.getRuntime().availableProcessors(); i++) { |
There was a problem hiding this comment.
This means all cores spinning for 20s - we'll have to see if it causes any side effects and slow down of other tests that happen to run at the same time in other agent VMs (make run-test uses concurrency by default).
There was a problem hiding this comment.
Indeed, tier 1 tests are recommended to run for 10 seconds.
|
The change looks okay. One thing to check if whether we have tests for JNI GetDirectBufferAddress when the byte buffer is a view of a memory segment. |
Description
This PR proposes to update the
ClassLoaderimplementation to properly guard access to the providedByteBufferwhen defining a class usingdefineClass(String, ByteBuffer, ...). Specifically, calls toSharedSecrets.getJavaNioAccess().acquireSession(ByteBuffer)andreleaseSession(ByteBuffer)have been introduced to ensure safe and consistent buffer access throughout the native class definition process, even in the case of aByteBufferis backed by aMemorySegment.Impact
This modification is internal to the
ClassLoaderimplementation and does not affect the public API.Improves the robustness and security of class loading from buffers.
Testing
Tier 1, 2, and 3 JDK tests pass on multiple platforms.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26724/head:pull/26724$ git checkout pull/26724Update a local copy of the PR:
$ git checkout pull/26724$ git pull https://git.openjdk.org/jdk.git pull/26724/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26724View PR using the GUI difftool:
$ git pr show -t 26724Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26724.diff
Using Webrev
Link to Webrev Comment