8261395: C1 crash "cannot make java calls from the native compiler"

David Holmes · iklam · coleenp · David Holmes · commit e828a939a815 · 2021-05-12T05:21:58.000Z
Co-authored-by: Ioi Lam &lt;iklam@openjdk.org&gt;
Co-authored-by: Coleen Phillimore &lt;coleenp@openjdk.org&gt;
Reviewed-by: iklam, hseigel, coleenp
diff --git a/src/hotspot/share/oops/instanceKlass.cpp b/src/hotspot/share/oops/instanceKlass.cpp
@@ -163,60 +163,31 @@ static inline bool is_class_loader(const Symbol* class_name,
 // private: called to verify that k is a static member of this nest.
 // We know that k is an instance class in the same package and hence the
 // same classloader.
-bool InstanceKlass::has_nest_member(InstanceKlass* k, TRAPS) const {
+bool InstanceKlass::has_nest_member(JavaThread* current, InstanceKlass* k) const {
   assert(!is_hidden(), "unexpected hidden class");
   if (_nest_members == NULL || _nest_members == Universe::the_empty_short_array()) {
     if (log_is_enabled(Trace, class, nestmates)) {
-      ResourceMark rm(THREAD);
+      ResourceMark rm(current);
       log_trace(class, nestmates)("Checked nest membership of %s in non-nest-host class %s",
                                   k->external_name(), this->external_name());
     }
     return false;
   }
 
   if (log_is_enabled(Trace, class, nestmates)) {
-    ResourceMark rm(THREAD);
+    ResourceMark rm(current);
     log_trace(class, nestmates)("Checking nest membership of %s in %s",
                                 k->external_name(), this->external_name());
   }
 
-  // Check for a resolved cp entry , else fall back to a name check.
-  // We don't want to resolve any class other than the one being checked.
+  // Check for the named class in _nest_members.
+  // We don't resolve, or load, any classes.
   for (int i = 0; i < _nest_members->length(); i++) {
     int cp_index = _nest_members->at(i);
-    if (_constants->tag_at(cp_index).is_klass()) {
-      Klass* k2 = _constants->klass_at(cp_index, THREAD);
-      assert(!HAS_PENDING_EXCEPTION || PENDING_EXCEPTION->is_a(vmClasses::VirtualMachineError_klass()),
-             "Exceptions should not be possible here");
-      if (k2 == k) {
-        log_trace(class, nestmates)("- class is listed at nest_members[%d] => cp[%d]", i, cp_index);
-        return true;
-      }
-    }
-    else {
-      Symbol* name = _constants->klass_name_at(cp_index);
-      if (name == k->name()) {
-        log_trace(class, nestmates)("- Found it at nest_members[%d] => cp[%d]", i, cp_index);
-
-        // Names match so check actual klass. This may trigger class loading if
-        // it doesn't match though that should be impossible as it means one classloader
-        // has defined two different classes with the same name! A compiler thread won't be
-        // able to perform that loading but we can't exclude the compiler threads from
-        // executing this logic. But it should actually be impossible to trigger loading here.
-        Klass* k2 = _constants->klass_at(cp_index, THREAD);
-        assert(!HAS_PENDING_EXCEPTION || PENDING_EXCEPTION->is_a(vmClasses::VirtualMachineError_klass()),
-               "Exceptions should not be possible here");
-        if (k2 == k) {
-          log_trace(class, nestmates)("- class is listed as a nest member");
-          return true;
-        }
-        else {
-          // same name but different klass!
-          log_trace(class, nestmates)(" - klass comparison failed!");
-          // can't have two names the same, so we're done
-          return false;
-        }
-      }
+    Symbol* name = _constants->klass_name_at(cp_index);
+    if (name == k->name()) {
+      log_trace(class, nestmates)("- named class found at nest_members[%d] => cp[%d]", i, cp_index);
+      return true;
     }
   }
   log_trace(class, nestmates)("- class is NOT a nest member!");
@@ -287,7 +258,8 @@ InstanceKlass* InstanceKlass::nest_host(TRAPS) {
   // need to resolve and save our nest-host class.
   if (_nest_host_index != 0) { // we have a real nest_host
     // Before trying to resolve check if we're in a suitable context
-    if (!THREAD->can_call_java() && !_constants->tag_at(_nest_host_index).is_klass()) {
+    bool can_resolve = THREAD->can_call_java();
+    if (!can_resolve && !_constants->tag_at(_nest_host_index).is_klass()) {
       log_trace(class, nestmates)("Rejected resolution of nest-host of %s in unsuitable thread",
                                   this->external_name());
       return NULL; // sentinel to say "try again from a different context"
@@ -325,26 +297,15 @@ InstanceKlass* InstanceKlass::nest_host(TRAPS) {
         // not an instance class.
         if (k->is_instance_klass()) {
           nest_host_k = InstanceKlass::cast(k);
-          bool is_member = nest_host_k->has_nest_member(this, THREAD);
-          // exception is rare, perhaps impossible
-          if (!HAS_PENDING_EXCEPTION) {
-            if (is_member) {
-              _nest_host = nest_host_k; // save resolved nest-host value
-
-              log_trace(class, nestmates)("Resolved nest-host of %s to %s",
-                                          this->external_name(), k->external_name());
-              return nest_host_k;
-            } else {
-              error = "current type is not listed as a nest member";
-            }
+          bool is_member = nest_host_k->has_nest_member(THREAD->as_Java_thread(), this);
+          if (is_member) {
+            _nest_host = nest_host_k; // save resolved nest-host value
+
+            log_trace(class, nestmates)("Resolved nest-host of %s to %s",
+                                        this->external_name(), k->external_name());
+            return nest_host_k;
           } else {
-            if (PENDING_EXCEPTION->is_a(vmClasses::VirtualMachineError_klass())) {
-              return NULL; // propagate VMEs
-            }
-            stringStream ss;
-            ss.print("exception on member check: ");
-            java_lang_Throwable::print(PENDING_EXCEPTION, &ss);
-            error = ss.as_string();
+            error = "current type is not listed as a nest member";
           }
         } else {
           error = "host is not an instance class";
diff --git a/src/hotspot/share/oops/instanceKlass.hpp b/src/hotspot/share/oops/instanceKlass.hpp
@@ -479,8 +479,9 @@ class InstanceKlass: public Klass {
   void set_permitted_subclasses(Array<u2>* s) { _permitted_subclasses = s; }
 
 private:
-  // Called to verify that k is a member of this nest - does not look at k's nest-host
-  bool has_nest_member(InstanceKlass* k, TRAPS) const;
+  // Called to verify that k is a member of this nest - does not look at k's nest-host,
+  // nor does it resolve any CP entries or load any classes.
+  bool has_nest_member(JavaThread* current, InstanceKlass* k) const;
 
 public:
   // Used to construct informative IllegalAccessError messages at a higher level,
diff --git a/test/hotspot/jtreg/runtime/Nestmates/protectionDomain/Host.java b/test/hotspot/jtreg/runtime/Nestmates/protectionDomain/Host.java
@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2021, 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.
+ */
+
+// Host and Host$Member will be loaded by a custom loader with different
+// protection domains.
+
+public class Host {
+
+    private static int forNestmatesOnly = 1;
+
+    public static class Member {
+        // We need our static initializer to ensure our CP reference
+        // to Host is resolved by the main thread.
+        static final Class<?> hostClass = Host.class;
+
+        int id;
+
+        // Executing, or JIT compiling, this method will result in
+        // a nestmate access check.
+        public Member() {
+            id = forNestmatesOnly++;
+        }
+    }
+}
diff --git a/test/hotspot/jtreg/runtime/Nestmates/protectionDomain/TestDifferentProtectionDomains.java b/test/hotspot/jtreg/runtime/Nestmates/protectionDomain/TestDifferentProtectionDomains.java
@@ -0,0 +1,121 @@
+/*
+ * Copyright (c) 2021, 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 8261395
+ * @summary Test the code paths when a nest-host and nest-member class are in
+ *          different protection domains and the compiler thread needs to
+ *          perform a nestmate access check.
+ * @comment We use WB to force-compile a constructor to recreate the original
+ *          failure scenario, so only run when we have "normal" compiler flags.
+ * @requires vm.compMode=="Xmixed" &
+ *           (vm.opt.TieredStopAtLevel == null | vm.opt.TieredStopAtLevel == 4)
+ * @library /test/lib /
+ * @build sun.hotspot.WhiteBox
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller sun.hotspot.WhiteBox
+ * @compile Host.java
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xbootclasspath/a:.
+ *                   -Xlog:class+nestmates=trace,protectiondomain=trace
+ *                   -Djava.security.manager=allow
+ *                   TestDifferentProtectionDomains
+ */
+
+import java.lang.reflect.Constructor;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.nio.file.Path;
+import java.security.ProtectionDomain;
+
+import compiler.whitebox.CompilerWhiteBoxTest;
+import sun.hotspot.WhiteBox;
+
+public class TestDifferentProtectionDomains {
+
+    static final String TARGET = "Host";
+
+    // We need a custom classloader so that we can
+    // use a different protection domain for our target classes.
+
+    static class CustomLoader extends ClassLoader {
+
+        CustomLoader(ClassLoader parent) {
+            super(parent);
+        }
+
+        @Override
+        public Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
+            synchronized (getClassLoadingLock(name)) {
+                // First, check if the class has already been loaded
+                Class<?> clz = findLoadedClass(name);
+                if (clz != null) {
+                    return clz;
+                }
+
+                // Check for target class
+                if (name.startsWith(TARGET)) {
+                    try {
+                        String clzFile = name.replaceAll("\\.", "/") + ".class";
+                        byte[] buff = getResourceAsStream(clzFile).readAllBytes();
+                        ProtectionDomain differentPD = new ProtectionDomain(null, null);
+                        return defineClass(name, buff, 0, buff.length, differentPD);
+                    } catch (Throwable t) {
+                        throw new RuntimeException("Unexpected", t);
+                    }
+                }
+            }
+            return super.loadClass(name, resolve);
+        }
+    }
+
+    public static void main(String[] args) throws Throwable {
+
+        CustomLoader cl = new CustomLoader(TestDifferentProtectionDomains.class.getClassLoader());
+        Class<?> host = cl.loadClass("Host");
+        Class<?> member = cl.loadClass("Host$Member");
+
+        if (host.getProtectionDomain() == member.getProtectionDomain()) {
+            throw new Error("ProtectionDomain instances were not different!");
+        }
+
+        Constructor cons = member.getDeclaredConstructor(new Class<?>[] {});
+        WhiteBox wb = WhiteBox.getWhiteBox();
+
+        // The code path for the original failure is now only followed when
+        // there is a security manager set, so we set one. We do this here
+        // as any earlier causes security exceptions running the test and we
+        // don't want to have to set up a policy file etc.
+        System.setSecurityManager(new SecurityManager());
+
+        // Force the constructor to compile, which then triggers the nestmate
+        // access check in the compiler thread, which leads to the original bug.
+        wb.enqueueMethodForCompilation(cons,  CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION);
+        while (!wb.isMethodCompiled(cons)) {
+            Thread.sleep(100);
+        }
+
+        // Just for good measure call the compiled constructor.
+        Object m = member.newInstance();
+    }
+}
