8313816: Accessing jmethodID might lead to spurious crashes

src/hotspot/share/classfile/classFileParser.cpp

@@ -5576,8 +5576,8 @@ ClassFileParser::~ClassFileParser() {
   }
 
   if (_methods != nullptr) {
-    // Free methods
-    InstanceKlass::deallocate_methods(_loader_data, _methods);
+    // Free methods - those methods are not fully wired and miss the method holder
+    InstanceKlass::deallocate_methods(_loader_data, _methods, false);
   }
 
   // beware of the Universe::empty_blah_array!!

src/hotspot/share/oops/instanceKlass.cpp

@@ -528,7 +528,8 @@ InstanceKlass::InstanceKlass(const ClassFileParser& parser, KlassKind kind, Refe
 }
 
 void InstanceKlass::deallocate_methods(ClassLoaderData* loader_data,
-                                       Array<Method*>* methods) {
+                                       Array<Method*>* methods,
+                                       bool with_method_holders) {
   if (methods != nullptr && methods != Universe::the_empty_method_array() &&
       !methods->is_shared()) {
     for (int i = 0; i < methods->length(); i++) {
@@ -537,6 +538,10 @@ 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");
+      // Do the pointer maintenance before releasing the metadata, but not for incomplete methods
+      if (with_method_holders) {
+        method->clear_jmethod_id();
+      }
       MetadataFactory::free_metadata(loader_data, method);
     }
     MetadataFactory::free_array<Method*>(loader_data, methods);

src/hotspot/share/oops/instanceKlass.hpp

@@ -992,7 +992,8 @@ 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,
+                                 bool with_method_holders = true);
   void static deallocate_interfaces(ClassLoaderData* loader_data,
                                     const Klass* super_klass,
                                     Array<InstanceKlass*>* local_interfaces,

src/hotspot/share/oops/method.cpp

@@ -2263,6 +2263,20 @@ void Method::clear_jmethod_ids(ClassLoaderData* loader_data) {
   loader_data->jmethod_ids()->clear_all_methods();
 }
 
+void Method::clear_jmethod_id() {
+  // Being at a safepoint prevents racing against other class redefinitions
+  assert(SafepointSynchronize::is_at_safepoint(), "should be at safepoint");
+  // The jmethodid is not stored in the Method instance, we need to look it up first
+  jmethodID methodid = find_jmethod_id_or_null();
+  // 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 (methodid != nullptr && *((Method**)methodid) == this) {
+    *((Method**)methodid) = nullptr;
+  }
+}
+
 bool Method::has_method_vptr(const void* ptr) {
   Method m;
   // This assumes that the vtbl pointer is the first word of a C++ object.

src/hotspot/share/oops/method.hpp

@@ -715,6 +715,7 @@ class Method : public Metadata {
 
   // Clear methods
   static void clear_jmethod_ids(ClassLoaderData* loader_data);
+  void clear_jmethod_id();
   static void print_jmethod_ids_count(const ClassLoaderData* loader_data, outputStream* out) PRODUCT_RETURN;
 
   // Get this method's jmethodID -- allocate if it doesn't exist

src/hotspot/share/prims/whitebox.cpp

@@ -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[] = {
@@ -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},
 };
 
 

test/hotspot/jtreg/serviceability/jvmti/thread/GetStackTrace/GetStackTraceAndRetransformTest/GetStackTraceAndRetransformTest.java

@@ -0,0 +1,167 @@
+/*
+ * Copyright (c) 2023 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2023, Datadog, Inc. 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 Test that a sequence of method retransformation and stacktrace capture while the old method
+ *          version is still on stack does not lead to a crash when that's method jmethodID is used as
+ *          an argument for JVMTI functions.
+ * @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 driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox
+ * @run driver GetStackTraceAndRetransformTest build-jar
+ * @run main/othervm/native -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -javaagent:./agent.jar -agentlib:GetStackTraceAndRetransformTest 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.ArrayList;
+import java.util.List;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.locks.LockSupport;
+
+import jdk.internal.org.objectweb.asm.ClassReader;
+import jdk.internal.org.objectweb.asm.ClassVisitor;
+import jdk.internal.org.objectweb.asm.ClassWriter;
+import jdk.internal.org.objectweb.asm.MethodVisitor;
+import jdk.internal.org.objectweb.asm.Opcodes;
+import jdk.internal.org.objectweb.asm.Type;
+import jdk.test.lib.helpers.ClassFileInstaller;
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.whitebox.WhiteBox;
+
+public class GetStackTraceAndRetransformTest {
+    public static final class Shared {
+        public static volatile Instrumentation inst;
+
+        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";
+
+    public static void main(String args[]) throws Throwable {
+        if (args.length == 1 && args[0].equals("build-jar")) {
+            buildAgent();
+            return;
+        }
+        initialize(Transformable.class);
+
+        Transformable.retransformAndStacktrace();
+        Transformable.stacktrace();
+
+        WhiteBox.getWhiteBox().cleanMetaspaces();
+        check(2);
+    }
+
+    private static String buildAgent() throws Exception {
+        Path jar = Paths.get(".", "agent.jar");
+        String jarPath = jar.toAbsolutePath().toString();
+        ClassFileInstaller.writeJar(jarPath,
+                ClassFileInstaller.Manifest.fromString(agentManifest),
+                Agent.class.getName());
+        return jarPath;
+    }
+
+    private static class Transformable {
+        static void retransformAndStacktrace() {
+            Shared.retransform();
+            capture(Thread.currentThread());
+        }
+
+        static void stacktrace() {
+            capture(Thread.currentThread());
+        }
+    }
+
+    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 {
+            // only if Transformable is being retransformed
+            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);
+                            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 + "#" + (++counter));
+                                    mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, java.io.PrintStream.class.getName().replace('.', '/'), "println", "(Ljava/lang/String;)V", false);
+                                }
+                            };
+                        }
+                    };
+                    cr.accept(cv, 0);
+                    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(int expected);
+}

test/hotspot/jtreg/serviceability/jvmti/thread/GetStackTrace/GetStackTraceAndRetransformTest/libGetStackTraceAndRetransformTest.cpp

@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2023 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2023, Datadog, Inc. 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.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include "jvmti.h"
+#include "jvmti_common.h"
+#include "../get_stack_trace.h"
+
+
+extern "C" {
+
+static jvmtiEnv *jvmti = NULL;
+static jmethodID* ids = NULL;
+static int ids_size = 0;
+
+JNIEXPORT jint JNICALL
+Agent_OnLoad(JavaVM *jvm, char *options, void *reserved) {
+  jint res = jvm->GetEnv((void **) &jvmti, JVMTI_VERSION_1_1);
+  if (res != JNI_OK || jvmti == NULL) {
+    printf("Wrong result of a valid call to GetEnv!\n");
+    return JNI_ERR;
+  }
+  ids = (jmethodID*)malloc(sizeof(jmethodID) * 10);
+  return JNI_OK;
+}
+
+JNIEXPORT void JNICALL
+Java_GetStackTraceAndRetransformTest_initialize(JNIEnv *env, jclass cls, jclass tgt) {
+  // we need to force jmethodids to be created for the methods we are going to retransform
+  env->GetStaticMethodID(tgt, "retransformAndStacktrace", "()V");
+  env->GetStaticMethodID(tgt, "stacktrace", "()V");
+}
+
+JNIEXPORT void JNICALL
+Java_GetStackTraceAndRetransformTest_capture(JNIEnv *env, jclass cls, jthread thread) {
+  jint count;
+  const int MAX_NUMBER_OF_FRAMES = 32;
+  jvmtiFrameInfo frames[MAX_NUMBER_OF_FRAMES];
+
+  jvmtiError err = jvmti->GetStackTrace(thread, 0, MAX_NUMBER_OF_FRAMES, frames, &count);
+  check_jvmti_status(env, err, "GetStackTrace failed.");
+
+  ids[ids_size++] = frames[1].method;
+}
+
+JNIEXPORT void JNICALL
+Java_GetStackTraceAndRetransformTest_check(JNIEnv *jni, jclass cls, jint expected) {
+  if (ids_size != expected) {
+    fprintf(stderr, "Unexpected number methods captured: %d (expected %d)\n", ids_size, expected);
+    exit(2);
+  }
+  for (int i = 0; i < ids_size; i++) {
+    jclass rslt = NULL;
+    char* class_name = NULL;
+    jvmti->GetMethodDeclaringClass(ids[i], &rslt);
+    if (rslt != NULL) {
+        jvmti->GetClassSignature(rslt, &class_name, NULL);
+    }
+  }
+}
+}

test/lib/jdk/test/whitebox/WhiteBox.java

@@ -521,6 +521,8 @@ public void clearInlineCaches(boolean preserve_static_stubs) {
   public native long metaspaceCapacityUntilGC();
   public native long metaspaceSharedRegionAlignment();
 
+  public native void cleanMetaspaces();
+
   // Metaspace Arena Tests
   public native long createMetaspaceTestContext(long commit_limit, long reserve_limit);
   public native void destroyMetaspaceTestContext(long context);