8345911: Enhance error message when IncompatibleClassChangeError is thrown for sealed class loading failures

David Holmes · David Holmes · commit 453310918b5d · 2024-12-18T04:15:57.000Z
Reviewed-by: coleenp, alanb
diff --git a/src/hotspot/share/classfile/classFileError.cpp b/src/hotspot/share/classfile/classFileError.cpp
@@ -45,11 +45,11 @@ void ClassFileParser::classfile_parse_error(const char* msg, TRAPS) const {
                      msg, _class_name->as_C_string());
 }
 
+// The caller is required/expected to have a ResourceMark in this case.
 void ClassFileParser::classfile_parse_error(const char* msg,
                                             int index,
                                             TRAPS) const {
   assert(_class_name != nullptr, "invariant");
-  ResourceMark rm(THREAD);
   Exceptions::fthrow(THREAD_AND_LOCATION, vmSymbols::java_lang_ClassFormatError(),
                      msg, index, _class_name->as_C_string());
 }
@@ -92,6 +92,12 @@ void ClassFileParser::classfile_icce_error(const char* msg,
                      msg, _class_name->as_klass_external_name(), k->external_name());
 }
 
+void ClassFileParser::classfile_icce_error(const char* msg,
+                                           TRAPS) const {
+  ResourceMark rm(THREAD);
+  Exceptions::fthrow(THREAD_AND_LOCATION, vmSymbols::java_lang_IncompatibleClassChangeError(), msg);
+}
+
 void ClassFileParser::classfile_ucve_error(const char* msg,
                                            const Symbol* class_name,
                                            u2 major,
diff --git a/src/hotspot/share/classfile/classFileParser.cpp b/src/hotspot/share/classfile/classFileParser.cpp
@@ -4070,9 +4070,13 @@ void ClassFileParser::check_super_class_access(const InstanceKlass* this_klass,
       return;
     }
 
-    if (super_ik->is_sealed() && !super_ik->has_as_permitted_subclass(this_klass)) {
-      classfile_icce_error("class %s cannot inherit from sealed class %s", super_ik, THREAD);
-      return;
+    if (super_ik->is_sealed()) {
+      stringStream ss;
+      ResourceMark rm(THREAD);
+      if (!super_ik->has_as_permitted_subclass(this_klass, ss)) {
+        classfile_icce_error(ss.as_string(), THREAD);
+        return;
+      }
     }
 
     Reflection::VerifyClassAccessResults vca_result =
@@ -4117,12 +4121,13 @@ void ClassFileParser::check_super_interface_access(const InstanceKlass* this_kla
     InstanceKlass* const k = local_interfaces->at(i);
     assert (k != nullptr && k->is_interface(), "invalid interface");
 
-    if (k->is_sealed() && !k->has_as_permitted_subclass(this_klass)) {
-      classfile_icce_error(this_klass->is_interface() ?
-                             "class %s cannot extend sealed interface %s" :
-                             "class %s cannot implement sealed interface %s",
-                           k, THREAD);
-      return;
+    if (k->is_sealed()) {
+      stringStream ss;
+      ResourceMark rm(THREAD);
+      if (!k->has_as_permitted_subclass(this_klass, ss)) {
+        classfile_icce_error(ss.as_string(), THREAD);
+        return;
+      }
     }
 
     Reflection::VerifyClassAccessResults vca_result =
diff --git a/src/hotspot/share/classfile/classFileParser.hpp b/src/hotspot/share/classfile/classFileParser.hpp
@@ -367,6 +367,10 @@ class ClassFileParser {
                             const Klass* k,
                             TRAPS) const;
 
+  // Uses msg directly in the ICCE, with no additional content
+  void classfile_icce_error(const char* msg,
+                            TRAPS) const;
+
   void classfile_ucve_error(const char* msg,
                             const Symbol* class_name,
                             u2 major,
diff --git a/src/hotspot/share/classfile/moduleEntry.cpp b/src/hotspot/share/classfile/moduleEntry.cpp
@@ -742,7 +742,7 @@ void ModuleEntryTable::modules_do(ModuleClosure* closure) {
 void ModuleEntry::print(outputStream* st) {
   st->print_cr("entry " PTR_FORMAT " name %s module " PTR_FORMAT " loader %s version %s location %s strict %s",
                p2i(this),
-               name() == nullptr ? UNNAMED_MODULE : name()->as_C_string(),
+               name_as_C_string(),
                p2i(module()),
                loader_data()->loader_name_and_id(),
                version() != nullptr ? version()->as_C_string() : "nullptr",
diff --git a/src/hotspot/share/classfile/moduleEntry.hpp b/src/hotspot/share/classfile/moduleEntry.hpp
@@ -185,6 +185,10 @@ class ModuleEntry : public CHeapObj<mtModule> {
   static ModuleEntry* create_boot_unnamed_module(ClassLoaderData* cld);
   static ModuleEntry* new_unnamed_module_entry(Handle module_handle, ClassLoaderData* cld);
 
+  // Note caller requires ResourceMark
+  const char* name_as_C_string() {
+    return is_named() ? name()->as_C_string() : UNNAMED_MODULE;
+  }
   void print(outputStream* st = tty);
   void verify();
 
diff --git a/src/hotspot/share/oops/instanceKlass.cpp b/src/hotspot/share/oops/instanceKlass.cpp
@@ -212,31 +212,45 @@ bool InstanceKlass::has_nest_member(JavaThread* current, InstanceKlass* k) const
   return false;
 }
 
-// Called to verify that k is a permitted subclass of this class
-bool InstanceKlass::has_as_permitted_subclass(const InstanceKlass* k) const {
+// Called to verify that k is a permitted subclass of this class.
+// The incoming stringStream is used to format the messages for error logging and for the caller
+// to use for exception throwing.
+bool InstanceKlass::has_as_permitted_subclass(const InstanceKlass* k, stringStream& ss) const {
   Thread* current = Thread::current();
   assert(k != nullptr, "sanity check");
   assert(_permitted_subclasses != nullptr && _permitted_subclasses != Universe::the_empty_short_array(),
          "unexpected empty _permitted_subclasses array");
 
   if (log_is_enabled(Trace, class, sealed)) {
     ResourceMark rm(current);
-    log_trace(class, sealed)("Checking for permitted subclass of %s in %s",
+    log_trace(class, sealed)("Checking for permitted subclass %s in %s",
                              k->external_name(), this->external_name());
   }
 
   // Check that the class and its super are in the same module.
   if (k->module() != this->module()) {
-    ResourceMark rm(current);
-    log_trace(class, sealed)("Check failed for same module of permitted subclass %s and sealed class %s",
-                             k->external_name(), this->external_name());
+    ss.print("Failed same module check: subclass %s is in module '%s' with loader %s, "
+             "and sealed class %s is in module '%s' with loader %s",
+             k->external_name(),
+             k->module()->name_as_C_string(),
+             k->module()->loader_data()->loader_name_and_id(),
+             this->external_name(),
+             this->module()->name_as_C_string(),
+             this->module()->loader_data()->loader_name_and_id());
+    log_trace(class, sealed)(" - %s", ss.as_string());
     return false;
   }
 
   if (!k->is_public() && !is_same_class_package(k)) {
-    ResourceMark rm(current);
-    log_trace(class, sealed)("Check failed, subclass %s not public and not in the same package as sealed class %s",
-                             k->external_name(), this->external_name());
+    ss.print("Failed same package check: non-public subclass %s is in package '%s' with classloader %s, "
+             "and sealed class %s is in package '%s' with classloader %s",
+             k->external_name(),
+             k->package() != nullptr ? k->package()->name()->as_C_string() : "unnamed",
+             k->module()->loader_data()->loader_name_and_id(),
+             this->external_name(),
+             this->package() != nullptr ? this->package()->name()->as_C_string() : "unnamed",
+             this->module()->loader_data()->loader_name_and_id());
+    log_trace(class, sealed)(" - %s", ss.as_string());
     return false;
   }
 
@@ -248,7 +262,10 @@ bool InstanceKlass::has_as_permitted_subclass(const InstanceKlass* k) const {
       return true;
     }
   }
-  log_trace(class, sealed)("- class is NOT a permitted subclass!");
+
+  ss.print("Failed listed permitted subclass check: class %s is not a permitted subclass of %s",
+           k->external_name(), this->external_name());
+  log_trace(class, sealed)(" - %s", ss.as_string());
   return false;
 }
 
diff --git a/src/hotspot/share/oops/instanceKlass.hpp b/src/hotspot/share/oops/instanceKlass.hpp
@@ -456,8 +456,10 @@ class InstanceKlass: public Klass {
   // Check if this klass is a nestmate of k - resolves this nest-host and k's
   bool has_nestmate_access_to(InstanceKlass* k, TRAPS);
 
-  // Called to verify that k is a permitted subclass of this class
-  bool has_as_permitted_subclass(const InstanceKlass* k) const;
+  // Called to verify that k is a permitted subclass of this class.
+  // The incoming stringStream is used for logging, and for the caller to create
+  // a detailed exception message on failure.
+  bool has_as_permitted_subclass(const InstanceKlass* k, stringStream& ss) const;
 
   enum InnerClassAttributeOffset {
     // From http://mirror.eng/products/jdk/1.1/docs/guide/innerclasses/spec/innerclasses.doc10.html#18814
diff --git a/test/hotspot/jtreg/runtime/modules/SealedInterfaceModuleTest.java b/test/hotspot/jtreg/runtime/modules/SealedInterfaceModuleTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2024, 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
@@ -74,7 +74,9 @@ public static void main(String args[]) throws Throwable {
             Class p2_C2_class = Class.forName("sealedP2.C2");
             throw new RuntimeException("Expected IncompatibleClassChangeError exception not thrown");
         } catch (IncompatibleClassChangeError e) {
-            if (!e.getMessage().contains("cannot implement sealed interface")) {
+            if (!e.getMessage().equals("Failed same package check: non-public subclass sealedP2.C2 " +
+                                       "is in package 'sealedP2' with classloader 'app', and sealed class " +
+                                       "sealedP1.SuperInterface is in package 'sealedP1' with classloader 'app'")) {
                 throw new RuntimeException("Wrong IncompatibleClassChangeError exception thrown: " + e.getMessage());
             }
         }
@@ -86,7 +88,9 @@ public static void main(String args[]) throws Throwable {
             Class p3_C3_class = Class.forName("sealedP3.C3");
             throw new RuntimeException("Expected IncompatibleClassChangeError exception not thrown");
         } catch (IncompatibleClassChangeError e) {
-            if (!e.getMessage().contains("cannot implement sealed interface")) {
+            if (!e.getMessage().equals("Failed same module check: subclass sealedP3.C3 is in module 'module_two' " +
+                                       "with loader 'app', and sealed class sealedP1.SuperInterface is in module " +
+                                       "'module_one' with loader 'app'")) {
                 throw new RuntimeException("Wrong IncompatibleClassChangeError exception thrown: " + e.getMessage());
             }
         }
diff --git a/test/hotspot/jtreg/runtime/modules/SealedModuleTest.java b/test/hotspot/jtreg/runtime/modules/SealedModuleTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2024, 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
@@ -74,7 +74,9 @@ public static void main(String args[]) throws Throwable {
             Class p2_C2_class = Class.forName("sealedP2.C2");
             throw new RuntimeException("Expected IncompatibleClassChangeError exception not thrown");
         } catch (IncompatibleClassChangeError e) {
-            if (!e.getMessage().contains("cannot inherit from sealed class")) {
+            if (!e.getMessage().equals("Failed same package check: non-public subclass sealedP2.C2 " +
+                                       "is in package 'sealedP2' with classloader 'app', and sealed class " +
+                                       "sealedP1.SuperClass is in package 'sealedP1' with classloader 'app'")) {
                 throw new RuntimeException("Wrong IncompatibleClassChangeError exception thrown: " + e.getMessage());
             }
         }
@@ -86,7 +88,9 @@ public static void main(String args[]) throws Throwable {
             Class p3_C3_class = Class.forName("sealedP3.C3");
             throw new RuntimeException("Expected IncompatibleClassChangeError exception not thrown");
         } catch (IncompatibleClassChangeError e) {
-            if (!e.getMessage().contains("cannot inherit from sealed class")) {
+            if (!e.getMessage().equals("Failed same module check: subclass sealedP3.C3 is in module 'module_two' " +
+                                       "with loader 'app', and sealed class sealedP1.SuperClass is in module " +
+                                       "'module_one' with loader 'app'")) {
                 throw new RuntimeException("Wrong IncompatibleClassChangeError exception thrown: " + e.getMessage());
             }
         }
diff --git a/test/hotspot/jtreg/runtime/sealedClasses/GetPermittedSubclassesTest.java b/test/hotspot/jtreg/runtime/sealedClasses/GetPermittedSubclassesTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2024, 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
@@ -49,6 +49,7 @@ final class NotSealed implements SealedI1 {}
 
     final class Final4 {}
 
+    // Check if the given class has the expected permitted subclasses
     public static void testSealedInfo(Class<?> c, String[] expected, boolean isSealed) {
         var permitted = c.getPermittedSubclasses();
 
@@ -92,16 +93,16 @@ public static void testSealedInfo(Class<?> c, String[] expected, boolean isSeale
 
     public static void testBadSealedClass(String className,
                                           Class<?> expectedException,
-                                          String expectedCFEMessage) throws Throwable {
+                                          String expectedMessage) throws Throwable {
         try {
             Class.forName(className);
-            throw new RuntimeException("Expected ClassFormatError exception not thrown for " + className);
+            throw new RuntimeException("Expected exception " + expectedException.getName() + " not thrown for " + className);
         } catch (ClassFormatError cfe) {
             if (ClassFormatError.class != expectedException) {
                 throw new RuntimeException(
                     "Class " + className + " got unexpected exception: " + cfe.getMessage());
             }
-            if (!cfe.getMessage().contains(expectedCFEMessage)) {
+            if (!cfe.getMessage().contains(expectedMessage)) {
                 throw new RuntimeException(
                     "Class " + className + " got unexpected ClassFormatError exception: " + cfe.getMessage());
             }
@@ -110,7 +111,7 @@ public static void testBadSealedClass(String className,
                 throw new RuntimeException(
                     "Class " + className + " got unexpected exception: " + icce.getMessage());
             }
-            if (!icce.getMessage().contains(expectedCFEMessage)) {
+            if (!icce.getMessage().contains(expectedMessage)) {
                 throw new RuntimeException(
                     "Class " + className + " got unexpected IncompatibleClassChangeError exception: " + icce.getMessage());
             }
@@ -135,7 +136,7 @@ public static void main(String... args) throws Throwable {
 
         // Test that a class with an empty PermittedSubclasses attribute cannot be subclass-ed.
         testBadSealedClass("SubClass", IncompatibleClassChangeError.class,
-                           "SubClass cannot inherit from sealed class NoSubclasses");
+                           "Failed listed permitted subclass check: class SubClass is not a permitted subclass of NoSubclasses");
 
         // Test returning only names of existing classes.
         testSealedInfo(NoLoadSubclasses.class, new String[]{"ExistingClassFile" }, true);
diff --git a/test/hotspot/jtreg/runtime/sealedClasses/SealedDifferentUnnamedModuleTest.java b/test/hotspot/jtreg/runtime/sealedClasses/SealedDifferentUnnamedModuleTest.java
@@ -0,0 +1,56 @@
+/*
+ * Copyright (c) 2024, 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 8345911
+ * @library /test/lib
+ * @compile SealedSuper.java SealedSub.java
+ * @comment Copy SealedSuper.class to the currnet directory so it will be on the bootclasspath
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller SealedSuper
+ * @run main/othervm -Xbootclasspath/a:. -Xlog:class+sealed=trace SealedDifferentUnnamedModuleTest
+ */
+
+public class SealedDifferentUnnamedModuleTest {
+
+    public static void main(String args[]) throws Throwable {
+
+        // Load the sealed superclass. It will be loaded by the boot loader and
+        // so reside in the boot loaders un-named module.
+        Class<?> c1 = Class.forName("SealedSuper");
+
+        // Test loading a "permitted" subclass in the app classloader, which then resides
+        // in the app loader's un-named module.
+        // This should fail.
+        try {
+            Class<?> c2 = Class.forName("SealedSub");
+            throw new RuntimeException("Expected IncompatibleClassChangeError exception not thrown");
+        } catch (IncompatibleClassChangeError e) {
+            if (!e.getMessage().equals("Failed same module check: subclass SealedSub is in module 'unnamed module' " +
+                                       "with loader 'app', and sealed class SealedSuper is in module 'unnamed module' " +
+                                       "with loader 'bootstrap'")) {
+                throw new RuntimeException("Wrong IncompatibleClassChangeError exception thrown: " + e.getMessage());
+            }
+        }
+    }
+}
diff --git a/test/hotspot/jtreg/runtime/sealedClasses/SealedSub.java b/test/hotspot/jtreg/runtime/sealedClasses/SealedSub.java
@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2024, 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.
+ */
+public final class SealedSub extends SealedSuper {
+}
diff --git a/test/hotspot/jtreg/runtime/sealedClasses/SealedSuper.java b/test/hotspot/jtreg/runtime/sealedClasses/SealedSuper.java
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2024, 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.
+ */
+
+public sealed class SealedSuper permits SealedSub {
+}
diff --git a/test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleIntfTest.java b/test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleIntfTest.java
diff --git a/test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleTest.java b/test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleTest.java

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`/*`
`2`		`- * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.`
	`2`	`+ * Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights reserved.`
`3`	`3`	`* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.`
`4`	`4`	`*`
`5`	`5`	`* This code is free software; you can redistribute it and/or modify it`
`@@ -74,7 +74,9 @@ public static void main(String args[]) throws Throwable {`
`74`	`74`	`Class p2_C2_class = Class.forName("sealedP2.C2");`
`75`	`75`	`throw new RuntimeException("Expected IncompatibleClassChangeError exception not thrown");`
`76`	`76`	`} catch (IncompatibleClassChangeError e) {`
`77`		`- if (!e.getMessage().contains("cannot implement sealed interface")) {`
	`77`	`+ if (!e.getMessage().equals("Failed same package check: non-public subclass sealedP2.C2 " +`
	`78`	`+ "is in package 'sealedP2' with classloader 'app', and sealed class " +`
	`79`	`+ "sealedP1.SuperInterface is in package 'sealedP1' with classloader 'app'")) {`
`78`	`80`	`throw new RuntimeException("Wrong IncompatibleClassChangeError exception thrown: " + e.getMessage());`
`79`	`81`	`}`
`80`	`82`	`}`
`@@ -86,7 +88,9 @@ public static void main(String args[]) throws Throwable {`
`86`	`88`	`Class p3_C3_class = Class.forName("sealedP3.C3");`
`87`	`89`	`throw new RuntimeException("Expected IncompatibleClassChangeError exception not thrown");`
`88`	`90`	`} catch (IncompatibleClassChangeError e) {`
`89`		`- if (!e.getMessage().contains("cannot implement sealed interface")) {`
	`91`	`+ if (!e.getMessage().equals("Failed same module check: subclass sealedP3.C3 is in module 'module_two' " +`
	`92`	`+ "with loader 'app', and sealed class sealedP1.SuperInterface is in module " +`
	`93`	`+ "'module_one' with loader 'app'")) {`
`90`	`94`	`throw new RuntimeException("Wrong IncompatibleClassChangeError exception thrown: " + e.getMessage());`
`91`	`95`	`}`
`92`	`96`	`}`