8182397: Race in field updates when creating ArrayKlasses can lead to crash
Summary: Update array_klass field in component mirror after klass.java_mirror field for concurrent readers in compiled code
Reviewed-by: aph, kbarrett
--- a/hotspot/src/share/vm/classfile/javaClasses.cpp Sat Jul 22 10:03:15 2017 +0200
+++ b/hotspot/src/share/vm/classfile/javaClasses.cpp Mon Jul 24 17:46:06 2017 -0400
@@ -818,6 +818,7 @@
void java_lang_Class::create_mirror(Klass* k, Handle class_loader,
Handle module, Handle protection_domain, TRAPS) {
+ assert(k != NULL, "Use create_basic_type_mirror for primitive types");
assert(k->java_mirror() == NULL, "should only assign mirror once");
// Use this moment of initialization to cache modifier_flags also,
// to support Class.getModifiers(). Instance classes recalculate
@@ -831,11 +832,10 @@
// Allocate mirror (java.lang.Class instance)
oop mirror_oop = InstanceMirrorKlass::cast(SystemDictionary::Class_klass())->allocate_instance(k, CHECK);
Handle mirror(THREAD, mirror_oop);
+ Handle comp_mirror;
// Setup indirection from mirror->klass
- if (k != NULL) {
- java_lang_Class::set_klass(mirror(), k);
- }
+ java_lang_Class::set_klass(mirror(), k);
InstanceMirrorKlass* mk = InstanceMirrorKlass::cast(mirror->klass());
assert(oop_size(mirror()) == mk->instance_size(k), "should have been set");
@@ -844,22 +844,22 @@
// It might also have a component mirror. This mirror must already exist.
if (k->is_array_klass()) {
- oop comp_mirror;
if (k->is_typeArray_klass()) {
BasicType type = TypeArrayKlass::cast(k)->element_type();
- comp_mirror = Universe::java_mirror(type);
+ comp_mirror = Handle(THREAD, Universe::java_mirror(type));
} else {
assert(k->is_objArray_klass(), "Must be");
Klass* element_klass = ObjArrayKlass::cast(k)->element_klass();
assert(element_klass != NULL, "Must have an element klass");
- comp_mirror = element_klass->java_mirror();
+ comp_mirror = Handle(THREAD, element_klass->java_mirror());
}
- assert(comp_mirror != NULL, "must have a mirror");
+ assert(comp_mirror() != NULL, "must have a mirror");
// Two-way link between the array klass and its component mirror:
// (array_klass) k -> mirror -> component_mirror -> array_klass -> k
- set_component_mirror(mirror(), comp_mirror);
- set_array_klass(comp_mirror, k);
+ set_component_mirror(mirror(), comp_mirror());
+ // See below for ordering dependencies between field array_klass in component mirror
+ // and java_mirror in this klass.
} else {
assert(k->is_instance_klass(), "Must be");
@@ -881,10 +881,13 @@
// set the module field in the java_lang_Class instance
set_mirror_module_field(k, mirror, module, THREAD);
- // Setup indirection from klass->mirror last
+ // Setup indirection from klass->mirror
// after any exceptions can happen during allocations.
- if (k != NULL) {
- k->set_java_mirror(mirror());
+ k->set_java_mirror(mirror());
+ if (comp_mirror() != NULL) {
+ // Set after k->java_mirror() is published, because compiled code running
+ // concurrently doesn't expect a k to have a null java_mirror.
+ release_set_array_klass(comp_mirror(), k);
}
} else {
if (fixup_mirror_list() == NULL) {
@@ -989,7 +992,7 @@
if (type != T_VOID) {
Klass* aklass = Universe::typeArrayKlassObj(type);
assert(aklass != NULL, "correct bootstrap");
- set_array_klass(java_class, aklass);
+ release_set_array_klass(java_class, aklass);
}
#ifdef ASSERT
InstanceMirrorKlass* mk = InstanceMirrorKlass::cast(SystemDictionary::Class_klass());
@@ -1086,9 +1089,9 @@
}
-void java_lang_Class::set_array_klass(oop java_class, Klass* klass) {
+void java_lang_Class::release_set_array_klass(oop java_class, Klass* klass) {
assert(klass->is_klass() && klass->is_array_klass(), "should be array klass");
- java_class->metadata_field_put(_array_klass_offset, klass);
+ java_class->release_metadata_field_put(_array_klass_offset, klass);
}
--- a/hotspot/src/share/vm/classfile/javaClasses.hpp Sat Jul 22 10:03:15 2017 +0200
+++ b/hotspot/src/share/vm/classfile/javaClasses.hpp Mon Jul 24 17:46:06 2017 -0400
@@ -237,7 +237,7 @@
static oop primitive_mirror(BasicType t);
// JVM_NewArray support
static Klass* array_klass(oop java_class);
- static void set_array_klass(oop java_class, Klass* klass);
+ static void release_set_array_klass(oop java_class, Klass* klass);
// compiler support for class operations
static int klass_offset_in_bytes() { return _klass_offset; }
static int array_klass_offset_in_bytes() { return _array_klass_offset; }
--- a/hotspot/src/share/vm/oops/oop.hpp Sat Jul 22 10:03:15 2017 +0200
+++ b/hotspot/src/share/vm/oops/oop.hpp Mon Jul 24 17:46:06 2017 -0400
@@ -204,6 +204,8 @@
inline Metadata* metadata_field(int offset) const;
inline void metadata_field_put(int offset, Metadata* value);
+ inline void release_metadata_field_put(int offset, Metadata* value);
+
inline jbyte byte_field(int offset) const;
inline void byte_field_put(int offset, jbyte contents);
--- a/hotspot/src/share/vm/oops/oop.inline.hpp Sat Jul 22 10:03:15 2017 +0200
+++ b/hotspot/src/share/vm/oops/oop.inline.hpp Mon Jul 24 17:46:06 2017 -0400
@@ -446,6 +446,10 @@
Metadata* oopDesc::metadata_field(int offset) const { return *metadata_field_addr(offset); }
void oopDesc::metadata_field_put(int offset, Metadata* value) { *metadata_field_addr(offset) = value; }
+void oopDesc::release_metadata_field_put(int offset, Metadata* value) {
+ OrderAccess::release_store_ptr(metadata_field_addr(offset), value);
+}
+
jbyte oopDesc::byte_field(int offset) const { return (jbyte) *byte_field_addr(offset); }
void oopDesc::byte_field_put(int offset, jbyte contents) { *byte_field_addr(offset) = (jint) contents; }
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/runtime/CreateMirror/ArraysNewInstanceBug.java Mon Jul 24 17:46:06 2017 -0400
@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2017, 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 ArraysNewInstanceBug
+ * @bug 8182397
+ * @summary race in setting array_klass field for component mirror with mirror update for klass
+ * @modules java.base/jdk.internal.misc
+ * @run main/othervm -Xcomp ArraysNewInstanceBug
+ */
+
+// This test crashes in compiled code with race, because the compiler generates code that assumes this ordering.
+import java.lang.reflect.Array;
+import java.net.URL;
+import java.net.URLClassLoader;
+
+public class ArraysNewInstanceBug implements Runnable {
+ static Class<?>[] classes;
+
+ int start;
+
+ ArraysNewInstanceBug(int start) {
+ this.start = start;
+ }
+
+ String[] result;
+
+ public void run() {
+ result = new String[classes.length];
+ System.err.print('.');
+ for (int i = start; i < classes.length; i++) {
+ result[i] = Array.newInstance(classes[i], 0).getClass().getName();
+ }
+ }
+
+ public static void main(String[] args) throws Throwable {
+ Class<?> c = ArraysNewInstanceBug.class;
+ ClassLoader apploader = c.getClassLoader();
+ for (int iter = 0; iter < 10 ; iter++) { // 10 is enough to get it to crash on my machine.
+ System.err.print('[');
+ classes = new Class<?>[1000];
+ String urlpath = "file://" + System.getProperty("test.classes") + "/";
+ for (int i = 0; i < classes.length; i++) {
+ ClassLoader loader = new URLClassLoader(new URL[] { new URL(urlpath) }, apploader.getParent());
+ classes[i] = loader.loadClass(c.getSimpleName());
+ }
+ System.err.print(']');
+ System.err.print('(');
+ int threadCount = 64;
+ Thread[] threads = new Thread[threadCount];
+ for (int i = 0; i < threads.length; i++) {
+ threads[i] = new Thread(new ArraysNewInstanceBug(i));
+ }
+ for (int i = 0; i < threads.length; i++) {
+ threads[i].start();
+ }
+ for (int i = 0; i < threads.length; i++) {
+ threads[i].join();
+ }
+ System.err.print(')');
+ }
+ }
+}