From 2958b90c6f3b8e6ce6bd5150aa4c2973cdd5b0ca Mon Sep 17 00:00:00 2001 From: Peter Hofer Date: Thu, 13 Feb 2025 16:35:14 +0100 Subject: [PATCH] Prevent walking image heap continuations with unpatched references and carrier threads unexpectedly leaking into frames. --- .../core/heap/StoredContinuationAccess.java | 22 +++++++++++++++++-- .../core/thread/ContinuationInternals.java | 13 ++++++++++- .../oracle/svm/core/thread/JavaThreads.java | 3 ++- .../core/thread/Target_java_lang_Thread.java | 8 ++++++- .../Target_jdk_internal_vm_Continuation.java | 22 +++++++++---------- 5 files changed, 51 insertions(+), 17 deletions(-) diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/StoredContinuationAccess.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/StoredContinuationAccess.java index 16112c0ee9cb..1f3a0a2a633d 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/StoredContinuationAccess.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/StoredContinuationAccess.java @@ -24,6 +24,8 @@ */ package com.oracle.svm.core.heap; +import static com.oracle.svm.core.Uninterruptible.CALLED_FROM_UNINTERRUPTIBLE_CODE; + import org.graalvm.nativeimage.ImageSingletons; import org.graalvm.nativeimage.StackValue; import org.graalvm.nativeimage.c.function.CodePointer; @@ -169,10 +171,24 @@ private static void setIP(StoredContinuation cont, CodePointer ip) { cont.ip = ip; } + @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true) + public static boolean shouldWalkContinuation(StoredContinuation s) { + /* + * StoredContinuations in the image heap are read-only and should not be visited. They may + * contain unresolved uncompressed references which are patched only on a platform thread's + * stack before resuming the continuation. We should typically not see such objects during + * GC, but they might be visited due to card marking when they are near an object which has + * been modified at runtime. + */ + return !Heap.getHeap().isInImageHeap(s); + } + @AlwaysInline("De-virtualize calls to ObjectReferenceVisitor") @Uninterruptible(reason = "StoredContinuation must not move.", callerMustBe = true) public static boolean walkReferences(StoredContinuation s, ObjectReferenceVisitor visitor) { - assert !Heap.getHeap().isInImageHeap(s) : "StoredContinuations in the image heap are read-only and don't need to be visited"; + if (!shouldWalkContinuation(s)) { + return true; + } JavaStackWalk walk = StackValue.get(JavaStackWalker.sizeOfJavaStackWalk()); JavaStackWalker.initializeForContinuation(walk, s); @@ -199,7 +215,9 @@ public static boolean walkReferences(StoredContinuation s, ObjectReferenceVisito @AlwaysInline("De-virtualize calls to visitor.") @Uninterruptible(reason = "StoredContinuation must not move.", callerMustBe = true) public static void walkFrames(StoredContinuation s, ContinuationStackFrameVisitor visitor, ContinuationStackFrameVisitorData data) { - assert !Heap.getHeap().isInImageHeap(s) : "StoredContinuations in the image heap are read-only and don't need to be visited"; + if (!shouldWalkContinuation(s)) { + return; + } JavaStackWalk walk = StackValue.get(JavaStackWalker.sizeOfJavaStackWalk()); JavaStackWalker.initializeForContinuation(walk, s); diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/ContinuationInternals.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/ContinuationInternals.java index e10fdda19449..48894ed2fb99 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/ContinuationInternals.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/ContinuationInternals.java @@ -24,7 +24,6 @@ */ package com.oracle.svm.core.thread; -import jdk.graal.compiler.word.Word; import org.graalvm.nativeimage.ImageSingletons; import org.graalvm.nativeimage.c.function.CodePointer; import org.graalvm.word.Pointer; @@ -39,6 +38,8 @@ import com.oracle.svm.core.stack.StackOverflowCheck; import com.oracle.svm.core.util.VMError; +import jdk.graal.compiler.word.Word; + /** Implementation of and access to {@link Target_jdk_internal_vm_Continuation} internals. */ @InternalVMMethod public final class ContinuationInternals { @@ -162,4 +163,14 @@ private static Integer doYield1(Target_jdk_internal_vm_Continuation c) { KnownIntrinsics.farReturn(null, returnSP, returnIP, false); throw VMError.shouldNotReachHereAtRuntime(); } + + /** + * Avoids references to the current carrier thread from leaking into the caller frame, which + * prevents persisting continuation stacks in (auxiliary) image heaps. + */ + @NeverInline("Prevent a reference to the current carrier thread from leaking into the caller frame.") + static Target_jdk_internal_vm_Continuation getContinuationFromCarrier() { + Target_java_lang_Thread carrier = JavaThreads.toTarget(Target_java_lang_Thread.currentCarrierThread()); + return carrier.cont; + } } diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/JavaThreads.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/JavaThreads.java index b58d8ac6d069..e804916854f2 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/JavaThreads.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/JavaThreads.java @@ -31,7 +31,6 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; -import jdk.graal.compiler.word.Word; import org.graalvm.nativeimage.CurrentIsolate; import org.graalvm.nativeimage.ImageSingletons; import org.graalvm.nativeimage.IsolateThread; @@ -62,6 +61,7 @@ import jdk.graal.compiler.core.common.SuppressFBWarnings; import jdk.graal.compiler.replacements.ReplacementsUtil; import jdk.graal.compiler.serviceprovider.JavaVersionUtil; +import jdk.graal.compiler.word.Word; /** * Implements operations on {@linkplain Target_java_lang_Thread Java threads}, which are on a higher @@ -209,6 +209,7 @@ public static boolean isCurrentThreadVirtual() { * Indicates whether the current thread is truly virtual (see {@link #isVirtual}) and * currently pinned to its carrier thread. */ + @NeverInline("Prevent a reference to the current carrier thread from leaking into the caller frame.") public static boolean isCurrentThreadVirtualAndPinned() { Target_java_lang_Thread carrier = JavaThreads.toTarget(Target_java_lang_Thread.currentCarrierThread()); return carrier != null && carrier.vthread != null && Target_jdk_internal_vm_Continuation.isPinned(carrier.cont.getScope()); diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_Thread.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_Thread.java index 26385ae097cf..9c672f7d004a 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_Thread.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_Thread.java @@ -463,7 +463,13 @@ private static void clearInterruptEvent() { @Alias // native void clearInterrupt(); - /** Carrier threads only: the current innermost continuation. */ + /** + * Carrier threads only: the current innermost continuation. + * + * Use {@link ContinuationInternals#getContinuationFromCarrier()} instead to avoid references to + * the current carrier thread from leaking into a stack frame, which prevents persisting + * continuation stacks in (auxiliary) image heaps. + */ @Alias // Target_jdk_internal_vm_Continuation cont; diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_jdk_internal_vm_Continuation.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_jdk_internal_vm_Continuation.java index a1a35906b70e..d977cc2120fe 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_jdk_internal_vm_Continuation.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_jdk_internal_vm_Continuation.java @@ -92,8 +92,7 @@ boolean isEmpty() { @Substitute @NeverInline("access stack pointer") private static int isPinned0(Target_jdk_internal_vm_ContinuationScope scope) { - Target_java_lang_Thread carrier = JavaThreads.toTarget(Target_java_lang_Thread.currentCarrierThread()); - Target_jdk_internal_vm_Continuation cont = carrier.cont; + Target_jdk_internal_vm_Continuation cont = ContinuationInternals.getContinuationFromCarrier(); if (cont != null) { while (true) { if (cont.pinCount != 0) { @@ -148,8 +147,7 @@ static void enterSpecial(Target_jdk_internal_vm_Continuation c, @SuppressWarning @Substitute private static int doYield() { - Target_java_lang_Thread carrier = JavaThreads.toTarget(Target_java_lang_Thread.currentCarrierThread()); - Target_jdk_internal_vm_Continuation cont = carrier.cont; + Target_jdk_internal_vm_Continuation cont = ContinuationInternals.getContinuationFromCarrier(); int pinnedReason = isPinned0(cont.getScope()); if (pinnedReason != 0) { return pinnedReason; @@ -181,23 +179,23 @@ void enter0() { @Substitute public static void pin() { - Target_java_lang_Thread carrier = JavaThreads.toTarget(Target_java_lang_Thread.currentCarrierThread()); - if (carrier.cont != null) { - if (carrier.cont.pinCount + 1 == 0) { // unsigned arithmetic + Target_jdk_internal_vm_Continuation cont = ContinuationInternals.getContinuationFromCarrier(); + if (cont != null) { + if (cont.pinCount + 1 == 0) { // unsigned arithmetic throw new IllegalStateException("Pin overflow"); } - carrier.cont.pinCount++; + cont.pinCount++; } } @Substitute public static void unpin() { - Target_java_lang_Thread carrier = JavaThreads.toTarget(Target_java_lang_Thread.currentCarrierThread()); - if (carrier.cont != null) { - if (carrier.cont.pinCount == 0) { + Target_jdk_internal_vm_Continuation cont = ContinuationInternals.getContinuationFromCarrier(); + if (cont != null) { + if (cont.pinCount == 0) { throw new IllegalStateException("Pin underflow"); } - carrier.cont.pinCount--; + cont.pinCount--; } }