8351040: [REDO] Protection zone for easier detection of accidental zero-nKlass use

Reviewed-by: mbaesken, iklam

Author: tstuefe

src/hotspot/share/cds/archiveBuilder.cpp

@@ -153,14 +153,14 @@ void ArchiveBuilder::SourceObjList::relocate(int i, ArchiveBuilder* builder) {
 ArchiveBuilder::ArchiveBuilder() :
   _current_dump_region(nullptr),
   _buffer_bottom(nullptr),
-  _num_dump_regions_used(0),
   _requested_static_archive_bottom(nullptr),
   _requested_static_archive_top(nullptr),
   _requested_dynamic_archive_bottom(nullptr),
   _requested_dynamic_archive_top(nullptr),
   _mapped_static_archive_bottom(nullptr),
   _mapped_static_archive_top(nullptr),
   _buffer_to_requested_delta(0),
+  _pz_region("pz", MAX_SHARED_DELTA), // protection zone -- used only during dumping; does NOT exist in cds archive.
   _rw_region("rw", MAX_SHARED_DELTA),
   _ro_region("ro", MAX_SHARED_DELTA),
   _ptrmap(mtClassShared),
@@ -323,8 +323,12 @@ address ArchiveBuilder::reserve_buffer() {
   _shared_rs = rs;
 
   _buffer_bottom = buffer_bottom;
-  _current_dump_region = &_rw_region;
-  _num_dump_regions_used = 1;
+
+  if (CDSConfig::is_dumping_static_archive()) {
+    _current_dump_region = &_pz_region;
+  } else {
+    _current_dump_region = &_rw_region;
+  }
   _current_dump_region->init(&_shared_rs, &_shared_vs);
 
   ArchivePtrMarker::initialize(&_ptrmap, &_shared_vs);
@@ -366,7 +370,8 @@ address ArchiveBuilder::reserve_buffer() {
   if (CDSConfig::is_dumping_static_archive()) {
     // We don't want any valid object to be at the very bottom of the archive.
     // See ArchivePtrMarker::mark_pointer().
-    rw_region()->allocate(16);
+    _pz_region.allocate(MetaspaceShared::protection_zone_size());
+    start_dump_region(&_rw_region);
   }
 
   return buffer_bottom;
@@ -544,7 +549,6 @@ ArchiveBuilder::FollowMode ArchiveBuilder::get_follow_mode(MetaspaceClosure::Ref
 void ArchiveBuilder::start_dump_region(DumpRegion* next) {
   current_dump_region()->pack(next);
   _current_dump_region = next;
-  _num_dump_regions_used ++;
 }
 
 char* ArchiveBuilder::ro_strdup(const char* s) {

src/hotspot/share/cds/archiveBuilder.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2025, 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
@@ -96,7 +96,6 @@ class ArchiveBuilder : public StackObj {
 protected:
   DumpRegion* _current_dump_region;
   address _buffer_bottom;                      // for writing the contents of rw/ro regions
-  int _num_dump_regions_used;
 
   // These are the addresses where we will request the static and dynamic archives to be
   // mapped at run time. If the request fails (due to ASLR), we will map the archives at
@@ -210,6 +209,12 @@ class ArchiveBuilder : public StackObj {
   ReservedSpace _shared_rs;
   VirtualSpace _shared_vs;
 
+  // The "pz" region is used only during static dumps to reserve an unused space between SharedBaseAddress and
+  // the bottom of the rw region. During runtime, this space will be filled with a reserved area that disallows
+  // read/write/exec, so we can track for bad CompressedKlassPointers encoding.
+  // Note: this region does NOT exist in the cds archive.
+  DumpRegion _pz_region;
+
   DumpRegion _rw_region;
   DumpRegion _ro_region;
 
@@ -270,9 +275,6 @@ class ArchiveBuilder : public StackObj {
 
 protected:
   virtual void iterate_roots(MetaspaceClosure* it) = 0;
-
-  static const int _total_dump_regions = 2;
-
   void start_dump_region(DumpRegion* next);
 
 public:
@@ -367,6 +369,7 @@ class ArchiveBuilder : public StackObj {
   void remember_embedded_pointer_in_enclosing_obj(MetaspaceClosure::Ref* ref);
   static void serialize_dynamic_archivable_items(SerializeClosure* soc);
 
+  DumpRegion* pz_region() { return &_pz_region; }
   DumpRegion* rw_region() { return &_rw_region; }
   DumpRegion* ro_region() { return &_ro_region; }
 

src/hotspot/share/cds/archiveUtils.cpp

@@ -74,34 +74,39 @@ void ArchivePtrMarker::initialize(CHeapBitMap* ptrmap, VirtualSpace* vs) {
 }
 
 void ArchivePtrMarker::initialize_rw_ro_maps(CHeapBitMap* rw_ptrmap, CHeapBitMap* ro_ptrmap) {
-  address* rw_bottom = (address*)ArchiveBuilder::current()->rw_region()->base();
-  address* ro_bottom = (address*)ArchiveBuilder::current()->ro_region()->base();
+  address* buff_bottom = (address*)ArchiveBuilder::current()->buffer_bottom();
+  address* rw_bottom   = (address*)ArchiveBuilder::current()->rw_region()->base();
+  address* ro_bottom   = (address*)ArchiveBuilder::current()->ro_region()->base();
 
-  _rw_ptrmap = rw_ptrmap;
-  _ro_ptrmap = ro_ptrmap;
+  // The bit in _ptrmap that cover the very first word in the rw/ro regions.
+  size_t rw_start = rw_bottom - buff_bottom;
+  size_t ro_start = ro_bottom - buff_bottom;
 
+  // The number of bits used by the rw/ro ptrmaps. We might have lots of zero
+  // bits at the bottom and top of rw/ro ptrmaps, but these zeros will be
+  // removed by FileMapInfo::write_bitmap_region().
   size_t rw_size = ArchiveBuilder::current()->rw_region()->used() / sizeof(address);
   size_t ro_size = ArchiveBuilder::current()->ro_region()->used() / sizeof(address);
-  // ro_start is the first bit in _ptrmap that covers the pointer that would sit at ro_bottom.
-  // E.g., if rw_bottom = (address*)100
-  //          ro_bottom = (address*)116
-  //       then for 64-bit platform:
-  //          ro_start = ro_bottom - rw_bottom = (116 - 100) / sizeof(address) = 2;
-  size_t ro_start = ro_bottom - rw_bottom;
-
-  // Note: ptrmap is big enough only to cover the last pointer in ro_region.
-  // See ArchivePtrMarker::compact()
-  _rw_ptrmap->initialize(rw_size);
-  _ro_ptrmap->initialize(_ptrmap->size() - ro_start);
-
-  for (size_t rw_bit = 0; rw_bit < _rw_ptrmap->size(); rw_bit++) {
-    _rw_ptrmap->at_put(rw_bit, _ptrmap->at(rw_bit));
+
+  // The last (exclusive) bit in _ptrmap that covers the rw/ro regions.
+  // Note: _ptrmap is dynamically expanded only when an actual pointer is written, so
+  // it may not be as large as we want.
+  size_t rw_end = MIN2<size_t>(rw_start + rw_size, _ptrmap->size());
+  size_t ro_end = MIN2<size_t>(ro_start + ro_size, _ptrmap->size());
+
+  rw_ptrmap->initialize(rw_size);
+  ro_ptrmap->initialize(ro_size);
+
+  for (size_t rw_bit = rw_start; rw_bit < rw_end; rw_bit++) {
+    rw_ptrmap->at_put(rw_bit - rw_start, _ptrmap->at(rw_bit));
   }
 
-  for(size_t ro_bit = ro_start; ro_bit < _ptrmap->size(); ro_bit++) {
-    _ro_ptrmap->at_put(ro_bit-ro_start, _ptrmap->at(ro_bit));
+  for(size_t ro_bit = ro_start; ro_bit < ro_end; ro_bit++) {
+    ro_ptrmap->at_put(ro_bit - ro_start, _ptrmap->at(ro_bit));
   }
-  assert(_ptrmap->size() - ro_start == _ro_ptrmap->size(), "must be");
+
+  _rw_ptrmap = rw_ptrmap;
+  _ro_ptrmap = ro_ptrmap;
 }
 
 void ArchivePtrMarker::mark_pointer(address* ptr_loc) {

src/hotspot/share/cds/dynamicArchive.cpp

@@ -173,7 +173,6 @@ class DynamicArchiveBuilder : public ArchiveBuilder {
 
     post_dump();
 
-    assert(_num_dump_regions_used == _total_dump_regions, "must be");
     verify_universe("After CDS dynamic dump");
   }
 

src/hotspot/share/cds/filemap.hpp

@@ -399,7 +399,7 @@ class FileMapInfo : public CHeapObj<mtInternal> {
   // The offset of the (exclusive) end of the last core region in this archive, relative to SharedBaseAddress
   size_t mapping_end_offset()  const { return last_core_region()->mapping_end_offset(); }
 
-  char* mapped_base()    const { return first_core_region()->mapped_base(); }
+  char* mapped_base()    const { return header()->mapped_base_address();    }
   char* mapped_end()     const { return last_core_region()->mapped_end();   }
 
   // Non-zero if the archive needs to be mapped a non-default location due to ASLR.

src/hotspot/share/cds/metaspaceShared.cpp

@@ -149,6 +149,10 @@ size_t MetaspaceShared::core_region_alignment() {
   return os::cds_core_region_alignment();
 }
 
+size_t MetaspaceShared::protection_zone_size() {
+  return os::cds_core_region_alignment();
+}
+
 static bool shared_base_valid(char* shared_base) {
   // We check user input for SharedBaseAddress at dump time.
 
@@ -1253,6 +1257,7 @@ MapArchiveResult MetaspaceShared::map_archives(FileMapInfo* static_mapinfo, File
 
   ReservedSpace total_space_rs, archive_space_rs, class_space_rs;
   MapArchiveResult result = MAP_ARCHIVE_OTHER_FAILURE;
+  size_t prot_zone_size = 0;
   char* mapped_base_address = reserve_address_space_for_archives(static_mapinfo,
                                                                  dynamic_mapinfo,
                                                                  use_requested_addr,
@@ -1264,14 +1269,21 @@ MapArchiveResult MetaspaceShared::map_archives(FileMapInfo* static_mapinfo, File
     log_debug(cds)("Failed to reserve spaces (use_requested_addr=%u)", (unsigned)use_requested_addr);
   } else {
 
+    if (Metaspace::using_class_space()) {
+      prot_zone_size = protection_zone_size();
+    }
+
 #ifdef ASSERT
     // Some sanity checks after reserving address spaces for archives
     //  and class space.
     assert(archive_space_rs.is_reserved(), "Sanity");
     if (Metaspace::using_class_space()) {
+      assert(archive_space_rs.base() == mapped_base_address &&
+          archive_space_rs.size() > protection_zone_size(),
+          "Archive space must lead and include the protection zone");
       // Class space must closely follow the archive space. Both spaces
       //  must be aligned correctly.
-      assert(class_space_rs.is_reserved(),
+      assert(class_space_rs.is_reserved() && class_space_rs.size() > 0,
              "A class space should have been reserved");
       assert(class_space_rs.base() >= archive_space_rs.end(),
              "class space should follow the cds archive space");
@@ -1284,8 +1296,9 @@ MapArchiveResult MetaspaceShared::map_archives(FileMapInfo* static_mapinfo, File
     }
 #endif // ASSERT
 
-    log_info(cds)("Reserved archive_space_rs [" INTPTR_FORMAT " - " INTPTR_FORMAT "] (%zu) bytes",
-                   p2i(archive_space_rs.base()), p2i(archive_space_rs.end()), archive_space_rs.size());
+    log_info(cds)("Reserved archive_space_rs [" INTPTR_FORMAT " - " INTPTR_FORMAT "] (%zu) bytes%s",
+                   p2i(archive_space_rs.base()), p2i(archive_space_rs.end()), archive_space_rs.size(),
+                   (prot_zone_size > 0 ? " (includes protection zone)" : ""));
     log_info(cds)("Reserved class_space_rs   [" INTPTR_FORMAT " - " INTPTR_FORMAT "] (%zu) bytes",
                    p2i(class_space_rs.base()), p2i(class_space_rs.end()), class_space_rs.size());
 
@@ -1312,8 +1325,35 @@ MapArchiveResult MetaspaceShared::map_archives(FileMapInfo* static_mapinfo, File
         MemoryReserver::release(archive_space_rs);
         // Mark as not reserved
         archive_space_rs = {};
+        // The protection zone is part of the archive:
+        // See comment above, the Windows way of loading CDS is to mmap the individual
+        // parts of the archive into the address region we just vacated. The protection
+        // zone will not be mapped (and, in fact, does not exist as physical region in
+        // the archive). Therefore, after removing the archive space above, we must
+        // re-reserve the protection zone part lest something else gets mapped into that
+        // area later.
+        if (prot_zone_size > 0) {
+          assert(prot_zone_size >= os::vm_allocation_granularity(), "must be"); // not just page size!
+          char* p = os::attempt_reserve_memory_at(mapped_base_address, prot_zone_size,
+                                                  false, MemTag::mtClassShared);
+          assert(p == mapped_base_address || p == nullptr, "must be");
+          if (p == nullptr) {
+            log_debug(cds)("Failed to re-reserve protection zone");
+            return MAP_ARCHIVE_MMAP_FAILURE;
+          }
+        }
       }
     }
+
+    if (prot_zone_size > 0) {
+      os::commit_memory(mapped_base_address, prot_zone_size, false); // will later be protected
+      // Before mapping the core regions into the newly established address space, we mark
+      // start and the end of the future protection zone with canaries. That way we easily
+      // catch mapping errors (accidentally mapping data into the future protection zone).
+      *(mapped_base_address) = 'P';
+      *(mapped_base_address + prot_zone_size - 1) = 'P';
+    }
+
     MapArchiveResult static_result = map_archive(static_mapinfo, mapped_base_address, archive_space_rs);
     MapArchiveResult dynamic_result = (static_result == MAP_ARCHIVE_SUCCESS) ?
                                      map_archive(dynamic_mapinfo, mapped_base_address, archive_space_rs) : MAP_ARCHIVE_OTHER_FAILURE;
@@ -1357,38 +1397,40 @@ MapArchiveResult MetaspaceShared::map_archives(FileMapInfo* static_mapinfo, File
   if (result == MAP_ARCHIVE_SUCCESS) {
     SharedBaseAddress = (size_t)mapped_base_address;
 #ifdef _LP64
-        if (Metaspace::using_class_space()) {
-          // Set up ccs in metaspace.
-          Metaspace::initialize_class_space(class_space_rs);
-
-          // Set up compressed Klass pointer encoding: the encoding range must
-          //  cover both archive and class space.
-          address cds_base = (address)static_mapinfo->mapped_base();
-          address ccs_end = (address)class_space_rs.end();
-          assert(ccs_end > cds_base, "Sanity check");
-          if (INCLUDE_CDS_JAVA_HEAP || UseCompactObjectHeaders) {
-            // The CDS archive may contain narrow Klass IDs that were precomputed at archive generation time:
-            // - every archived java object header (only if INCLUDE_CDS_JAVA_HEAP)
-            // - every archived Klass' prototype   (only if +UseCompactObjectHeaders)
-            //
-            // In order for those IDs to still be valid, we need to dictate base and shift: base should be the
-            // mapping start, shift the shift used at archive generation time.
-            address precomputed_narrow_klass_base = cds_base;
-            const int precomputed_narrow_klass_shift = ArchiveBuilder::precomputed_narrow_klass_shift();
-            CompressedKlassPointers::initialize_for_given_encoding(
-              cds_base, ccs_end - cds_base, // Klass range
-              precomputed_narrow_klass_base, precomputed_narrow_klass_shift // precomputed encoding, see ArchiveBuilder
-            );
-          } else {
-            // Let JVM freely chose encoding base and shift
-            CompressedKlassPointers::initialize (
-              cds_base, ccs_end - cds_base // Klass range
-              );
-          }
-          // map_or_load_heap_region() compares the current narrow oop and klass encodings
-          // with the archived ones, so it must be done after all encodings are determined.
-          static_mapinfo->map_or_load_heap_region();
-        }
+    if (Metaspace::using_class_space()) {
+      assert(prot_zone_size > 0 &&
+             *(mapped_base_address) == 'P' &&
+             *(mapped_base_address + prot_zone_size - 1) == 'P',
+             "Protection zone was overwritten?");
+      // Set up ccs in metaspace.
+      Metaspace::initialize_class_space(class_space_rs);
+
+      // Set up compressed Klass pointer encoding: the encoding range must
+      //  cover both archive and class space.
+      const address encoding_base = (address)mapped_base_address;
+      const address klass_range_start = encoding_base + prot_zone_size;
+      const size_t klass_range_size = (address)class_space_rs.end() - klass_range_start;
+      if (INCLUDE_CDS_JAVA_HEAP || UseCompactObjectHeaders) {
+        // The CDS archive may contain narrow Klass IDs that were precomputed at archive generation time:
+        // - every archived java object header (only if INCLUDE_CDS_JAVA_HEAP)
+        // - every archived Klass' prototype   (only if +UseCompactObjectHeaders)
+        //
+        // In order for those IDs to still be valid, we need to dictate base and shift: base should be the
+        // mapping start (including protection zone), shift should be the shift used at archive generation time.
+        CompressedKlassPointers::initialize_for_given_encoding(
+          klass_range_start, klass_range_size,
+          encoding_base, ArchiveBuilder::precomputed_narrow_klass_shift() // precomputed encoding, see ArchiveBuilder
+        );
+      } else {
+        // Let JVM freely choose encoding base and shift
+        CompressedKlassPointers::initialize(klass_range_start, klass_range_size);
+      }
+      CompressedKlassPointers::establish_protection_zone(encoding_base, prot_zone_size);
+
+      // map_or_load_heap_region() compares the current narrow oop and klass encodings
+      // with the archived ones, so it must be done after all encodings are determined.
+      static_mapinfo->map_or_load_heap_region();
+    }
 #endif // _LP64
     log_info(cds)("initial optimized module handling: %s", CDSConfig::is_using_optimized_module_handling() ? "enabled" : "disabled");
     log_info(cds)("initial full module graph: %s", CDSConfig::is_using_full_module_graph() ? "enabled" : "disabled");
@@ -1470,7 +1512,6 @@ char* MetaspaceShared::reserve_address_space_for_archives(FileMapInfo* static_ma
   const size_t archive_space_alignment = core_region_alignment();
 
   // Size and requested location of the archive_space_rs (for both static and dynamic archives)
-  assert(static_mapinfo->mapping_base_offset() == 0, "Must be");
   size_t archive_end_offset  = (dynamic_mapinfo == nullptr) ? static_mapinfo->mapping_end_offset() : dynamic_mapinfo->mapping_end_offset();
   size_t archive_space_size = align_up(archive_end_offset, archive_space_alignment);
 

src/hotspot/share/cds/metaspaceShared.hpp

@@ -139,6 +139,7 @@ class MetaspaceShared : AllStatic {
   // Alignment for the 2 core CDS regions (RW/RO) only.
   // (Heap region alignments are decided by GC).
   static size_t core_region_alignment();
+  static size_t protection_zone_size();
   static void rewrite_nofast_bytecodes_and_calculate_fingerprints(Thread* thread, InstanceKlass* ik);
   // print loaded classes names to file.
   static void dump_loaded_classes(const char* file_name, TRAPS);

src/hotspot/share/include/cds.h

@@ -70,7 +70,10 @@ typedef struct CDSFileMapRegion {
   size_t  _ptrmap_offset;     // Bitmap for relocating native pointer fields in archived heap objects.
                               // (The base address is the bottom of the BM region).
   size_t  _ptrmap_size_in_bits;
-  char*   _mapped_base;       // Actually mapped address (null if this region is not mapped).
+  char*   _mapped_base;       // Actually mapped address used for mapping the core regions. At that address the
+                              // zero nklass protection zone is established; following that (at offset
+                              // MetaspaceShared::protection_zone_size()) the lowest core region (rw for the
+                              // static archive) is is mapped.
   bool    _in_reserved_space; // Is this region in a ReservedSpace
 } CDSFileMapRegion;