8245487: Potential double-free of interfaces array

Free _local_interfaces when we free _transitive_interfaces since they can share memory.

Reviewed-by: fparain, lfoltan, iklam

src/hotspot/share/classfile/classFileParser.cpp

@@ -3949,12 +3949,12 @@ void ClassFileParser::apply_parsed_class_metadata(
   this_klass->set_inner_classes(_inner_classes);
   this_klass->set_nest_members(_nest_members);
   this_klass->set_nest_host_index(_nest_host);
-  this_klass->set_local_interfaces(_local_interfaces);
   this_klass->set_annotations(_combined_annotations);
   this_klass->set_permitted_subclasses(_permitted_subclasses);
   this_klass->set_record_components(_record_components);
-  // Delay the setting of _transitive_interfaces until after initialize_supers() in
-  // fill_instance_klass(). It is because the _transitive_interfaces may be shared with
+  // Delay the setting of _local_interfaces and _transitive_interfaces until after
+  // initialize_supers() in fill_instance_klass(). It is because the _local_interfaces could
+  // be shared with _transitive_interfaces and _transitive_interfaces may be shared with
   // its _super. If an OOM occurs while loading the current klass, its _super field
   // may not have been set. When GC tries to free the klass, the _transitive_interfaces
   // may be deallocated mistakenly in InstanceKlass::deallocate_interfaces(). Subsequent
@@ -5788,7 +5788,6 @@ void ClassFileParser::fill_instance_klass(InstanceKlass* ik,
   assert(NULL == _methods, "invariant");
   assert(NULL == _inner_classes, "invariant");
   assert(NULL == _nest_members, "invariant");
-  assert(NULL == _local_interfaces, "invariant");
   assert(NULL == _combined_annotations, "invariant");
   assert(NULL == _record_components, "invariant");
   assert(NULL == _permitted_subclasses, "invariant");
@@ -5862,7 +5861,9 @@ void ClassFileParser::fill_instance_klass(InstanceKlass* ik,
   // Fill in information needed to compute superclasses.
   ik->initialize_supers(const_cast<InstanceKlass*>(_super_klass), _transitive_interfaces, CHECK);
   ik->set_transitive_interfaces(_transitive_interfaces);
+  ik->set_local_interfaces(_local_interfaces);
   _transitive_interfaces = NULL;
+  _local_interfaces = NULL;
 
   // Initialize itable offset tables
   klassItable::setup_itable_offset_table(ik);
@@ -6190,7 +6191,6 @@ void ClassFileParser::clear_class_metadata() {
   _inner_classes = NULL;
   _nest_members = NULL;
   _permitted_subclasses = NULL;
-  _local_interfaces = NULL;
   _combined_annotations = NULL;
   _class_annotations = _class_type_annotations = NULL;
   _fields_annotations = _fields_type_annotations = NULL;
@@ -6258,6 +6258,7 @@ ClassFileParser::~ClassFileParser() {
 
   clear_class_metadata();
   _transitive_interfaces = NULL;
+  _local_interfaces = NULL;
 
   // deallocate the klass if already created.  Don't directly deallocate, but add
   // to the deallocate list so that the klass is removed from the CLD::_klasses list

src/hotspot/share/classfile/classLoaderDataGraph.cpp

@@ -666,6 +666,13 @@ ClassLoaderMetaspace* ClassLoaderDataGraphMetaspaceIterator::get_next() {
   return result;
 }
 
+void ClassLoaderDataGraph::verify() {
+  ClassLoaderDataGraphIterator iter;
+  while (ClassLoaderData* cld = iter.get_next()) {
+    cld->verify();
+  }
+}
+
 #ifndef PRODUCT
 // callable from debugger
 extern "C" int print_loader_data_graph() {
@@ -674,13 +681,6 @@ extern "C" int print_loader_data_graph() {
   return 0;
 }
 
-void ClassLoaderDataGraph::verify() {
-  ClassLoaderDataGraphIterator iter;
-  while (ClassLoaderData* cld = iter.get_next()) {
-    cld->verify();
-  }
-}
-
 void ClassLoaderDataGraph::print_on(outputStream * const out) {
   ClassLoaderDataGraphIterator iter;
   while (ClassLoaderData* cld = iter.get_next()) {

src/hotspot/share/memory/universe.cpp

@@ -1094,12 +1094,10 @@ void Universe::verify(VerifyOption option, const char* prefix) {
     log_debug(gc, verify)("SystemDictionary");
     SystemDictionary::verify();
   }
-#ifndef PRODUCT
   if (should_verify_subset(Verify_ClassLoaderDataGraph)) {
     log_debug(gc, verify)("ClassLoaderDataGraph");
     ClassLoaderDataGraph::verify();
   }
-#endif
   if (should_verify_subset(Verify_MetaspaceUtils)) {
     log_debug(gc, verify)("MetaspaceUtils");
     MetaspaceUtils::verify_free_chunks();

test/hotspot/jtreg/runtime/classFileParserBug/BadClassPackage.jasm

@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2020, 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.
+ */
+
+package  java/lang;
+
+super public class BadClassPackage
+	implements java/util/function/Function
+	version 47:0
+{
+
+
+public Method "<init>":"()V"
+	stack 1 locals 1
+{
+		aload_0;
+		invokespecial	Method Object."<init>":"()V";
+		return;
+}
+} // end Class BadClassPackage

test/hotspot/jtreg/runtime/classFileParserBug/TestBadPackageWithInterface.java

@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2020, 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 8245487
+ * @summary Check that if the VM rejects classes from packages starting with "java/", it will exit
+ *          cleanly after InstanceKlass::verify_on(), and not leave freed memory in _local_interfaces.
+ * @library /test/lib
+ * @compile BadClassPackage.jasm
+ * @run driver TestBadPackageWithInterface
+ */
+
+import java.io.InputStream;
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.process.ProcessTools;
+
+public class TestBadPackageWithInterface {
+    public static void main(String args[]) throws Throwable {
+        ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
+            "-cp", System.getProperty("test.classes"),
+            "-XX:+UnlockDiagnosticVMOptions",
+            "-XX:+VerifyBeforeExit", MyLoader.class.getName());
+        OutputAnalyzer oa = new OutputAnalyzer(pb.start());
+        oa.shouldHaveExitValue(0);
+
+    }
+
+    static class MyLoader extends ClassLoader {
+        public static void main(String args[]) throws Throwable {
+            try {
+                ClassLoader loader = TestBadPackageWithInterface.class.getClassLoader();
+                InputStream in = loader.getResourceAsStream("java/lang/BadClassPackage.class");
+                byte[] bytes = in.readAllBytes();
+
+                MyLoader myLoader = new MyLoader();
+                myLoader.defineClass(bytes, 0, bytes.length);
+            }
+            catch (SecurityException expected) {
+                System.out.println("Expected ==================================================");
+                expected.printStackTrace(System.out);
+                System.out.println("==================================================");
+            }
+        }
+    }
+}