8226406: JVM fails to detect mismatched or corrupt CDS archive

Check important archive header fields such as _jvm_ident before processing other fields.

Reviewed-by: iklam, jiangli

src/hotspot/share/memory/filemap.cpp

@@ -155,6 +155,8 @@ template <int N> static void get_header_version(char (&header_version) [N]) {
   const char *vm_version = VM_Version::internal_vm_info_string();
   const int version_len = (int)strlen(vm_version);
 
+  memset(header_version, 0, JVM_IDENT_MAX);
+
   if (version_len < (JVM_IDENT_MAX-1)) {
     strcpy(header_version, vm_version);
 
@@ -170,6 +172,8 @@ template <int N> static void get_header_version(char (&header_version) [N]) {
     sprintf(&header_version[JVM_IDENT_MAX-9], "%08x", hash);
     header_version[JVM_IDENT_MAX-1] = 0;  // Null terminate.
   }
+
+  assert(header_version[JVM_IDENT_MAX-1] == 0, "must be");
 }
 
 FileMapInfo::FileMapInfo(bool is_static) {
@@ -730,10 +734,59 @@ bool FileMapInfo::init_from_file(int fd, bool is_static) {
     fail_continue("Unable to read the file header.");
     return false;
   }
+
+  if (!Arguments::has_jimage()) {
+    FileMapInfo::fail_continue("The shared archive file cannot be used with an exploded module build.");
+    return false;
+  }
+
+  unsigned int expected_magic = is_static ? CDS_ARCHIVE_MAGIC : CDS_DYNAMIC_ARCHIVE_MAGIC;
+  if (_header->_magic != expected_magic) {
+    log_info(cds)("_magic expected: 0x%08x", expected_magic);
+    log_info(cds)("         actual: 0x%08x", _header->_magic);
+    FileMapInfo::fail_continue("The shared archive file has a bad magic number.");
+    return false;
+  }
+
   if (_header->_version != CURRENT_CDS_ARCHIVE_VERSION) {
+    log_info(cds)("_version expected: %d", CURRENT_CDS_ARCHIVE_VERSION);
+    log_info(cds)("           actual: %d", _header->_version);
     fail_continue("The shared archive file has the wrong version.");
     return false;
   }
+
+  if (_header->_header_size != sz) {
+    log_info(cds)("_header_size expected: " SIZE_FORMAT, sz);
+    log_info(cds)("               actual: " SIZE_FORMAT, _header->_header_size);
+    FileMapInfo::fail_continue("The shared archive file has an incorrect header size.");
+    return false;
+  }
+
+  if (_header->_jvm_ident[JVM_IDENT_MAX-1] != 0) {
+    FileMapInfo::fail_continue("JVM version identifier is corrupted.");
+    return false;
+  }
+
+  char header_version[JVM_IDENT_MAX];
+  get_header_version(header_version);
+  if (strncmp(_header->_jvm_ident, header_version, JVM_IDENT_MAX-1) != 0) {
+    log_info(cds)("_jvm_ident expected: %s", header_version);
+    log_info(cds)("             actual: %s", _header->_jvm_ident);
+    FileMapInfo::fail_continue("The shared archive file was created by a different"
+                  " version or build of HotSpot");
+    return false;
+  }
+
+  if (VerifySharedSpaces) {
+    int expected_crc = _header->compute_crc();
+    if (expected_crc != _header->_crc) {
+      log_info(cds)("_crc expected: %d", expected_crc);
+      log_info(cds)("       actual: %d", _header->_crc);
+      FileMapInfo::fail_continue("Header checksum verification failed.");
+      return false;
+    }
+  }
+
   _file_offset = n;
 
   size_t info_size = _header->_paths_misc_info_size;
@@ -748,10 +801,6 @@ bool FileMapInfo::init_from_file(int fd, bool is_static) {
   _file_offset += n + _header->_base_archive_name_size; // accounts for the size of _base_archive_name
 
   if (is_static) {
-    if (_header->_magic != CDS_ARCHIVE_MAGIC) {
-      fail_continue("Incorrect static archive magic number");
-      return false;
-    }
     // just checking the last region is sufficient since the archive is written
     // in sequential order
     size_t len = lseek(fd, 0, SEEK_END);
@@ -1589,33 +1638,7 @@ int FileMapHeader::compute_crc() {
 
 // This function should only be called during run time with UseSharedSpaces enabled.
 bool FileMapHeader::validate() {
-  if (VerifySharedSpaces && compute_crc() != _crc) {
-    FileMapInfo::fail_continue("Header checksum verification failed.");
-    return false;
-  }
 
-  if (!Arguments::has_jimage()) {
-    FileMapInfo::fail_continue("The shared archive file cannot be used with an exploded module build.");
-    return false;
-  }
-
-  if (_version != CURRENT_CDS_ARCHIVE_VERSION) {
-    FileMapInfo::fail_continue("The shared archive file is the wrong version.");
-    return false;
-  }
-  if (_magic != CDS_ARCHIVE_MAGIC && _magic != CDS_DYNAMIC_ARCHIVE_MAGIC) {
-    FileMapInfo::fail_continue("The shared archive file has a bad magic number.");
-    return false;
-  }
-  char header_version[JVM_IDENT_MAX];
-  get_header_version(header_version);
-  if (strncmp(_jvm_ident, header_version, JVM_IDENT_MAX-1) != 0) {
-    log_info(class, path)("expected: %s", header_version);
-    log_info(class, path)("actual:   %s", _jvm_ident);
-    FileMapInfo::fail_continue("The shared archive file was created by a different"
-                  " version or build of HotSpot");
-    return false;
-  }
   if (_obj_alignment != ObjectAlignmentInBytes) {
     FileMapInfo::fail_continue("The shared archive file's ObjectAlignmentInBytes of %d"
                   " does not equal the current ObjectAlignmentInBytes of " INTX_FORMAT ".",

src/hotspot/share/prims/cdsoffsets.cpp

@@ -49,6 +49,7 @@ CDSOffsets* CDSOffsets::_all = NULL;
     ADD_NEXT(_all, "FileMapHeader::_magic", offset_of(FileMapHeader, _magic));              \
     ADD_NEXT(_all, "FileMapHeader::_crc", offset_of(FileMapHeader, _crc));                  \
     ADD_NEXT(_all, "FileMapHeader::_version", offset_of(FileMapHeader, _version));          \
+    ADD_NEXT(_all, "FileMapHeader::_jvm_ident", offset_of(FileMapHeader, _jvm_ident));      \
     ADD_NEXT(_all, "FileMapHeader::_space[0]", offset_of(FileMapHeader, _space));           \
     ADD_NEXT(_all, "CDSFileMapRegion::_crc", offset_of(CDSFileMapRegion, _crc));            \
     ADD_NEXT(_all, "CDSFileMapRegion::_used", offset_of(CDSFileMapRegion, _used));          \

test/hotspot/jtreg/runtime/appcds/SharedArchiveConsistency.java

@@ -61,13 +61,17 @@ import sun.hotspot.WhiteBox;
 
 public class SharedArchiveConsistency {
     public static WhiteBox wb;
-    public static int offset_magic;    // FileMapHeader::_magic
+    public static int offset_magic;     // CDSFileMapHeaderBase::_magic
-    public static int sp_offset_crc;   // CDSFileMapRegion::_crc
+    public static int offset_version;   // CDSFileMapHeaderBase::_version
+    public static int offset_jvm_ident; // FileMapHeader::_jvm_ident
+    public static int sp_offset_crc;    // CDSFileMapRegion::_crc
+    public static int offset_paths_misc_info_size;
     public static int file_header_size = -1;// total size of header, variant, need calculation
     public static int CDSFileMapRegion_size; // size of CDSFileMapRegion
     public static int sp_offset;       // offset of CDSFileMapRegion
     public static int sp_used_offset;  // offset of CDSFileMapRegion::_used
     public static int size_t_size;     // size of size_t
+    public static int int_size;        // size of int
 
     public static File jsa;        // will be updated during test
     public static File orgJsaFile; // kept the original file not touched.
@@ -94,6 +98,8 @@ public class SharedArchiveConsistency {
     public static void getFileOffsetInfo() throws Exception {
         wb = WhiteBox.getWhiteBox();
         offset_magic = wb.getOffsetForName("FileMapHeader::_magic");
+        offset_version = wb.getOffsetForName("FileMapHeader::_version");
+        offset_jvm_ident = wb.getOffsetForName("FileMapHeader::_jvm_ident");
         sp_offset_crc = wb.getOffsetForName("CDSFileMapRegion::_crc");
         try {
             int nonExistOffset = wb.getOffsetForName("FileMapHeader::_non_exist_offset");
@@ -113,13 +119,13 @@ public class SharedArchiveConsistency {
             return file_header_size;
         }
         // this is not real header size, it is struct size
-        int int_size = wb.getOffsetForName("int_size");
+        int_size = wb.getOffsetForName("int_size");
         file_header_size = wb.getOffsetForName("file_header_size");
-        int offset_path_misc_info = wb.getOffsetForName("FileMapHeader::_paths_misc_info_size") -
+        offset_paths_misc_info_size = wb.getOffsetForName("FileMapHeader::_paths_misc_info_size") -
             offset_magic;
-        int path_misc_info_size   = (int)readInt(fc, offset_path_misc_info, int_size);
+        int path_misc_info_size   = (int)readInt(fc, offset_paths_misc_info_size, int_size);
-        file_header_size += path_misc_info_size; //readInt(fc, offset_path_misc_info, size_t_size);
+        file_header_size += path_misc_info_size;
-        System.out.println("offset_path_misc_info = " + offset_path_misc_info);
+        System.out.println("offset_paths_misc_info_size = " + offset_paths_misc_info_size);
         System.out.println("path_misc_info_size   = " + path_misc_info_size);
         System.out.println("file_header_size      = " + file_header_size);
         file_header_size = (int)align_up_page(file_header_size);
@@ -160,15 +166,15 @@ public class SharedArchiveConsistency {
         fc.force(true);
     }
 
-    public static FileChannel getFileChannel() throws Exception {
+    public static FileChannel getFileChannel(File jsaFile) throws Exception {
         List<StandardOpenOption> arry = new ArrayList<StandardOpenOption>();
         arry.add(READ);
         arry.add(WRITE);
-        return FileChannel.open(jsa.toPath(), new HashSet<StandardOpenOption>(arry));
+        return FileChannel.open(jsaFile.toPath(), new HashSet<StandardOpenOption>(arry));
     }
 
-    public static void modifyJsaContentRandomly() throws Exception {
+    public static void modifyJsaContentRandomly(File jsaFile) throws Exception {
-        FileChannel fc = getFileChannel();
+        FileChannel fc = getFileChannel(jsaFile);
         // corrupt random area in the data areas
         long[] used    = new long[num_regions];       // record used bytes
         long start0, start, end, off;
@@ -210,8 +216,8 @@ public class SharedArchiveConsistency {
         return used;
     }
 
-    public static boolean modifyJsaContent(int region) throws Exception {
+    public static boolean modifyJsaContent(int region, File jsaFile) throws Exception {
-        FileChannel fc = getFileChannel();
+        FileChannel fc = getFileChannel(jsaFile);
         byte[] buf = new byte[4096];
         ByteBuffer bbuf = ByteBuffer.wrap(buf);
 
@@ -248,8 +254,8 @@ public class SharedArchiveConsistency {
         return true;
     }
 
-    public static void modifyJsaHeader() throws Exception {
+    public static void modifyJsaHeader(File jsaFile) throws Exception {
-        FileChannel fc = getFileChannel();
+        FileChannel fc = getFileChannel(jsaFile);
         // screw up header info
         byte[] buf = new byte[getFileHeaderSize(fc)];
         ByteBuffer bbuf = ByteBuffer.wrap(buf);
@@ -259,6 +265,30 @@ public class SharedArchiveConsistency {
         }
     }
 
+    public static void modifyJvmIdent() throws Exception {
+        FileChannel fc = getFileChannel(jsa);
+        int headerSize = getFileHeaderSize(fc);
+        System.out.println("    offset_jvm_ident " + offset_jvm_ident);
+        byte[] buf = new byte[256];
+        ByteBuffer bbuf = ByteBuffer.wrap(buf);
+        writeData(fc, (long)offset_jvm_ident, bbuf);
+        if (fc.isOpen()) {
+            fc.close();
+        }
+    }
+
+    public static void modifyHeaderIntField(long offset, int value) throws Exception {
+        FileChannel fc = getFileChannel(jsa);
+        int headerSize = getFileHeaderSize(fc);
+        System.out.println("    offset " + offset);
+        byte[] buf = ByteBuffer.allocate(4).putInt(value).array();
+        ByteBuffer bbuf = ByteBuffer.wrap(buf);
+        writeData(fc, offset, bbuf);
+        if (fc.isOpen()) {
+            fc.close();
+        }
+    }
+
     public static void copyFile(File from, File to) throws Exception {
         if (to.exists()) {
             if(!to.delete()) {
@@ -348,10 +378,10 @@ public class SharedArchiveConsistency {
         // test, should pass
         System.out.println("1. Normal, should pass but may fail\n");
 
-        String[] execArgs = {"-cp", jarFile, "Hello"};
+        String[] execArgs = {"-Xlog:cds", "-cp", jarFile, "Hello"};
         // tests that corrupt contents of the archive need to run with
         // VerifySharedSpaces enabled to detect inconsistencies
-        String[] verifyExecArgs = {"-XX:+VerifySharedSpaces", "-cp", jarFile, "Hello"};
+        String[] verifyExecArgs = {"-Xlog:cds", "-XX:+VerifySharedSpaces", "-cp", jarFile, "Hello"};
 
         OutputAnalyzer output = TestCommon.execCommon(execArgs);
 
@@ -373,10 +403,36 @@ public class SharedArchiveConsistency {
         orgJsaFile = new File(new File(currentDir), "appcds.jsa.bak");
         copyFile(jsa, orgJsaFile);
 
-
         // modify jsa header, test should fail
         System.out.println("\n2. Corrupt header, should fail\n");
-        modifyJsaHeader();
+        modifyJsaHeader(jsa);
+        output = TestCommon.execCommon(execArgs);
+        output.shouldContain("The shared archive file has a bad magic number");
+        output.shouldNotContain("Checksum verification failed");
+
+        copyFile(orgJsaFile, jsa);
+        // modify _jvm_ident and _paths_misc_info_size, test should fail
+        System.out.println("\n2a. Corrupt _jvm_ident and _paths_misc_info_size, should fail\n");
+        modifyJvmIdent();
+        modifyHeaderIntField(offset_paths_misc_info_size, Integer.MAX_VALUE);
+        output = TestCommon.execCommon(execArgs);
+        output.shouldContain("The shared archive file was created by a different version or build of HotSpot");
+        output.shouldNotContain("Checksum verification failed");
+
+        copyFile(orgJsaFile, jsa);
+        // modify _magic and _paths_misc_info_size, test should fail
+        System.out.println("\n2b. Corrupt _magic and _paths_misc_info_size, should fail\n");
+        modifyHeaderIntField(offset_magic, 0x00000000);
+        modifyHeaderIntField(offset_paths_misc_info_size, Integer.MAX_VALUE);
+        output = TestCommon.execCommon(execArgs);
+        output.shouldContain("The shared archive file has a bad magic number");
+        output.shouldNotContain("Checksum verification failed");
+
+        copyFile(orgJsaFile, jsa);
+        // modify _version and _paths_misc_info_size, test should fail
+        System.out.println("\n2c. Corrupt _version and _paths_misc_info_size, should fail\n");
+        modifyHeaderIntField(offset_version, 0x00000000);
+        modifyHeaderIntField(offset_paths_misc_info_size, Integer.MAX_VALUE);
         output = TestCommon.execCommon(execArgs);
         output.shouldContain("The shared archive file has the wrong version");
         output.shouldNotContain("Checksum verification failed");
@@ -387,8 +443,9 @@ public class SharedArchiveConsistency {
         for (int i=0; i<num_regions; i++) {
             newJsaFile = new File(TestCommon.getNewArchiveName(shared_region_name[i]));
             copyFile(orgJsaFile, newJsaFile);
-            if (modifyJsaContent(i)) {
+            TestCommon.setCurrentArchiveName(newJsaFile.toString());
-                testAndCheck(execArgs);
+            if (modifyJsaContent(i, newJsaFile)) {
+                testAndCheck(verifyExecArgs);
             }
         }
 
@@ -396,15 +453,17 @@ public class SharedArchiveConsistency {
         System.out.println("\n4. Corrupt Header and Content, should fail\n");
         newJsaFile = new File(TestCommon.getNewArchiveName("header-and-content"));
         copyFile(orgJsaFile, newJsaFile);
-        modifyJsaHeader();
+        TestCommon.setCurrentArchiveName(newJsaFile.toString());
-        modifyJsaContent(0);  // this will not be reached since failed on header change first
+        modifyJsaHeader(newJsaFile);
+        modifyJsaContent(0, newJsaFile);  // this will not be reached since failed on header change first
         output = TestCommon.execCommon(execArgs);
-        output.shouldContain("The shared archive file has the wrong version");
+        output.shouldContain("The shared archive file has a bad magic number");
         output.shouldNotContain("Checksum verification failed");
 
         // delete bytes in data section
         System.out.println("\n5. Delete bytes at beginning of data section, should fail\n");
         copyFile(orgJsaFile, jsa, true);
+        TestCommon.setCurrentArchiveName(jsa.toString());
         testAndCheck(verifyExecArgs);
 
         // insert bytes in data section forward
@@ -415,7 +474,8 @@ public class SharedArchiveConsistency {
         System.out.println("\n7. modify Content in random areas, should fail\n");
         newJsaFile = new File(TestCommon.getNewArchiveName("random-areas"));
         copyFile(orgJsaFile, newJsaFile);
-        modifyJsaContentRandomly();
+        TestCommon.setCurrentArchiveName(newJsaFile.toString());
+        modifyJsaContentRandomly(newJsaFile);
         testAndCheck(verifyExecArgs);
     }
 }

test/hotspot/jtreg/runtime/appcds/TestCommon.java

@@ -90,6 +90,10 @@ public class TestCommon extends CDSTestUtils {
         return currentArchiveName;
     }
 
+    public static void setCurrentArchiveName(String archiveName) {
+        currentArchiveName = archiveName;
+    }
+
     public static String getNewArchiveName() {
         return getNewArchiveName(null);
     }