8264413: Data is written to file header even if its CRC32 was calculated

Reviewed-by: ccheung, minqi

Author: y1yang0

src/hotspot/share/memory/archiveBuilder.cpp

@@ -41,6 +41,7 @@
 #include "oops/instanceKlass.hpp"
 #include "oops/objArrayKlass.hpp"
 #include "oops/oopHandle.inline.hpp"
+#include "runtime/globals_extension.hpp"
 #include "runtime/sharedRuntime.hpp"
 #include "runtime/thread.hpp"
 #include "utilities/align.hpp"
@@ -1089,7 +1090,13 @@ void ArchiveBuilder::write_archive(FileMapInfo* mapinfo,
   print_region_stats(mapinfo, closed_heap_regions, open_heap_regions);
 
   mapinfo->set_requested_base((char*)MetaspaceShared::requested_base_address());
+  if (mapinfo->header()->magic() == CDS_DYNAMIC_ARCHIVE_MAGIC) {
+    mapinfo->set_header_base_archive_name_size(strlen(Arguments::GetSharedArchivePath()) + 1);
+    mapinfo->set_header_base_archive_is_default(FLAG_IS_DEFAULT(SharedArchiveFile));
+  }
   mapinfo->set_header_crc(mapinfo->compute_header_crc());
+  // After this point, we should not write any data into mapinfo->header() since this
+  // would corrupt its checksum we have calculated before.
   mapinfo->write_header();
   mapinfo->close();
 

src/hotspot/share/memory/filemap.cpp

@@ -53,7 +53,6 @@
 #include "oops/oop.inline.hpp"
 #include "prims/jvmtiExport.hpp"
 #include "runtime/arguments.hpp"
-#include "runtime/globals_extension.hpp"
 #include "runtime/java.hpp"
 #include "runtime/mutexLocker.hpp"
 #include "runtime/os.inline.hpp"
@@ -1236,17 +1235,14 @@ void FileMapInfo::open_for_write(const char* path) {
 void FileMapInfo::write_header() {
   _file_offset = 0;
   seek_to_position(_file_offset);
-  char* base_archive_name = NULL;
-  if (header()->magic() == CDS_DYNAMIC_ARCHIVE_MAGIC) {
-    base_archive_name = (char*)Arguments::GetSharedArchivePath();
-    header()->set_base_archive_name_size(strlen(base_archive_name) + 1);
-    header()->set_base_archive_is_default(FLAG_IS_DEFAULT(SharedArchiveFile));
-  }
-
   assert(is_file_position_aligned(), "must be");
   write_bytes(header(), header()->header_size());
-  if (base_archive_name != NULL) {
-    write_bytes(base_archive_name, header()->base_archive_name_size());
+
+  if (header()->magic() == CDS_DYNAMIC_ARCHIVE_MAGIC) {
+    char* base_archive_name = (char*)Arguments::GetSharedArchivePath();
+    if (base_archive_name != NULL) {
+      write_bytes(base_archive_name, header()->base_archive_name_size());
+    }
   }
 }
 

src/hotspot/share/memory/filemap.hpp

@@ -241,7 +241,7 @@ class FileMapHeader: private CDSFileMapHeaderBase {
   void set_as_offset(char* p, size_t *offset);
 public:
   // Accessors -- fields declared in CDSFileMapHeaderBase
-  unsigned int magic() const {return _magic;}
+  unsigned int magic()                    const { return _magic; }
   int crc()                               const { return _crc; }
   int version()                           const { return _version; }
 
@@ -383,13 +383,17 @@ class FileMapInfo : public CHeapObj<mtInternal> {
   void   invalidate();
   int    crc()                 const { return header()->crc(); }
   int    version()             const { return header()->version(); }
+  unsigned int magic()         const { return header()->magic(); }
   address narrow_oop_base()    const { return header()->narrow_oop_base(); }
   int     narrow_oop_shift()   const { return header()->narrow_oop_shift(); }
   uintx   max_heap_size()      const { return header()->max_heap_size(); }
   address narrow_klass_base()  const { return header()->narrow_klass_base(); }
   int     narrow_klass_shift() const { return header()->narrow_klass_shift(); }
   size_t  core_region_alignment() const { return header()->core_region_alignment(); }
 
+  void   set_header_base_archive_name_size(size_t size)      { header()->set_base_archive_name_size(size); }
+  void   set_header_base_archive_is_default(bool is_default) { header()->set_base_archive_is_default(is_default); }
+
   CompressedOops::Mode narrow_oop_mode()      const { return header()->narrow_oop_mode(); }
   jshort app_module_paths_start_index()       const { return header()->app_module_paths_start_index(); }
   jshort app_class_paths_start_index()        const { return header()->app_class_paths_start_index(); }

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/VerifyWithDynamicArchive.java

@@ -0,0 +1,72 @@
+/*
+ * Copyright (c) 2021, Alibaba Group Holding Limited. 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 8264413
+ * @summary test dynamic cds archive when turning on VerifySharedSpaces
+ * @requires vm.cds
+ * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds
+ * @compile ../test-classes/Hello.java
+ * @compile ../test-classes/HelloMore.java
+ * @build sun.hotspot.WhiteBox
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller sun.hotspot.WhiteBox
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xbootclasspath/a:. VerifyWithDynamicArchive
+ */
+
+public class VerifyWithDynamicArchive extends DynamicArchiveTestBase {
+
+    public static void main(String[] args) throws Exception {
+        runTest(VerifyWithDynamicArchive::testDefaultBase);
+    }
+
+    static void testDefaultBase() throws Exception {
+        String topArchiveName = getNewArchiveName("top");
+        doTest(topArchiveName);
+    }
+
+    private static void doTest(String topArchiveName) throws Exception {
+        String appJar = JarBuilder.getOrCreateHelloJar();
+
+        dump(topArchiveName,
+             "-Xlog:cds",
+             "-Xlog:cds+dynamic=debug",
+             "-cp", appJar, "Hello")
+            .assertNormalExit(output -> {
+                    output.shouldContain("Written dynamic archive 0x");
+                });
+
+        run(topArchiveName,
+            "-Xlog:class+load",
+            "-Xlog:cds+dynamic=debug,cds=debug",
+            "-XX:+VerifySharedSpaces",
+            "-cp", appJar,
+            "Hello")
+            .assertNormalExit(output -> {
+                    output.shouldContain("Hello source: shared objects file")
+                          .shouldNotContain("Header checksum verification failed")
+                          .shouldHaveExitValue(0);
+                });
+    }
+}