[GR-61998] Prevent walking image heap continuations with unpatched references and carrier threads unexpectedly leaking into frames.

PullRequest: graal/20053

Author: peter-hofer

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);

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;
+    }
 }

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 <em>truly</em> 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());

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;
 

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--;
         }
     }