Skip to content

Commit

Permalink
8313816: Accessing jmethodID might lead to spurious crashes
Browse files Browse the repository at this point in the history
  • Loading branch information
jbachorik committed Nov 14, 2023
1 parent 346dbd6 commit 8b05a61
Show file tree
Hide file tree
Showing 6 changed files with 407 additions and 3 deletions.
15 changes: 13 additions & 2 deletions src/hotspot/share/oops/instanceKlass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ InstanceKlass::InstanceKlass(const ClassFileParser& parser, KlassKind kind, Refe
}

void InstanceKlass::deallocate_methods(ClassLoaderData* loader_data,
Array<Method*>* methods) {
Array<Method*>* methods, InstanceKlass* klass) {
if (methods != nullptr && methods != Universe::the_empty_method_array() &&
!methods->is_shared()) {
for (int i = 0; i < methods->length(); i++) {
Expand All @@ -537,6 +537,17 @@ void InstanceKlass::deallocate_methods(ClassLoaderData* loader_data,
// Only want to delete methods that are not executing for RedefineClasses.
// The previous version will point to them so they're not totally dangling
assert (!method->on_stack(), "shouldn't be called with methods on stack");
if (klass) {
jmethodID jmid = method->find_jmethod_id_or_null();
// Do the pointer maintenance before releasing the metadata, just in case
// We need to make sure that jmethodID actually resolves to this method
// - multiple redefined versions may share jmethodID slots and if a method
// has already been rewired to a newer version we could be removing reference
// to a still existing method instance
if (jmid != nullptr && *((Method**)jmid) == method) {
*((Method**)jmid) = nullptr;
}
}
MetadataFactory::free_metadata(loader_data, method);
}
MetadataFactory::free_array<Method*>(loader_data, methods);
Expand Down Expand Up @@ -604,7 +615,7 @@ void InstanceKlass::deallocate_contents(ClassLoaderData* loader_data) {
// redefine classes. MethodData can also be released separately.
release_C_heap_structures(/* release_sub_metadata */ false);

deallocate_methods(loader_data, methods());
deallocate_methods(loader_data, methods(), this);
set_methods(nullptr);

deallocate_record_components(loader_data, record_components());
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/oops/instanceKlass.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -992,7 +992,7 @@ class InstanceKlass: public Klass {
// instanceKlasses and the metadata they point to.
void deallocate_contents(ClassLoaderData* loader_data);
static void deallocate_methods(ClassLoaderData* loader_data,
Array<Method*>* methods);
Array<Method*>* methods, InstanceKlass* klass = nullptr);
void static deallocate_interfaces(ClassLoaderData* loader_data,
const Klass* super_klass,
Array<InstanceKlass*>* local_interfaces,
Expand Down
5 changes: 5 additions & 0 deletions src/hotspot/share/prims/whitebox.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2627,6 +2627,10 @@ WB_ENTRY(void, WB_PreTouchMemory(JNIEnv* env, jobject wb, jlong addr, jlong size
}
WB_END

WB_ENTRY(void, WB_CleanMetaspaces(JNIEnv* env, jobject target))
ClassLoaderDataGraph::safepoint_and_clean_metaspaces();
WB_END

#define CC (char*)

static JNINativeMethod methods[] = {
Expand Down Expand Up @@ -2913,6 +2917,7 @@ static JNINativeMethod methods[] = {
{CC"unlockCritical", CC"()V", (void*)&WB_UnlockCritical},
{CC"setVirtualThreadsNotifyJvmtiMode", CC"(Z)Z", (void*)&WB_SetVirtualThreadsNotifyJvmtiMode},
{CC"preTouchMemory", CC"(JJ)V", (void*)&WB_PreTouchMemory},
{CC"cleanMetaspaces", CC"()V", (void*)&WB_CleanMetaspaces},
};


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,285 @@
/*
* Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/**
* @test
* @bug 8313816
* @summary TODO
* @requires vm.jvmti
* @requires vm.flagless
* @library /test/lib
* @build jdk.test.whitebox.WhiteBox
* @modules java.instrument java.base/jdk.internal.org.objectweb.asm
* @compile GetStackTraceAndRetransformTest.java
* @run main/othervm/native GetStackTraceAndRetransformTest
*/

import java.lang.instrument.ClassFileTransformer;
import java.lang.instrument.IllegalClassFormatException;
import java.lang.instrument.Instrumentation;
import java.lang.instrument.UnmodifiableClassException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.security.ProtectionDomain;
import java.util.concurrent.CyclicBarrier;
import java.util.jar.JarFile;
import java.util.concurrent.locks.LockSupport;

import jdk.internal.org.objectweb.asm.ClassReader;
import jdk.internal.org.objectweb.asm.ClassWriter;
import jdk.internal.org.objectweb.asm.ClassVisitor;
import jdk.internal.org.objectweb.asm.MethodVisitor;
import jdk.internal.org.objectweb.asm.Opcodes;
import jdk.internal.org.objectweb.asm.Type;

import jdk.test.whitebox.WhiteBox;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.process.ProcessTools;
import jdk.test.lib.helpers.ClassFileInstaller;
public class GetStackTraceAndRetransformTest {
public static final class Shared {
public static volatile Instrumentation inst;
public static volatile boolean retransformed = false;
public static volatile boolean active = true;

public static void waitForRetransformed() {
while (!retransformed && active) {
LockSupport.parkNanos(1000000);
}
Shared.retransformed = false;
}

public static void retransform() {
try {
Shared.inst.retransformClasses(new Class[] { Transformable.class });
} catch (UnmodifiableClassException e) {
throw new RuntimeException(e);
}
}
}

private static String agentManifest =
"Premain-Class: " + GetStackTraceAndRetransformTest.Agent.class.getName() + "\n"
+ "Can-Retransform-Classes: true\n";
private static String launcherManifest =
"Main-Class: " + GetStackTraceAndRetransformTest.Launcher.class.getName() + "\n";

public static void main(String args[]) throws Throwable {
String agentJar = buildAgent();
String launcherJar = buildLauncher();
String bootCpExt = GetStackTraceAndRetransformTest.class.getProtectionDomain().getCodeSource().getLocation().getPath();
int idx = bootCpExt.lastIndexOf("/classes/0/");
if (idx != -1) {
bootCpExt = bootCpExt.substring(0, idx) + "/classes/0/test/lib";
}
ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
"-Xbootclasspath/a:" + bootCpExt,
"--add-exports=java.base/jdk.internal.org.objectweb.asm=ALL-UNNAMED",
"-javaagent:" + agentJar,
"-agentlib:GetStackTraceAndRetransformTest",
"-XX:+UnlockDiagnosticVMOptions",
"-XX:+WhiteBoxAPI",
"-XX:ErrorFile=/tmp/hs_err.log",
"-jar", launcherJar);
System.err.println("===> cmd: " + pb.command());
OutputAnalyzer oa = ProcessTools.executeProcess(pb);
System.err.println("===> out: \n" + oa.getOutput());
oa.shouldHaveExitValue(0);
}

private static String buildAgent() throws Exception {
Path jar = Files.createTempFile(Paths.get("."), null, ".jar");
String jarPath = jar.toAbsolutePath().toString();
ClassFileInstaller.writeJar(jarPath,
ClassFileInstaller.Manifest.fromString(agentManifest),
Agent.class.getName());
return jarPath;
}

private static String buildLauncher() throws Exception {
Path jar = Files.createTempFile(Paths.get("."), null, ".jar");
String jarPath = jar.toAbsolutePath().toString();
ClassFileInstaller.writeJar(jarPath,
ClassFileInstaller.Manifest.fromString(launcherManifest),
GetStackTraceAndRetransformTest.class.getName(),
Worker.class.getName(),
Transformable.class.getName(),
Shared.class.getName(),
Launcher.class.getName(),
SimpleTransformer.class.getName(),
SimpleTransformer.class.getName() + "$1",
SimpleTransformer.class.getName() + "$1$1");
return jarPath;
}

private static class Transformable {
static void callAction1() {
Shared.retransform();
capture(Thread.currentThread());
}

static void callAction2() {
capture(Thread.currentThread());
}

static void callAction3() {
capture(Thread.currentThread());
}
}

public static class Launcher {
public static void main(String[] args) {
System.err.println("===> Launching");
initialize(Transformable.class);

Worker.doit();
Worker.step();
Worker.stop();

WhiteBox wb = WhiteBox.getWhiteBox();
wb.cleanMetaspaces();
LockSupport.parkNanos(3_000_000_000L);
System.err.println("===> Check");
check();
}
}

public static class Worker {
private static Thread thrd = null;
private static final CyclicBarrier barrier = new CyclicBarrier(2);

static {
System.loadLibrary("GetStackTraceAndRetransformTest");
}

public static void doit() {
if (thrd != null) {
throw new RuntimeException("Worker thread already started");
}
System.err.println("===> Starting worker thread");
thrd = new Thread(() -> {
int counter = 1;
try {
while (Shared.active) {
barrier.await();
System.err.println("===> Calling action " + counter);
switch (counter) {
case 1:
Transformable.callAction1();
break;
case 2:
Transformable.callAction2();
break;
case 3:
Transformable.callAction3();
break;
}
counter = (counter % 3) + 1;
barrier.await();
}
} catch (Throwable e) {
throw new RuntimeException(e);
}
System.err.println("===> Exiting worker thread");
}, "Worker Thread");
thrd.setDaemon(true);
thrd.start();
}

public static void step() {
try {
barrier.await(); // enter step
barrier.await(); // exit step
} catch (Throwable e) {
throw new RuntimeException(e);
}
}

public static void stop() {
Shared.active = false;
step();
try {
thrd.join();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
}
}

public static class Agent implements ClassFileTransformer {
public static void premain(String args, Instrumentation inst) {
inst.addTransformer(new SimpleTransformer(), true);
Shared.inst = inst;
}
}

private static class SimpleTransformer implements ClassFileTransformer {
private static int counter = 0;
@Override
public byte[] transform(ClassLoader loader,
String className,
Class<?> classBeingRedefined,
ProtectionDomain protectionDomain,
byte[] classfileBuffer
) throws IllegalClassFormatException {
if (classBeingRedefined != null && className.equals("GetStackTraceAndRetransformTest$Transformable")) {
ClassReader cr = new ClassReader(classfileBuffer);
ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES);

try {
ClassVisitor cv = new ClassVisitor(Opcodes.ASM9, cw) {
@Override
public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) {
MethodVisitor mv = super.visitMethod(access, name, desc, signature, exceptions);
if (name.startsWith("callAction")) {
return new MethodVisitor(Opcodes.ASM5, mv) {
@Override
public void visitCode() {
super.visitCode();
mv.visitFieldInsn(Opcodes.GETSTATIC, System.class.getName().replace('.', '/'), "err", Type.getDescriptor(java.io.PrintStream.class));
mv.visitLdcInsn("Hello from transformed method: " + name.replace("callAction", "") + ", " + (++counter));
mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, java.io.PrintStream.class.getName().replace('.', '/'), "println", "(Ljava/lang/String;)V", false);
}
};
}
return mv;
}
};
cr.accept(cv, 0);
Shared.retransformed = true;
System.err.println("===> retransformed");
return cw.toByteArray();
} catch (Throwable t) {
t.printStackTrace(System.err);
throw t;
}
}
return null;
}
}

public static native void initialize(Class<?> target);
public static native void capture(Thread thread);
public static native void check();
}
Loading

0 comments on commit 8b05a61

Please sign in to comment.