8209385: CDS runtime classpath checking is too strict when only classes from the system modules are archived
authorccheung
Fri, 17 Aug 2018 14:50:59 -0700
changeset 51439 0517bd2a0eda
parent 51438 6ca468ea3564
child 51440 a1aaf68b119d
8209385: CDS runtime classpath checking is too strict when only classes from the system modules are archived Summary: skip checking the path entries which are not being referenced during CDS dump time Reviewed-by: jiangli, iklam
src/hotspot/share/classfile/classLoaderExt.cpp
src/hotspot/share/classfile/classLoaderExt.hpp
src/hotspot/share/classfile/sharedPathsMiscInfo.cpp
src/hotspot/share/memory/filemap.cpp
src/hotspot/share/memory/filemap.hpp
test/hotspot/jtreg/runtime/appcds/DirClasspathTest.java
test/hotspot/jtreg/runtime/appcds/PrintSharedArchiveAndExit.java
test/hotspot/jtreg/runtime/appcds/UnusedCPDuringDump.java
--- a/src/hotspot/share/classfile/classLoaderExt.cpp	Fri Aug 17 22:20:47 2018 +0100
+++ b/src/hotspot/share/classfile/classLoaderExt.cpp	Fri Aug 17 14:50:59 2018 -0700
@@ -50,6 +50,7 @@
 
 jshort ClassLoaderExt::_app_class_paths_start_index = ClassLoaderExt::max_classpath_index;
 jshort ClassLoaderExt::_app_module_paths_start_index = ClassLoaderExt::max_classpath_index;
+jshort ClassLoaderExt::_max_used_path_index = 0;
 bool ClassLoaderExt::_has_app_classes = false;
 bool ClassLoaderExt::_has_platform_classes = false;
 
@@ -242,6 +243,9 @@
     classloader_type = ClassLoader::PLATFORM_LOADER;
     ClassLoaderExt::set_has_platform_classes();
   }
+  if (classpath_index > ClassLoaderExt::max_used_path_index()) {
+    ClassLoaderExt::set_max_used_path_index(classpath_index);
+  }
   result->set_shared_classpath_index(classpath_index);
   result->set_class_loader_type(classloader_type);
 }
--- a/src/hotspot/share/classfile/classLoaderExt.hpp	Fri Aug 17 22:20:47 2018 +0100
+++ b/src/hotspot/share/classfile/classLoaderExt.hpp	Fri Aug 17 14:50:59 2018 -0700
@@ -49,6 +49,8 @@
   static jshort _app_class_paths_start_index;
   // index of first modular JAR in shared modulepath entry table
   static jshort _app_module_paths_start_index;
+  // the largest path index being used during CDS dump time
+  static jshort _max_used_path_index;
 
   static bool _has_app_classes;
   static bool _has_platform_classes;
@@ -91,6 +93,12 @@
 
   static jshort app_module_paths_start_index() { return _app_module_paths_start_index; }
 
+  static jshort max_used_path_index() { return _max_used_path_index; }
+
+  static void set_max_used_path_index(jshort used_index) {
+    _max_used_path_index = used_index;
+  }
+
   static void init_paths_start_index(jshort app_start) {
     _app_class_paths_start_index = app_start;
   }
--- a/src/hotspot/share/classfile/sharedPathsMiscInfo.cpp	Fri Aug 17 22:20:47 2018 +0100
+++ b/src/hotspot/share/classfile/sharedPathsMiscInfo.cpp	Fri Aug 17 14:50:59 2018 -0700
@@ -115,10 +115,15 @@
     return fail("Corrupted archive file header");
   }
 
+  jshort cur_index = 0;
+  jshort max_cp_index = FileMapInfo::current_info()->header()->max_used_path_index();
+  jshort module_paths_start_index =
+    FileMapInfo::current_info()->header()->app_module_paths_start_index();
   while (_cur_ptr < _end_ptr) {
     jint type;
     const char* path = _cur_ptr;
     _cur_ptr += strlen(path) + 1;
+
     if (!read_jint(&type)) {
       return fail("Corrupted archive file header");
     }
@@ -129,13 +134,19 @@
       print_path(&ls, type, path);
       ls.cr();
     }
-    if (!check(type, path)) {
-      if (!PrintSharedArchiveAndExit) {
-        return false;
+    // skip checking the class path(s) which was not referenced during CDS dump
+    if ((cur_index <= max_cp_index) || (cur_index >= module_paths_start_index)) {
+      if (!check(type, path)) {
+        if (!PrintSharedArchiveAndExit) {
+          return false;
+        }
+      } else {
+        ClassLoader::trace_class_path("ok");
       }
     } else {
-      ClassLoader::trace_class_path("ok");
+      ClassLoader::trace_class_path("skipped check");
     }
+    cur_index++;
   }
 
   return true;
--- a/src/hotspot/share/memory/filemap.cpp	Fri Aug 17 22:20:47 2018 +0100
+++ b/src/hotspot/share/memory/filemap.cpp	Fri Aug 17 14:50:59 2018 -0700
@@ -199,6 +199,7 @@
   ClassLoaderExt::finalize_shared_paths_misc_info();
   _app_class_paths_start_index = ClassLoaderExt::app_class_paths_start_index();
   _app_module_paths_start_index = ClassLoaderExt::app_module_paths_start_index();
+  _max_used_path_index = ClassLoaderExt::max_used_path_index();
 
   _verify_local = BytecodeVerificationLocal;
   _verify_remote = BytecodeVerificationRemote;
@@ -359,13 +360,13 @@
 
   bool has_nonempty_dir = false;
 
-  int end = _shared_path_table_size;
-  if (!ClassLoaderExt::has_platform_or_app_classes()) {
-    // only check the boot path if no app class is loaded
-    end = ClassLoaderExt::app_class_paths_start_index();
+  int last = _shared_path_table_size - 1;
+  if (last > ClassLoaderExt::max_used_path_index()) {
+     // no need to check any path beyond max_used_path_index
+     last = ClassLoaderExt::max_used_path_index();
   }
 
-  for (int i = 0; i < end; i++) {
+  for (int i = 0; i <= last; i++) {
     SharedClassPathEntry *e = shared_path(i);
     if (e->is_dir()) {
       const char* path = e->name();
@@ -467,13 +468,8 @@
 
   int module_paths_start_index = _header->_app_module_paths_start_index;
 
-  // If the shared archive contain app or platform classes, validate all entries
-  // in the shared path table. Otherwise, only validate the boot path entries (with
-  // entry index < _app_class_paths_start_index).
-  int count = _header->has_platform_or_app_classes() ?
-              _shared_path_table_size : _header->_app_class_paths_start_index;
-
-  for (int i=0; i<count; i++) {
+  // validate the path entries up to the _max_used_path_index
+  for (int i=0; i < _header->_max_used_path_index + 1; i++) {
     if (i < module_paths_start_index) {
       if (shared_path(i)->validate()) {
         log_info(class, path)("ok");
--- a/src/hotspot/share/memory/filemap.hpp	Fri Aug 17 22:20:47 2018 +0100
+++ b/src/hotspot/share/memory/filemap.hpp	Fri Aug 17 14:50:59 2018 -0700
@@ -192,6 +192,7 @@
 
     jshort _app_class_paths_start_index;  // Index of first app classpath entry
     jshort _app_module_paths_start_index; // Index of first module path entry
+    jshort _max_used_path_index;          // max path index referenced during CDS dump
     bool   _verify_local;                 // BytecodeVerificationLocal setting
     bool   _verify_remote;                // BytecodeVerificationRemote setting
     bool   _has_platform_or_app_classes;  // Archive contains app classes
@@ -200,6 +201,8 @@
       _has_platform_or_app_classes = v;
     }
     bool has_platform_or_app_classes() { return _has_platform_or_app_classes; }
+    jshort max_used_path_index()       { return _max_used_path_index; }
+    jshort app_module_paths_start_index() { return _app_module_paths_start_index; }
 
     char* region_addr(int idx);
 
--- a/test/hotspot/jtreg/runtime/appcds/DirClasspathTest.java	Fri Aug 17 22:20:47 2018 +0100
+++ b/test/hotspot/jtreg/runtime/appcds/DirClasspathTest.java	Fri Aug 17 14:50:59 2018 -0700
@@ -27,12 +27,14 @@
  * @summary Handling of directories in -cp is based on the classlist
  * @requires vm.cds
  * @library /test/lib
+ * @compile test-classes/Hello.java
  * @run main DirClasspathTest
  */
 
 import jdk.test.lib.Platform;
 import jdk.test.lib.process.OutputAnalyzer;
 import java.io.File;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Arrays;
@@ -87,13 +89,28 @@
         /////////////////////////////////////////////////////////////////
         String appClassList[] = {"java/lang/Object", "com/sun/tools/javac/Main"};
 
-        // Non-empty dir in -cp: should report error
+        // Non-empty dir in -cp: should be OK (as long as no classes were loaded from there)
         output = TestCommon.dump(dir.getPath(), appClassList, "-Xlog:class+path=info");
+        TestCommon.checkDump(output);
+
+        // Long path to non-empty dir in -cp: should be OK (as long as no classes were loaded from there)
+        output = TestCommon.dump(longDir.getPath(), appClassList, "-Xlog:class+path=info");
+        TestCommon.checkDump(output);
+
+        /////////////////////////////////////////////////////////////////
+        // Loading an app class from a directory
+        /////////////////////////////////////////////////////////////////
+        String appClassList2[] = {"Hello", "java/lang/Object", "com/sun/tools/javac/Main"};
+        // Non-empty dir in -cp: should report error if a class is loaded from it
+        output = TestCommon.dump(classDir.toString(), appClassList2, "-Xlog:class+path=info");
         output.shouldNotHaveExitValue(0);
         output.shouldContain("Cannot have non-empty directory in paths");
 
-        // Long path to non-empty dir in -cp: should report error
-        output = TestCommon.dump(longDir.getPath(), appClassList, "-Xlog:class+path=info");
+        // Long path to non-empty dir in -cp: should report error if a class is loaded from it
+        File srcClass = new File(classDir.toFile(), "Hello.class");
+        File destClass = new File(longDir, "Hello.class");
+        Files.copy(srcClass.toPath(), destClass.toPath());
+        output = TestCommon.dump(longDir.getPath(), appClassList2, "-Xlog:class+path=info");
         output.shouldNotHaveExitValue(0);
         output.shouldContain("Cannot have non-empty directory in paths");
     }
--- a/test/hotspot/jtreg/runtime/appcds/PrintSharedArchiveAndExit.java	Fri Aug 17 22:20:47 2018 +0100
+++ b/test/hotspot/jtreg/runtime/appcds/PrintSharedArchiveAndExit.java	Fri Aug 17 14:50:59 2018 -0700
@@ -65,7 +65,7 @@
     String cp = appJar + File.pathSeparator + appJar2;
     String lastCheckMsg = "checking shared classpath entry: " + appJar2; // the last JAR to check
 
-    TestCommon.testDump(cp, TestCommon.list("Hello"));
+    TestCommon.testDump(cp, TestCommon.list("Hello", "HelloMore"));
 
     log("Normal execution -- all the JAR paths should be checked");
     TestCommon.run(
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/hotspot/jtreg/runtime/appcds/UnusedCPDuringDump.java	Fri Aug 17 14:50:59 2018 -0700
@@ -0,0 +1,63 @@
+/*
+ * Copyright (c) 2018, 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 8209385
+ * @summary non-empty dir in -cp should be fine during dump time if only classes
+ *          from the system modules are being loaded even though some are
+ *          defined to the PlatformClassLoader and AppClassLoader.
+ * @requires vm.cds
+ * @library /test/lib
+ * @modules jdk.jartool/sun.tools.jar
+ * @compile test-classes/Hello.java
+ * @run main/othervm -Dtest.cds.copy.child.stdout=false UnusedCPDuringDump
+ */
+
+import java.io.File;
+import jdk.test.lib.process.OutputAnalyzer;
+
+public class UnusedCPDuringDump {
+
+    public static void main(String[] args) throws Exception {
+        File dir = new File(System.getProperty("user.dir"));
+        File emptydir = new File(dir, "emptydir");
+        emptydir.mkdir();
+        String appJar = JarBuilder.getOrCreateHelloJar();
+
+        OutputAnalyzer output = TestCommon.dump(dir.getPath(),
+            TestCommon.list("sun/util/resources/cldr/provider/CLDRLocaleDataMetaInfo",
+                            "com/sun/tools/sjavac/client/ClientMain"),
+                            "-Xlog:class+path=info,class+load=debug");
+        TestCommon.checkDump(output,
+                             "[class,load] sun.util.resources.cldr.provider.CLDRLocaleDataMetaInfo",
+                             "for instance a 'jdk/internal/loader/ClassLoaders$PlatformClassLoader'",
+                             "[class,load] com.sun.tools.sjavac.client.ClientMain",
+                             "for instance a 'jdk/internal/loader/ClassLoaders$AppClassLoader'");
+
+        String jsaOpt = "-XX:SharedArchiveFile=" + TestCommon.getCurrentArchiveName();
+        TestCommon.run("-cp", appJar, jsaOpt, "-Xlog:class+path=info,class+load=debug", "Hello")
+            .assertNormalExit("Hello World");
+  }
+}