8276126: Dump time class transformation causes heap objects of non-boot classes to be archived

Reviewed-by: iklam, minqi

src/hotspot/share/cds/heapShared.cpp

@@ -71,6 +71,7 @@
 bool HeapShared::_closed_regions_mapped = false;
 bool HeapShared::_open_regions_mapped = false;
 bool HeapShared::_is_loaded = false;
+bool HeapShared::_disable_writing = false;
 address   HeapShared::_narrow_oop_base;
 int       HeapShared::_narrow_oop_shift;
 DumpedInternedStrings *HeapShared::_dumped_interned_strings = NULL;

src/hotspot/share/cds/heapShared.hpp

@@ -155,10 +155,18 @@ public:
 
   // Can this VM write heap regions into the CDS archive? Currently only G1+compressed{oops,cp}
   static bool can_write() {
-    CDS_JAVA_HEAP_ONLY(return (UseG1GC && UseCompressedOops && UseCompressedClassPointers);)
+    CDS_JAVA_HEAP_ONLY(
+      if (_disable_writing) {
+        return false;
+      }
+      return (UseG1GC && UseCompressedOops && UseCompressedClassPointers);
+    )
     NOT_CDS_JAVA_HEAP(return false;)
   }
 
+  static void disable_writing() {
+    CDS_JAVA_HEAP_ONLY(_disable_writing = true;)
+  }
   // Can this VM map archived heap regions? Currently only G1+compressed{oops,cp}
   static bool can_map() {
     CDS_JAVA_HEAP_ONLY(return (UseG1GC && UseCompressedOops && UseCompressedClassPointers);)
@@ -188,6 +196,7 @@ public:
 
 private:
 #if INCLUDE_CDS_JAVA_HEAP
+  static bool _disable_writing;
   static bool _closed_regions_mapped;
   static bool _open_regions_mapped;
   static bool _is_loaded;

src/hotspot/share/classfile/classLoader.cpp

@@ -1253,7 +1253,8 @@ char* ClassLoader::skip_uri_protocol(char* source) {
 
 // Record the shared classpath index and loader type for classes loaded
 // by the builtin loaders at dump time.
-void ClassLoader::record_result(JavaThread* current, InstanceKlass* ik, const ClassFileStream* stream) {
+void ClassLoader::record_result(JavaThread* current, InstanceKlass* ik,
+                                const ClassFileStream* stream, bool redefined) {
   Arguments::assert_is_dumping_archive();
   assert(stream != NULL, "sanity");
 
@@ -1337,10 +1338,10 @@ void ClassLoader::record_result(JavaThread* current, InstanceKlass* ik, const Cl
       }
     }
 
-    // No path entry found for this class. Must be a shared class loaded by the
+    // No path entry found for this class: most likely a shared class loaded by the
     // user defined classloader.
-    if (classpath_index < 0) {
-      assert(ik->shared_classpath_index() < 0, "Sanity");
+    if (classpath_index < 0 && !SystemDictionaryShared::is_builtin_loader(ik->class_loader_data())) {
+      assert(ik->shared_classpath_index() < 0, "not assigned yet");
       ik->set_shared_classpath_index(UNREGISTERED_INDEX);
       SystemDictionaryShared::set_shared_class_misc_info(ik, (ClassFileStream*)stream);
       return;
@@ -1359,7 +1360,7 @@ void ClassLoader::record_result(JavaThread* current, InstanceKlass* ik, const Cl
                                                          ik->name()->utf8_length());
   assert(file_name != NULL, "invariant");
 
-  ClassLoaderExt::record_result(classpath_index, ik);
+  ClassLoaderExt::record_result(classpath_index, ik, redefined);
 }
 #endif // INCLUDE_CDS
 

src/hotspot/share/classfile/classLoader.hpp

@@ -365,7 +365,8 @@ class ClassLoader: AllStatic {
   static int num_module_path_entries();
   static void  exit_with_path_failure(const char* error, const char* message);
   static char* skip_uri_protocol(char* source);
-  static void  record_result(JavaThread* current, InstanceKlass* ik, const ClassFileStream* stream);
+  static void  record_result(JavaThread* current, InstanceKlass* ik,
+                             const ClassFileStream* stream, bool redefined);
 #endif
 
   static char* lookup_vm_options();

src/hotspot/share/classfile/classLoaderExt.cpp

@@ -24,6 +24,7 @@
 
 #include "precompiled.hpp"
 #include "cds/filemap.hpp"
+#include "cds/heapShared.hpp"
 #include "classfile/classFileParser.hpp"
 #include "classfile/classLoader.inline.hpp"
 #include "classfile/classLoaderExt.hpp"
@@ -227,7 +228,7 @@ void ClassLoaderExt::setup_search_paths(JavaThread* current) {
   ClassLoaderExt::setup_app_search_path(current);
 }
 
-void ClassLoaderExt::record_result(const s2 classpath_index, InstanceKlass* result) {
+void ClassLoaderExt::record_result(const s2 classpath_index, InstanceKlass* result, bool redefined) {
   Arguments::assert_is_dumping_archive();
 
   // We need to remember where the class comes from during dumping.
@@ -245,4 +246,21 @@ void ClassLoaderExt::record_result(const s2 classpath_index, InstanceKlass* resu
   }
   result->set_shared_classpath_index(classpath_index);
   result->set_shared_class_loader_type(classloader_type);
+#if INCLUDE_CDS_JAVA_HEAP
+  if (DumpSharedSpaces && AllowArchivingWithJavaAgent && classloader_type == ClassLoader::BOOT_LOADER &&
+      classpath_index < 0 && HeapShared::can_write() && redefined) {
+    // During static dump, classes for the built-in loaders are always loaded from
+    // known locations (jimage, classpath or modulepath), so classpath_index should
+    // always be >= 0.
+    // The only exception is when a java agent is used during dump time (for testing
+    // purposes only). If a class is transformed by the agent, the CodeSource of
+    // this class may point to an unknown location. This may break heap object archiving,
+    // which requires all the boot classes to be from known locations. This is an
+    // uncommon scenario (even in test cases). Let's simply disable heap object archiving.
+    ResourceMark rm;
+    log_warning(cds)("CDS heap objects cannot be written because class %s maybe modified by ClassFileLoadHook.",
+                     result->external_name());
+    HeapShared::disable_writing();
+  }
+#endif // INCLUDE_CDS_JAVA_HEAP
 }

src/hotspot/share/classfile/classLoaderExt.hpp

@@ -111,7 +111,7 @@ public:
     return _has_non_jar_in_classpath;
   }
 
-  static void record_result(const s2 classpath_index, InstanceKlass* result);
+  static void record_result(const s2 classpath_index, InstanceKlass* result, bool redefined);
   static void set_has_app_classes() {
     _has_app_classes = true;
   }

src/hotspot/share/classfile/klassFactory.cpp

@@ -211,7 +211,7 @@ InstanceKlass* KlassFactory::create_from_stream(ClassFileStream* stream,
 
 #if INCLUDE_CDS
   if (Arguments::is_dumping_archive()) {
-    ClassLoader::record_result(THREAD, result, stream);
+    ClassLoader::record_result(THREAD, result, stream, old_stream != stream);
   }
 #endif // INCLUDE_CDS
 

test/hotspot/jtreg/TEST.groups

@@ -354,6 +354,7 @@ hotspot_appcds_dynamic = \
  -runtime/cds/appcds/LambdaEagerInit.java \
  -runtime/cds/appcds/LambdaProxyClasslist.java \
  -runtime/cds/appcds/LambdaVerificationFailedDuringDump.java \
+ -runtime/cds/appcds/LambdaWithJavaAgent.java \
  -runtime/cds/appcds/LambdaWithOldClass.java \
  -runtime/cds/appcds/LongClassListPath.java \
  -runtime/cds/appcds/LotsOfClasses.java \

test/hotspot/jtreg/runtime/cds/appcds/LambdaWithJavaAgent.java

@@ -0,0 +1,93 @@
+/*
+ * 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 8276126
+ * @summary Test static dumping with java agent transforming a class loaded
+ *          by the boot class loader.
+ * @requires vm.cds.write.archived.java.heap
+ * @requires vm.jvmti
+ * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds/test-classes
+ * @compile test-classes/Hello.java
+ * @compile test-classes/TransformBootClass.java
+ * @run driver LambdaWithJavaAgent
+ */
+
+import jdk.test.lib.cds.CDSOptions;
+import jdk.test.lib.cds.CDSTestUtils;
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.helpers.ClassFileInstaller;
+
+public class LambdaWithJavaAgent {
+
+    public static String agentClasses[] = {
+        TransformBootClass.class.getName(),
+    };
+
+    public static void main(String[] args) throws Exception {
+        String mainClass = Hello.class.getName();
+        String namePrefix = "lambda-with-java-agent";
+        JarBuilder.build(namePrefix, mainClass);
+
+        String appJar = TestCommon.getTestJar(namePrefix + ".jar");
+        String classList = namePrefix + ".list";
+        String archiveName = namePrefix + ".jsa";
+
+        String agentJar =
+            ClassFileInstaller.writeJar("TransformBootClass.jar",
+                                        ClassFileInstaller.Manifest.fromSourceFile("test-classes/TransformBootClass.mf"),
+                                        agentClasses);
+        String useJavaAgent = "-javaagent:" + agentJar + "=jdk/internal/math/FDBigInteger";
+
+        // dump class list
+        CDSTestUtils.dumpClassList(classList, "-cp", appJar, mainClass);
+
+        // create archive with the class list
+        CDSOptions opts = (new CDSOptions())
+            .addPrefix("-XX:ExtraSharedClassListFile=" + classList,
+                       "-cp", appJar,
+                       "-XX:+AllowArchivingWithJavaAgent",
+                       useJavaAgent,
+                       "-Xlog:class+load,cds+class=debug,cds")
+            .setArchiveName(archiveName);
+        OutputAnalyzer output = CDSTestUtils.createArchiveAndCheck(opts);
+        output.shouldContain("CDS heap objects cannot be written because class jdk.internal.math.FDBigInteger maybe modified by ClassFileLoadHook")
+              .shouldContain("Skipping jdk/internal/math/FDBigInteger: Unsupported location")
+              .shouldMatch(".class.load.*jdk.internal.math.FDBigInteger.*source.*modules");
+
+        // run with archive
+        CDSOptions runOpts = (new CDSOptions())
+            .addPrefix("-cp", appJar, "-Xlog:class+load,cds=debug",
+                       "-XX:+AllowArchivingWithJavaAgent",
+                       useJavaAgent)
+            .setArchiveName(archiveName)
+            .setUseVersion(false)
+            .addSuffix(mainClass);
+        output = CDSTestUtils.runWithArchive(runOpts);
+        TestCommon.checkExecReturn(output, 0, true,
+            "Hello source: shared objects file");
+    }
+}

test/hotspot/jtreg/runtime/cds/appcds/test-classes/TransformBootClass.java

@@ -0,0 +1,62 @@
+/*
+ * 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.
+ *
+ */
+
+import java.lang.instrument.ClassFileTransformer;
+import java.lang.instrument.Instrumentation;
+import java.lang.instrument.IllegalClassFormatException;
+import java.security.ProtectionDomain;
+
+public class TransformBootClass implements ClassFileTransformer {
+
+    static String targetClass = null;
+
+    public byte[] transform(ClassLoader loader, String name, Class<?> classBeingRedefined,
+                            ProtectionDomain pd, byte[] buffer) throws IllegalClassFormatException {
+
+        if (name.equals(targetClass)) {
+            System.out.println("Transforming class " + name);
+            return buffer;
+        }
+        return null;
+    }
+
+    private static Instrumentation savedInstrumentation;
+
+    public static void premain(String agentArguments, Instrumentation instrumentation) {
+        System.out.println("TransformBootClass.premain() is called");
+        instrumentation.addTransformer(new TransformBootClass(), /*canRetransform=*/true);
+        savedInstrumentation = instrumentation;
+        if (agentArguments != null) {
+            targetClass = agentArguments;
+        }
+    }
+
+    public static Instrumentation getInstrumentation() {
+        return savedInstrumentation;
+    }
+
+    public static void agentmain(String args, Instrumentation inst) throws Exception {
+        premain(args, inst);
+    }
+}

test/hotspot/jtreg/runtime/cds/appcds/test-classes/TransformBootClass.mf

@@ -0,0 +1,5 @@
+Manifest-Version: 1.0
+Premain-Class: TransformBootClass
+Agent-Class: TransformBootClass
+Can-Retransform-Classes: true
+Can-Redefine-Classes: true