From 134e5e7eae46d77ec57b0a5b67715f08438f0721 Mon Sep 17 00:00:00 2001
Date: Fri, 22 Jan 2021 15:22:17 +0800
Subject: 8182397: race in field updates

Summary: <ArrayKlasses>: race in field updates when creating ArrayKlasses can lead to crash
LLT: hotspot/test/runtime/CreateMirror/ArraysNewInstanceBug.java
Bug url: https://bugs.openjdk.java.net/browse/JDK-8182397
---
 .../src/share/vm/classfile/javaClasses.cpp    | 31 +++----
 .../src/share/vm/classfile/javaClasses.hpp    |  2 +-
 hotspot/src/share/vm/oops/oop.hpp             |  2 +
 hotspot/src/share/vm/oops/oop.inline.hpp      |  4 +
 .../CreateMirror/ArraysNewInstanceBug.java    | 83 +++++++++++++++++++
 5 files changed, 107 insertions(+), 15 deletions(-)
 create mode 100644 hotspot/test/runtime/CreateMirror/ArraysNewInstanceBug.java

diff --git a/hotspot/src/share/vm/classfile/javaClasses.cpp b/hotspot/src/share/vm/classfile/javaClasses.cpp
index 3e37f9bd9..ac984d961 100644
--- a/hotspot/src/share/vm/classfile/javaClasses.cpp
+++ b/hotspot/src/share/vm/classfile/javaClasses.cpp
@@ -575,6 +575,7 @@ void java_lang_Class::initialize_mirror_fields(KlassHandle k,
 
 void java_lang_Class::create_mirror(KlassHandle k, Handle class_loader,
                                     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
@@ -587,11 +588,10 @@ void java_lang_Class::create_mirror(KlassHandle k, Handle class_loader,
   if (SystemDictionary::Class_klass_loaded()) {
     // Allocate mirror (java.lang.Class instance)
     Handle mirror = InstanceMirrorKlass::cast(SystemDictionary::Class_klass())->allocate_instance(k, CHECK);
+    Handle comp_mirror;
 
     // Setup indirection from mirror->klass
-    if (!k.is_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");
@@ -600,21 +600,21 @@ void java_lang_Class::create_mirror(KlassHandle k, Handle class_loader,
 
     // It might also have a component mirror.  This mirror must already exist.
     if (k->oop_is_array()) {
-      Handle comp_mirror;
       if (k->oop_is_typeArray()) {
         BasicType type = TypeArrayKlass::cast(k())->element_type();
-        comp_mirror = Universe::java_mirror(type);
+        comp_mirror = Handle(THREAD, Universe::java_mirror(type));
       } else {
         assert(k->oop_is_objArray(), "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.not_null(), "must have a mirror");
+      assert(comp_mirror() != NULL, "must have a mirror");
 
       // Two-way link between the array klass and its component mirror:
       ArrayKlass::cast(k())->set_component_mirror(comp_mirror());
-      set_array_klass(comp_mirror(), k());
+      // See below for ordering dependencies between field array_klass in component mirror
+      // and java_mirror in this klass.
     } else {
       assert(k->oop_is_instance(), "Must be");
 
@@ -633,10 +633,13 @@ void java_lang_Class::create_mirror(KlassHandle k, Handle class_loader,
     assert(class_loader() == k->class_loader(), "should be same");
     set_class_loader(mirror(), class_loader());
 
-    // Setup indirection from klass->mirror last
+    // Setup indirection from klass->mirror
     // after any exceptions can happen during allocations.
-    if (!k.is_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) {
@@ -718,7 +721,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());
@@ -815,9 +818,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->oop_is_array(), "should be array klass");
-  java_class->metadata_field_put(_array_klass_offset, klass);
+  java_class->release_metadata_field_put(_array_klass_offset, klass);
 }
 
 
diff --git a/hotspot/src/share/vm/classfile/javaClasses.hpp b/hotspot/src/share/vm/classfile/javaClasses.hpp
index c330c99e5..a5903d9b9 100644
--- a/hotspot/src/share/vm/classfile/javaClasses.hpp
+++ b/hotspot/src/share/vm/classfile/javaClasses.hpp
@@ -281,7 +281,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; }
diff --git a/hotspot/src/share/vm/oops/oop.hpp b/hotspot/src/share/vm/oops/oop.hpp
index ddaf177d0..97d44c046 100644
--- a/hotspot/src/share/vm/oops/oop.hpp
+++ b/hotspot/src/share/vm/oops/oop.hpp
@@ -202,6 +202,8 @@ class oopDesc {
   Metadata* metadata_field(int offset) const;
   void metadata_field_put(int offset, Metadata* value);
 
+  inline void release_metadata_field_put(int offset, Metadata* value);
+
   jbyte byte_field(int offset) const;
   void byte_field_put(int offset, jbyte contents);
 
diff --git a/hotspot/src/share/vm/oops/oop.inline.hpp b/hotspot/src/share/vm/oops/oop.inline.hpp
index ddb9dca2d..2ba94158f 100644
--- a/hotspot/src/share/vm/oops/oop.inline.hpp
+++ b/hotspot/src/share/vm/oops/oop.inline.hpp
@@ -373,6 +373,10 @@ inline 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);
+}
+
 inline void oopDesc::obj_field_put_raw(int offset, oop value) {
   UseCompressedOops ?
     encode_store_heap_oop(obj_field_addr<narrowOop>(offset), value) :
diff --git a/hotspot/test/runtime/CreateMirror/ArraysNewInstanceBug.java b/hotspot/test/runtime/CreateMirror/ArraysNewInstanceBug.java
new file mode 100644
index 000000000..870e8ea94
--- /dev/null
+++ b/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(')');
+        }
+    }
+}
-- 
2.19.0