8182397: Race in field updates when creating ArrayKlasses can lead to crash

Update array_klass field in component mirror after klass.java_mirror field for concurrent readers in compiled code

Reviewed-by: aph, kbarrett

hotspot/src/share/vm/classfile/javaClasses.cpp

@@ -818,6 +818,7 @@ void java_lang_Class::set_mirror_module_field(Klass* k, Handle mirror, Handle mo
 
 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 @@ void java_lang_Class::create_mirror(Klass* k, Handle class_loader,
     // 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 @@ void java_lang_Class::create_mirror(Klass* k, Handle class_loader,
 
     // 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 @@ void java_lang_Class::create_mirror(Klass* k, Handle class_loader,
     // 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 @@ oop java_lang_Class::create_basic_type_mirror(const char* basic_type_name, Basic
   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 @@ Klass* java_lang_Class::array_klass(oop java_class) {
 }
 
 
-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);
 }
 
 

hotspot/src/share/vm/classfile/javaClasses.hpp

@@ -237,7 +237,7 @@ class java_lang_Class : AllStatic {
   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; }

hotspot/src/share/vm/oops/oop.hpp

@@ -204,6 +204,8 @@ class oopDesc {
   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);
 

hotspot/src/share/vm/oops/oop.inline.hpp

@@ -446,6 +446,10 @@ void oopDesc::obj_field_put_volatile(int offset, oop value) {
 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; }
 

hotspot/test/runtime/CreateMirror/ArraysNewInstanceBug.java

@@ -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(')');
+        }
+    }
+}