Skip to content

Commit

Permalink
Prevent walking image heap continuations with unpatched references an…
Browse files Browse the repository at this point in the history
…d carrier threads unexpectedly leaking into frames.
  • Loading branch information
peter-hofer committed Feb 18, 2025
1 parent 3d54364 commit 2958b90
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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--;
}
}

Expand Down

0 comments on commit 2958b90

Please sign in to comment.