8292318: Memory corruption in remove_dumptime_info

Reviewed-by: coleenp, ccheung

Author: iklam

src/hotspot/share/classfile/systemDictionaryShared.cpp

@@ -80,7 +80,6 @@ DumpTimeSharedClassTable* SystemDictionaryShared::_dumptime_table = NULL;
 DumpTimeSharedClassTable* SystemDictionaryShared::_cloned_dumptime_table = NULL;
 DumpTimeLambdaProxyClassDictionary* SystemDictionaryShared::_dumptime_lambda_proxy_class_dictionary = NULL;
 DumpTimeLambdaProxyClassDictionary* SystemDictionaryShared::_cloned_dumptime_lambda_proxy_class_dictionary = NULL;
-SystemDictionaryShared::SavedCpCacheEntriesTable* SystemDictionaryShared::_saved_cpcache_entries_table = NULL;
 
 // Used by NoClassLoadingMark
 DEBUG_ONLY(bool SystemDictionaryShared::_class_loading_may_happen = true;)
@@ -505,7 +504,6 @@ void SystemDictionaryShared::initialize() {
     _dumptime_table = new (ResourceObj::C_HEAP, mtClass) DumpTimeSharedClassTable;
     _dumptime_lambda_proxy_class_dictionary =
                       new (ResourceObj::C_HEAP, mtClass) DumpTimeLambdaProxyClassDictionary;
-    _saved_cpcache_entries_table = new (ResourceObj::C_HEAP, mtClass) SavedCpCacheEntriesTable;
   }
 }
 
@@ -518,11 +516,6 @@ void SystemDictionaryShared::init_dumptime_info(InstanceKlass* k) {
 void SystemDictionaryShared::remove_dumptime_info(InstanceKlass* k) {
   MutexLocker ml(DumpTimeTable_lock, Mutex::_no_safepoint_check_flag);
   _dumptime_table->remove(k);
-
-  ConstantPoolCache* cpc = k->constants()->cache();
-  if (cpc != NULL) {
-    remove_saved_cpcache_entries_locked(cpc);
-  }
 }
 
 void SystemDictionaryShared::handle_class_unloading(InstanceKlass* klass) {
@@ -1336,36 +1329,6 @@ void SystemDictionaryShared::update_shared_entry(InstanceKlass* k, int id) {
   info->_id = id;
 }
 
-void SystemDictionaryShared::set_saved_cpcache_entries(ConstantPoolCache* cpc, ConstantPoolCacheEntry* entries) {
-  MutexLocker ml(DumpTimeTable_lock, Mutex::_no_safepoint_check_flag);
-  bool is_new = _saved_cpcache_entries_table->put(cpc, entries);
-  assert(is_new, "saved entries must never changed");
-}
-
-ConstantPoolCacheEntry* SystemDictionaryShared::get_saved_cpcache_entries_locked(ConstantPoolCache* cpc) {
-  assert_lock_strong(DumpTimeTable_lock);
-  ConstantPoolCacheEntry** p = _saved_cpcache_entries_table->get(cpc);
-  if (p != nullptr) {
-    return *p;
-  } else {
-    return nullptr;
-  }
-}
-
-void SystemDictionaryShared::remove_saved_cpcache_entries(ConstantPoolCache* cpc) {
-  MutexLocker ml(DumpTimeTable_lock, Mutex::_no_safepoint_check_flag);
-  remove_saved_cpcache_entries_locked(cpc);
-}
-
-void SystemDictionaryShared::remove_saved_cpcache_entries_locked(ConstantPoolCache* cpc) {
-  assert_lock_strong(DumpTimeTable_lock);
-  ConstantPoolCacheEntry** p = _saved_cpcache_entries_table->get(cpc);
-  if (p != nullptr) {
-    _saved_cpcache_entries_table->remove(cpc);
-    FREE_C_HEAP_ARRAY(ConstantPoolCacheEntry, *p);
-  }
-}
-
 const char* class_loader_name_for_shared(Klass* k) {
   assert(k != nullptr, "Sanity");
   assert(k->is_shared(), "Must be");

src/hotspot/share/classfile/systemDictionaryShared.hpp

@@ -168,15 +168,6 @@ class SystemDictionaryShared: public SystemDictionary {
   static DumpTimeLambdaProxyClassDictionary* _dumptime_lambda_proxy_class_dictionary;
   static DumpTimeLambdaProxyClassDictionary* _cloned_dumptime_lambda_proxy_class_dictionary;
 
-  // Doesn't need to be cloned as it's not modified during dump time.
-  using SavedCpCacheEntriesTable = ResourceHashtable<
-    ConstantPoolCache*,
-    ConstantPoolCacheEntry*,
-    15889, // prime number
-    ResourceObj::C_HEAP,
-    mtClassShared>;
-  static SavedCpCacheEntriesTable* _saved_cpcache_entries_table;
-
   static ArchiveInfo _static_archive;
   static ArchiveInfo _dynamic_archive;
 
@@ -248,11 +239,6 @@ class SystemDictionaryShared: public SystemDictionary {
     return ClassLoaderData::the_null_class_loader_data()->dictionary();
   }
 
-  static void set_saved_cpcache_entries(ConstantPoolCache* cpc, ConstantPoolCacheEntry* entries);
-  static ConstantPoolCacheEntry* get_saved_cpcache_entries_locked(ConstantPoolCache* k);
-  static void remove_saved_cpcache_entries(ConstantPoolCache* cpc);
-  static void remove_saved_cpcache_entries_locked(ConstantPoolCache* cpc);
-
   static void update_shared_entry(InstanceKlass* klass, int id);
   static void set_shared_class_misc_info(InstanceKlass* k, ClassFileStream* cfs);
 

src/hotspot/share/interpreter/rewriter.cpp

@@ -112,21 +112,21 @@ void Rewriter::make_constant_pool_cache(TRAPS) {
                                         _resolved_reference_limit,
                                         THREAD);
 
+  if (!HAS_PENDING_EXCEPTION && Arguments::is_dumping_archive()) {
+    if (_pool->pool_holder()->is_shared()) {
+      assert(DynamicDumpSharedSpaces, "must be");
+      // We are linking a shared class from the base archive. This
+      // class won't be written into the dynamic archive, so there's no
+      // need to save its CpCaches.
+    } else {
+      cache->save_for_archive(THREAD);
+    }
+  }
+
   // Clean up constant pool cache if initialize_resolved_references() failed.
   if (HAS_PENDING_EXCEPTION) {
     MetadataFactory::free_metadata(loader_data, cache);
     _pool->set_cache(NULL);  // so the verifier isn't confused
-  } else {
-    if (Arguments::is_dumping_archive()) {
-      if (_pool->pool_holder()->is_shared()) {
-        assert(DynamicDumpSharedSpaces, "must be");
-        // We are linking a shared class from the base archive. This
-        // class won't be written into the dynamic archive, so there's no
-        // need to save its CpCaches.
-      } else {
-        cache->save_for_archive();
-      }
-    }
   }
 }
 

src/hotspot/share/oops/cpCache.cpp

@@ -23,6 +23,7 @@
  */
 
 #include "precompiled.hpp"
+#include "cds/archiveBuilder.hpp"
 #include "cds/heapShared.hpp"
 #include "classfile/resolutionErrors.hpp"
 #include "classfile/systemDictionary.hpp"
@@ -683,30 +684,30 @@ void ConstantPoolCache::record_gc_epoch() {
   _gc_epoch = Continuations::gc_epoch();
 }
 
-void ConstantPoolCache::save_for_archive() {
+void ConstantPoolCache::save_for_archive(TRAPS) {
 #if INCLUDE_CDS
-  ConstantPoolCacheEntry* copy = NEW_C_HEAP_ARRAY(ConstantPoolCacheEntry, length(), mtClassShared);
+  ClassLoaderData* loader_data = constant_pool()->pool_holder()->class_loader_data();
+  _initial_entries = MetadataFactory::new_array<ConstantPoolCacheEntry>(loader_data, length(), CHECK);
   for (int i = 0; i < length(); i++) {
-    copy[i] = *entry_at(i);
+    _initial_entries->at_put(i, *entry_at(i));
   }
-
-  SystemDictionaryShared::set_saved_cpcache_entries(this, copy);
 #endif
 }
 
 void ConstantPoolCache::remove_unshareable_info() {
 #if INCLUDE_CDS
   Arguments::assert_is_dumping_archive();
-  // <this> is the copy to be written into the archive. It's in
-  // the ArchiveBuilder's "buffer space". However, the saved_cpcache_entries
-  // are recorded with the original ConstantPoolCache object.
-  ConstantPoolCache* orig_cpc = ArchiveBuilder::current()->get_src_obj(this);
-  ConstantPoolCacheEntry* saved = SystemDictionaryShared::get_saved_cpcache_entries_locked(orig_cpc);
+  // <this> is the copy to be written into the archive. It's in the ArchiveBuilder's "buffer space".
+  // However, this->_initial_entries was not copied/relocated by the ArchiveBuilder, so it's
+  // still pointing to the array allocated inside save_for_archive().
+  assert(_initial_entries != NULL, "archived cpcache must have been initialized");
+  assert(!ArchiveBuilder::current()->is_in_buffer_space(_initial_entries), "must be");
   for (int i=0; i<length(); i++) {
     // Restore each entry to the initial state -- just after Rewriter::make_constant_pool_cache()
     // has finished.
-    *entry_at(i) = saved[i];
+    *entry_at(i) = _initial_entries->at(i);
   }
+  _initial_entries = NULL;
 #endif
 }
 
@@ -716,10 +717,11 @@ void ConstantPoolCache::deallocate_contents(ClassLoaderData* data) {
   set_resolved_references(OopHandle());
   MetadataFactory::free_array<u2>(data, _reference_map);
   set_reference_map(NULL);
-
 #if INCLUDE_CDS
-  if (Arguments::is_dumping_archive()) {
-    SystemDictionaryShared::remove_saved_cpcache_entries(this);
+  if (_initial_entries != NULL) {
+    Arguments::assert_is_dumping_archive();
+    MetadataFactory::free_array<ConstantPoolCacheEntry>(data, _initial_entries);
+    _initial_entries = NULL;
   }
 #endif
 }

src/hotspot/share/oops/cpCache.hpp

@@ -401,6 +401,11 @@ class ConstantPoolCache: public MetaspaceObj {
   // If you add a new field that points to any metaspace object, you
   // must add this field to ConstantPoolCache::metaspace_pointers_do().
   int             _length;
+
+  // The narrowOop pointer to the archived resolved_references. Set at CDS dump
+  // time when caching java heap object is supported.
+  CDS_JAVA_HEAP_ONLY(int _archived_references_index;) // Gap on LP64
+
   ConstantPool*   _constant_pool;          // the corresponding constant pool
 
   // The following fields need to be modified at runtime, so they cannot be
@@ -413,9 +418,7 @@ class ConstantPoolCache: public MetaspaceObj {
   // RedefineClasses support
   uint64_t             _gc_epoch;
 
-  // The narrowOop pointer to the archived resolved_references. Set at CDS dump
-  // time when caching java heap object is supported.
-  CDS_JAVA_HEAP_ONLY(int _archived_references_index;)
+  CDS_ONLY(Array<ConstantPoolCacheEntry>* _initial_entries;)
 
   // Sizing
   debug_only(friend class ClassVerifier;)
@@ -454,7 +457,7 @@ class ConstantPoolCache: public MetaspaceObj {
 
   // CDS support
   void remove_unshareable_info();
-  void save_for_archive();
+  void save_for_archive(TRAPS);
  private:
   void walk_entries_for_initialization(bool check_only);
   void set_length(int length)                    { _length = length; }

src/hotspot/share/oops/instanceKlass.cpp

@@ -576,8 +576,6 @@ void InstanceKlass::deallocate_record_components(ClassLoaderData* loader_data,
 // This function deallocates the metadata and C heap pointers that the
 // InstanceKlass points to.
 void InstanceKlass::deallocate_contents(ClassLoaderData* loader_data) {
-  SystemDictionaryShared::handle_class_unloading(this);
-
   // Orphan the mirror first, CMS thinks it's still live.
   if (java_mirror() != NULL) {
     java_lang_Class::set_klass(java_mirror(), NULL);
@@ -692,6 +690,8 @@ void InstanceKlass::deallocate_contents(ClassLoaderData* loader_data) {
     MetadataFactory::free_metadata(loader_data, annotations());
   }
   set_annotations(NULL);
+
+  SystemDictionaryShared::handle_class_unloading(this);
 }
 
 bool InstanceKlass::is_record() const {

test/hotspot/jtreg/runtime/cds/MaxMetaspaceSize.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2022, 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
@@ -51,8 +51,8 @@ public static void main(String[] args) throws Exception {
       processArgs.add("-XX:MaxMetaspaceSize=1m");
     }
 
-    String msg = "OutOfMemoryError: Metaspace";
+    String msg = "OutOfMemoryError: ((Metaspace)|(Compressed class space))";
     ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(processArgs);
-    CDSTestUtils.executeAndLog(pb, "dump").shouldContain(msg).shouldHaveExitValue(1);
+    CDSTestUtils.executeAndLog(pb, "dump").shouldMatch(msg).shouldHaveExitValue(1);
   }
 }