8042235: redefining method used by multiple MethodHandles crashes VM
Note all MemberNames created on internal list for adjusting method entries.
Reviewed-by: sspitsyn, dcubed, lfoltan
diff --git a/hotspot/src/share/vm/classfile/javaClasses.cpp b/hotspot/src/share/vm/classfile/javaClasses.cpp
index 754134b..d900ef5 100644
--- a/hotspot/src/share/vm/classfile/javaClasses.cpp
+++ b/hotspot/src/share/vm/classfile/javaClasses.cpp
@@ -41,6 +41,7 @@
#include "oops/method.hpp"
#include "oops/symbol.hpp"
#include "oops/typeArrayOop.hpp"
+#include "prims/jvmtiRedefineClassesTrace.hpp"
#include "runtime/fieldDescriptor.hpp"
#include "runtime/handles.inline.hpp"
#include "runtime/interfaceSupport.hpp"
@@ -2794,12 +2795,35 @@
return (Metadata*)mname->address_field(_vmtarget_offset);
}
+bool java_lang_invoke_MemberName::is_method(oop mname) {
+ assert(is_instance(mname), "must be MemberName");
+ return (flags(mname) & (MN_IS_METHOD | MN_IS_CONSTRUCTOR)) > 0;
+}
+
#if INCLUDE_JVMTI
// Can be executed on VM thread only
-void java_lang_invoke_MemberName::adjust_vmtarget(oop mname, Metadata* ref) {
- assert((is_instance(mname) && (flags(mname) & (MN_IS_METHOD | MN_IS_CONSTRUCTOR)) > 0), "wrong type");
+void java_lang_invoke_MemberName::adjust_vmtarget(oop mname, Method* old_method,
+ Method* new_method, bool* trace_name_printed) {
+ assert(is_method(mname), "wrong type");
assert(Thread::current()->is_VM_thread(), "not VM thread");
- mname->address_field_put(_vmtarget_offset, (address)ref);
+
+ Method* target = (Method*)mname->address_field(_vmtarget_offset);
+ if (target == old_method) {
+ mname->address_field_put(_vmtarget_offset, (address)new_method);
+
+ if (RC_TRACE_IN_RANGE(0x00100000, 0x00400000)) {
+ if (!(*trace_name_printed)) {
+ // RC_TRACE_MESG macro has an embedded ResourceMark
+ RC_TRACE_MESG(("adjust: name=%s",
+ old_method->method_holder()->external_name()));
+ *trace_name_printed = true;
+ }
+ // RC_TRACE macro has an embedded ResourceMark
+ RC_TRACE(0x00400000, ("MemberName method update: %s(%s)",
+ new_method->name()->as_C_string(),
+ new_method->signature()->as_C_string()));
+ }
+ }
}
#endif // INCLUDE_JVMTI
diff --git a/hotspot/src/share/vm/classfile/javaClasses.hpp b/hotspot/src/share/vm/classfile/javaClasses.hpp
index c7890d2..ee2c807 100644
--- a/hotspot/src/share/vm/classfile/javaClasses.hpp
+++ b/hotspot/src/share/vm/classfile/javaClasses.hpp
@@ -1100,7 +1100,8 @@
static Metadata* vmtarget(oop mname);
static void set_vmtarget(oop mname, Metadata* target);
#if INCLUDE_JVMTI
- static void adjust_vmtarget(oop mname, Metadata* target);
+ static void adjust_vmtarget(oop mname, Method* old_method, Method* new_method,
+ bool* trace_name_printed);
#endif // INCLUDE_JVMTI
static intptr_t vmindex(oop mname);
@@ -1114,6 +1115,8 @@
return obj != NULL && is_subclass(obj->klass());
}
+ static bool is_method(oop obj);
+
// Relevant integer codes (keep these in synch. with MethodHandleNatives.Constants):
enum {
MN_IS_METHOD = 0x00010000, // method (not constructor)
diff --git a/hotspot/src/share/vm/oops/instanceKlass.cpp b/hotspot/src/share/vm/oops/instanceKlass.cpp
index ab925e4..e80eaec 100644
--- a/hotspot/src/share/vm/oops/instanceKlass.cpp
+++ b/hotspot/src/share/vm/oops/instanceKlass.cpp
@@ -2931,28 +2931,27 @@
return NULL;
}
-void InstanceKlass::add_member_name(int index, Handle mem_name) {
+bool InstanceKlass::add_member_name(Handle mem_name) {
jweak mem_name_wref = JNIHandles::make_weak_global(mem_name);
MutexLocker ml(MemberNameTable_lock);
- assert(0 <= index && index < idnum_allocated_count(), "index is out of bounds");
DEBUG_ONLY(No_Safepoint_Verifier nsv);
+ // Check if method has been redefined while taking out MemberNameTable_lock, if so
+ // return false. We cannot cache obsolete methods. They will crash when the function
+ // is called!
+ Method* method = (Method*)java_lang_invoke_MemberName::vmtarget(mem_name());
+ if (method->is_obsolete()) {
+ return false;
+ } else if (method->is_old()) {
+ // Replace method with redefined version
+ java_lang_invoke_MemberName::set_vmtarget(mem_name(), method_with_idnum(method->method_idnum()));
+ }
+
if (_member_names == NULL) {
_member_names = new (ResourceObj::C_HEAP, mtClass) MemberNameTable(idnum_allocated_count());
}
- _member_names->add_member_name(index, mem_name_wref);
-}
-
-oop InstanceKlass::get_member_name(int index) {
- MutexLocker ml(MemberNameTable_lock);
- assert(0 <= index && index < idnum_allocated_count(), "index is out of bounds");
- DEBUG_ONLY(No_Safepoint_Verifier nsv);
-
- if (_member_names == NULL) {
- return NULL;
- }
- oop mem_name =_member_names->get_member_name(index);
- return mem_name;
+ _member_names->add_member_name(mem_name_wref);
+ return true;
}
// -----------------------------------------------------------------------------------------------------
diff --git a/hotspot/src/share/vm/oops/instanceKlass.hpp b/hotspot/src/share/vm/oops/instanceKlass.hpp
index a34a8dc..94a3c80 100644
--- a/hotspot/src/share/vm/oops/instanceKlass.hpp
+++ b/hotspot/src/share/vm/oops/instanceKlass.hpp
@@ -1072,8 +1072,7 @@
// JSR-292 support
MemberNameTable* member_names() { return _member_names; }
void set_member_names(MemberNameTable* member_names) { _member_names = member_names; }
- void add_member_name(int index, Handle member_name);
- oop get_member_name(int index);
+ bool add_member_name(Handle member_name);
public:
// JVMTI support
diff --git a/hotspot/src/share/vm/prims/jvm.cpp b/hotspot/src/share/vm/prims/jvm.cpp
index e28db7c..b137729 100644
--- a/hotspot/src/share/vm/prims/jvm.cpp
+++ b/hotspot/src/share/vm/prims/jvm.cpp
@@ -567,13 +567,14 @@
// Make shallow object copy
const int size = obj->size();
- oop new_obj = NULL;
+ oop new_obj_oop = NULL;
if (obj->is_array()) {
const int length = ((arrayOop)obj())->length();
- new_obj = CollectedHeap::array_allocate(klass, size, length, CHECK_NULL);
+ new_obj_oop = CollectedHeap::array_allocate(klass, size, length, CHECK_NULL);
} else {
- new_obj = CollectedHeap::obj_allocate(klass, size, CHECK_NULL);
+ new_obj_oop = CollectedHeap::obj_allocate(klass, size, CHECK_NULL);
}
+
// 4839641 (4840070): We must do an oop-atomic copy, because if another thread
// is modifying a reference field in the clonee, a non-oop-atomic copy might
// be suspended in the middle of copying the pointer and end up with parts
@@ -584,24 +585,41 @@
// The same is true of StubRoutines::object_copy and the various oop_copy
// variants, and of the code generated by the inline_native_clone intrinsic.
assert(MinObjAlignmentInBytes >= BytesPerLong, "objects misaligned");
- Copy::conjoint_jlongs_atomic((jlong*)obj(), (jlong*)new_obj,
+ Copy::conjoint_jlongs_atomic((jlong*)obj(), (jlong*)new_obj_oop,
(size_t)align_object_size(size) / HeapWordsPerLong);
// Clear the header
- new_obj->init_mark();
+ new_obj_oop->init_mark();
// Store check (mark entire object and let gc sort it out)
BarrierSet* bs = Universe::heap()->barrier_set();
assert(bs->has_write_region_opt(), "Barrier set does not have write_region");
- bs->write_region(MemRegion((HeapWord*)new_obj, size));
+ bs->write_region(MemRegion((HeapWord*)new_obj_oop, size));
+
+ Handle new_obj(THREAD, new_obj_oop);
+ // Special handling for MemberNames. Since they contain Method* metadata, they
+ // must be registered so that RedefineClasses can fix metadata contained in them.
+ if (java_lang_invoke_MemberName::is_instance(new_obj()) &&
+ java_lang_invoke_MemberName::is_method(new_obj())) {
+ Method* method = (Method*)java_lang_invoke_MemberName::vmtarget(new_obj());
+ // MemberName may be unresolved, so doesn't need registration until resolved.
+ if (method != NULL) {
+ methodHandle m(THREAD, method);
+ // This can safepoint and redefine method, so need both new_obj and method
+ // in a handle, for two different reasons. new_obj can move, method can be
+ // deleted if nothing is using it on the stack.
+ m->method_holder()->add_member_name(new_obj());
+ }
+ }
// Caution: this involves a java upcall, so the clone should be
// "gc-robust" by this stage.
if (klass->has_finalizer()) {
assert(obj->is_instance(), "should be instanceOop");
- new_obj = InstanceKlass::register_finalizer(instanceOop(new_obj), CHECK_NULL);
+ new_obj_oop = InstanceKlass::register_finalizer(instanceOop(new_obj()), CHECK_NULL);
+ new_obj = Handle(THREAD, new_obj_oop);
}
- return JNIHandles::make_local(env, oop(new_obj));
+ return JNIHandles::make_local(env, new_obj());
JVM_END
// java.io.File ///////////////////////////////////////////////////////////////
diff --git a/hotspot/src/share/vm/prims/methodHandles.cpp b/hotspot/src/share/vm/prims/methodHandles.cpp
index 8013a13..1b3b4fd 100644
--- a/hotspot/src/share/vm/prims/methodHandles.cpp
+++ b/hotspot/src/share/vm/prims/methodHandles.cpp
@@ -29,7 +29,6 @@
#include "interpreter/oopMapCache.hpp"
#include "memory/allocation.inline.hpp"
#include "memory/oopFactory.hpp"
-#include "prims/jvmtiRedefineClassesTrace.hpp"
#include "prims/methodHandles.hpp"
#include "runtime/compilationPolicy.hpp"
#include "runtime/javaCalls.hpp"
@@ -276,9 +275,12 @@
// This is done eagerly, since it is readily available without
// constructing any new objects.
// TO DO: maybe intern mname_oop
- m->method_holder()->add_member_name(m->method_idnum(), mname);
-
- return mname();
+ if (m->method_holder()->add_member_name(mname)) {
+ return mname();
+ } else {
+ // Redefinition caused this to fail. Return NULL (and an exception?)
+ return NULL;
+ }
}
oop MethodHandles::init_field_MemberName(Handle mname, fieldDescriptor& fd, bool is_setter) {
@@ -951,63 +953,27 @@
}
}
-void MemberNameTable::add_member_name(int index, jweak mem_name_wref) {
+void MemberNameTable::add_member_name(jweak mem_name_wref) {
assert_locked_or_safepoint(MemberNameTable_lock);
- this->at_put_grow(index, mem_name_wref);
-}
-
-// Return a member name oop or NULL.
-oop MemberNameTable::get_member_name(int index) {
- assert_locked_or_safepoint(MemberNameTable_lock);
-
- jweak ref = this->at(index);
- oop mem_name = JNIHandles::resolve(ref);
- return mem_name;
+ this->push(mem_name_wref);
}
#if INCLUDE_JVMTI
-oop MemberNameTable::find_member_name_by_method(Method* old_method) {
- assert_locked_or_safepoint(MemberNameTable_lock);
- oop found = NULL;
- int len = this->length();
-
- for (int idx = 0; idx < len; idx++) {
- oop mem_name = JNIHandles::resolve(this->at(idx));
- if (mem_name == NULL) {
- continue;
- }
- Method* method = (Method*)java_lang_invoke_MemberName::vmtarget(mem_name);
- if (method == old_method) {
- found = mem_name;
- break;
- }
- }
- return found;
-}
-
-// It is called at safepoint only
+// It is called at safepoint only for RedefineClasses
void MemberNameTable::adjust_method_entries(Method** old_methods, Method** new_methods,
int methods_length, bool *trace_name_printed) {
assert(SafepointSynchronize::is_at_safepoint(), "only called at safepoint");
- // search the MemberNameTable for uses of either obsolete or EMCP methods
+ // For each redefined method
for (int j = 0; j < methods_length; j++) {
Method* old_method = old_methods[j];
Method* new_method = new_methods[j];
- oop mem_name = find_member_name_by_method(old_method);
- if (mem_name != NULL) {
- java_lang_invoke_MemberName::adjust_vmtarget(mem_name, new_method);
- if (RC_TRACE_IN_RANGE(0x00100000, 0x00400000)) {
- if (!(*trace_name_printed)) {
- // RC_TRACE_MESG macro has an embedded ResourceMark
- RC_TRACE_MESG(("adjust: name=%s",
- old_method->method_holder()->external_name()));
- *trace_name_printed = true;
- }
- // RC_TRACE macro has an embedded ResourceMark
- RC_TRACE(0x00400000, ("MemberName method update: %s(%s)",
- new_method->name()->as_C_string(),
- new_method->signature()->as_C_string()));
+ // search the MemberNameTable for uses of either obsolete or EMCP methods
+ for (int idx = 0; idx < length(); idx++) {
+ oop mem_name = JNIHandles::resolve(this->at(idx));
+ if (mem_name != NULL) {
+ java_lang_invoke_MemberName::adjust_vmtarget(mem_name, old_method, new_method,
+ trace_name_printed);
}
}
}
diff --git a/hotspot/src/share/vm/prims/methodHandles.hpp b/hotspot/src/share/vm/prims/methodHandles.hpp
index 4084d6c..ff7870e 100644
--- a/hotspot/src/share/vm/prims/methodHandles.hpp
+++ b/hotspot/src/share/vm/prims/methodHandles.hpp
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2008, 2014, 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
@@ -236,18 +236,14 @@
public:
MemberNameTable(int methods_cnt);
~MemberNameTable();
- void add_member_name(int index, jweak mem_name_ref);
- oop get_member_name(int index);
+ void add_member_name(jweak mem_name_ref);
#if INCLUDE_JVMTI
- public:
// RedefineClasses() API support:
// If a MemberName refers to old_method then update it
// to refer to new_method.
void adjust_method_entries(Method** old_methods, Method** new_methods,
int methods_length, bool *trace_name_printed);
- private:
- oop find_member_name_by_method(Method* old_method);
#endif // INCLUDE_JVMTI
};
diff --git a/hotspot/test/compiler/jsr292/RedefineMethodUsedByMultipleMethodHandles.java b/hotspot/test/compiler/jsr292/RedefineMethodUsedByMultipleMethodHandles.java
new file mode 100644
index 0000000..1695c35
--- /dev/null
+++ b/hotspot/test/compiler/jsr292/RedefineMethodUsedByMultipleMethodHandles.java
@@ -0,0 +1,174 @@
+/*
+ * Copyright (c) 2014, 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 8042235
+ * @summary redefining method used by multiple MethodHandles crashes VM
+ * @compile -XDignore.symbol.file RedefineMethodUsedByMultipleMethodHandles.java
+ * @run main RedefineMethodUsedByMultipleMethodHandles
+ */
+
+import java.io.*;
+import java.lang.instrument.*;
+import java.lang.invoke.*;
+import java.lang.invoke.MethodHandles.Lookup;
+import java.lang.management.*;
+import java.lang.reflect.*;
+import java.nio.file.*;
+import java.security.*;
+import java.util.jar.*;
+
+import javax.tools.*;
+
+import jdk.internal.org.objectweb.asm.*;
+
+public class RedefineMethodUsedByMultipleMethodHandles {
+
+ static class Foo {
+ public static Object getName() {
+ return "foo";
+ }
+ }
+
+ public static void main(String[] args) throws Throwable {
+
+ Lookup lookup = MethodHandles.lookup();
+ Method fooMethod = Foo.class.getDeclaredMethod("getName");
+
+ // fooMH2 displaces fooMH1 from the MemberNamesTable
+ MethodHandle fooMH1 = lookup.unreflect(fooMethod);
+ MethodHandle fooMH2 = lookup.unreflect(fooMethod);
+
+ System.out.println("fooMH1.invoke = " + fooMH1.invokeExact());
+ System.out.println("fooMH2.invoke = " + fooMH2.invokeExact());
+
+ // Redefining Foo.getName() causes vmtarget to be updated
+ // in fooMH2 but not fooMH1
+ redefineFoo();
+
+ // Full GC causes fooMH1.vmtarget to be deallocated
+ System.gc();
+
+ // Calling fooMH1.vmtarget crashes the VM
+ System.out.println("fooMH1.invoke = " + fooMH1.invokeExact());
+ }
+
+ /**
+ * Adds the class file bytes for {@code c} to {@code jar}.
+ */
+ static void add(JarOutputStream jar, Class<?> c) throws IOException {
+ String classAsPath = c.getName().replace('.', '/') + ".class";
+ jar.putNextEntry(new JarEntry(classAsPath));
+ InputStream stream = c.getClassLoader().getResourceAsStream(classAsPath);
+
+ int b;
+ while ((b = stream.read()) != -1) {
+ jar.write(b);
+ }
+ }
+
+ static void redefineFoo() throws Exception {
+ Manifest manifest = new Manifest();
+ manifest.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0");
+ Attributes mainAttrs = manifest.getMainAttributes();
+ mainAttrs.putValue("Agent-Class", FooAgent.class.getName());
+ mainAttrs.putValue("Can-Redefine-Classes", "true");
+ mainAttrs.putValue("Can-Retransform-Classes", "true");
+
+ Path jar = Files.createTempFile("myagent", ".jar");
+ try {
+ JarOutputStream jarStream = new JarOutputStream(new FileOutputStream(jar.toFile()), manifest);
+ add(jarStream, FooAgent.class);
+ add(jarStream, FooTransformer.class);
+ jarStream.close();
+ runAgent(jar);
+ } finally {
+ Files.deleteIfExists(jar);
+ }
+ }
+
+ public static void runAgent(Path agent) throws Exception {
+ String vmName = ManagementFactory.getRuntimeMXBean().getName();
+ int p = vmName.indexOf('@');
+ assert p != -1 : "VM name not in <pid>@<host> format: " + vmName;
+ String pid = vmName.substring(0, p);
+ ClassLoader cl = ToolProvider.getSystemToolClassLoader();
+ Class<?> c = Class.forName("com.sun.tools.attach.VirtualMachine", true, cl);
+ Method attach = c.getDeclaredMethod("attach", String.class);
+ Method loadAgent = c.getDeclaredMethod("loadAgent", String.class);
+ Method detach = c.getDeclaredMethod("detach");
+ Object vm = attach.invoke(null, pid);
+ loadAgent.invoke(vm, agent.toString());
+ detach.invoke(vm);
+ }
+
+ public static class FooAgent {
+
+ public static void agentmain(@SuppressWarnings("unused") String args, Instrumentation inst) throws Exception {
+ assert inst.isRedefineClassesSupported();
+ assert inst.isRetransformClassesSupported();
+ inst.addTransformer(new FooTransformer(), true);
+ Class<?>[] classes = inst.getAllLoadedClasses();
+ for (int i = 0; i < classes.length; i++) {
+ Class<?> c = classes[i];
+ if (c == Foo.class) {
+ inst.retransformClasses(new Class[]{c});
+ }
+ }
+ }
+ }
+
+ static class FooTransformer implements ClassFileTransformer {
+
+ @Override
+ public byte[] transform(ClassLoader cl, String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) throws IllegalClassFormatException {
+ if (Foo.class.equals(classBeingRedefined)) {
+ System.out.println("redefining " + classBeingRedefined);
+ ClassReader cr = new ClassReader(classfileBuffer);
+ ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES);
+ ClassVisitor adapter = new ClassVisitor(Opcodes.ASM5, cw) {
+ @Override
+ public MethodVisitor visitMethod(int access, String base, String desc, String signature, String[] exceptions) {
+ MethodVisitor mv = cv.visitMethod(access, base, desc, signature, exceptions);
+ if (mv != null) {
+ mv = new MethodVisitor(Opcodes.ASM5, mv) {
+ @Override
+ public void visitLdcInsn(Object cst) {
+ System.out.println("replacing \"" + cst + "\" with \"bar\"");
+ mv.visitLdcInsn("bar");
+ }
+ };
+ }
+ return mv;
+ }
+ };
+
+ cr.accept(adapter, ClassReader.SKIP_FRAMES);
+ cw.visitEnd();
+ return cw.toByteArray();
+ }
+ return classfileBuffer;
+ }
+ }
+}