From c0542c42a58ace180314cf28017bd5a936bd4de4 Mon Sep 17 00:00:00 2001 From: "zhengyu.gu" Date: Tue, 14 Jan 2025 22:02:35 -0500 Subject: [PATCH 1/7] 8347758: modules.cpp leaks string returned from get_numbered_property_as_sorted_string() --- src/hotspot/share/classfile/modules.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/hotspot/share/classfile/modules.cpp b/src/hotspot/share/classfile/modules.cpp index 7e26febda89f2..c5dc65679ef0f 100644 --- a/src/hotspot/share/classfile/modules.cpp +++ b/src/hotspot/share/classfile/modules.cpp @@ -1,5 +1,5 @@ /* -* Copyright (c) 2016, 2024, Oracle and/or its affiliates. All rights reserved. +* Copyright (c) 2016, 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 @@ -631,6 +631,7 @@ void Modules::dump_native_access_flag() { const char* native_access_names = get_native_access_flags_as_sorted_string(); if (native_access_names != nullptr) { _archived_native_access_flags = ArchiveBuilder::current()->ro_strdup(native_access_names); + os::free((void*)native_access_names); } } @@ -641,10 +642,12 @@ const char* Modules::get_native_access_flags_as_sorted_string() { void Modules::serialize_native_access_flags(SerializeClosure* soc) { soc->do_ptr(&_archived_native_access_flags); if (soc->reading()) { - check_archived_flag_consistency(_archived_native_access_flags, get_native_access_flags_as_sorted_string(), "jdk.module.enable.native.access"); + const char* native_access_names = get_native_access_flags_as_sorted_string(); + check_archived_flag_consistency(_archived_native_access_flags, native_access_names, "jdk.module.enable.native.access"); // Don't hold onto the pointer, in case we might decide to unmap the archive. _archived_native_access_flags = nullptr; + os::free((void*)native_access_names); } } @@ -652,6 +655,7 @@ void Modules::dump_addmods_names() { const char* addmods_names = get_addmods_names_as_sorted_string(); if (addmods_names != nullptr) { _archived_addmods_names = ArchiveBuilder::current()->ro_strdup(addmods_names); + os::free((void*)addmods_names); } } @@ -662,10 +666,12 @@ const char* Modules::get_addmods_names_as_sorted_string() { void Modules::serialize_addmods_names(SerializeClosure* soc) { soc->do_ptr(&_archived_addmods_names); if (soc->reading()) { - check_archived_flag_consistency(_archived_addmods_names, get_addmods_names_as_sorted_string(), "jdk.module.addmods"); + const char* addmods_names = get_addmods_names_as_sorted_string(); + check_archived_flag_consistency(_archived_addmods_names, addmods_names, "jdk.module.addmods"); // Don't hold onto the pointer, in case we might decide to unmap the archive. _archived_addmods_names = nullptr; + os::free((void*)addmods_names); } } From e4ab2bb1f1993e04980476b04ce6021ef1882c03 Mon Sep 17 00:00:00 2001 From: "zhengyu.gu" Date: Wed, 15 Jan 2025 09:45:17 -0500 Subject: [PATCH 2/7] Avoid copying strings --- src/hotspot/share/classfile/modules.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/hotspot/share/classfile/modules.cpp b/src/hotspot/share/classfile/modules.cpp index c5dc65679ef0f..e2bc8f797dce8 100644 --- a/src/hotspot/share/classfile/modules.cpp +++ b/src/hotspot/share/classfile/modules.cpp @@ -572,6 +572,7 @@ void Modules::dump_main_module_name() { } void Modules::check_archived_flag_consistency(char* archived_flag, const char* runtime_flag, const char* property) { + assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); log_info(cds)("%s %s", property, archived_flag != nullptr ? archived_flag : "(null)"); bool disable = false; @@ -614,6 +615,7 @@ void Modules::serialize_archived_module_info(SerializeClosure* soc) { } void Modules::serialize(SerializeClosure* soc) { + ResourceMark rm; soc->do_ptr(&_archived_main_module_name); if (soc->reading()) { const char* runtime_main_module = Arguments::get_property("jdk.module.main"); @@ -628,34 +630,34 @@ void Modules::serialize(SerializeClosure* soc) { } void Modules::dump_native_access_flag() { + ResourceMark rm; const char* native_access_names = get_native_access_flags_as_sorted_string(); if (native_access_names != nullptr) { _archived_native_access_flags = ArchiveBuilder::current()->ro_strdup(native_access_names); - os::free((void*)native_access_names); } } const char* Modules::get_native_access_flags_as_sorted_string() { + assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); return get_numbered_property_as_sorted_string("jdk.module.enable.native.access"); } void Modules::serialize_native_access_flags(SerializeClosure* soc) { + ResourceMark rm; soc->do_ptr(&_archived_native_access_flags); if (soc->reading()) { - const char* native_access_names = get_native_access_flags_as_sorted_string(); - check_archived_flag_consistency(_archived_native_access_flags, native_access_names, "jdk.module.enable.native.access"); + check_archived_flag_consistency(_archived_native_access_flags, get_native_access_flags_as_sorted_string(), "jdk.module.enable.native.access"); // Don't hold onto the pointer, in case we might decide to unmap the archive. _archived_native_access_flags = nullptr; - os::free((void*)native_access_names); } } void Modules::dump_addmods_names() { + ResourceMark rm; const char* addmods_names = get_addmods_names_as_sorted_string(); if (addmods_names != nullptr) { _archived_addmods_names = ArchiveBuilder::current()->ro_strdup(addmods_names); - os::free((void*)addmods_names); } } @@ -664,6 +666,7 @@ const char* Modules::get_addmods_names_as_sorted_string() { } void Modules::serialize_addmods_names(SerializeClosure* soc) { + ResourceMark rm; soc->do_ptr(&_archived_addmods_names); if (soc->reading()) { const char* addmods_names = get_addmods_names_as_sorted_string(); @@ -671,12 +674,11 @@ void Modules::serialize_addmods_names(SerializeClosure* soc) { // Don't hold onto the pointer, in case we might decide to unmap the archive. _archived_addmods_names = nullptr; - os::free((void*)addmods_names); } } const char* Modules::get_numbered_property_as_sorted_string(const char* property) { - ResourceMark rm; + assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); // theoretical string size limit for decimal int, but the following loop will end much sooner due to // OS command-line size limit. const int max_digits = 10; @@ -729,7 +731,7 @@ const char* Modules::get_numbered_property_as_sorted_string(const char* property } } - return (st.size() > 0) ? os::strdup(st.as_string()) : nullptr; // Example: "java.base,java.compiler" + return (st.size() > 0) ? st.as_string() : nullptr; // Example: "java.base,java.compiler" } void Modules::define_archived_modules(Handle h_platform_loader, Handle h_system_loader, TRAPS) { From d553afa50df024bce6383f7c7d14419b64ac853b Mon Sep 17 00:00:00 2001 From: "zhengyu.gu" Date: Wed, 15 Jan 2025 13:34:53 -0500 Subject: [PATCH 3/7] Calvin's comment --- src/hotspot/share/classfile/modules.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/hotspot/share/classfile/modules.cpp b/src/hotspot/share/classfile/modules.cpp index e2bc8f797dce8..c179b1654758e 100644 --- a/src/hotspot/share/classfile/modules.cpp +++ b/src/hotspot/share/classfile/modules.cpp @@ -669,8 +669,7 @@ void Modules::serialize_addmods_names(SerializeClosure* soc) { ResourceMark rm; soc->do_ptr(&_archived_addmods_names); if (soc->reading()) { - const char* addmods_names = get_addmods_names_as_sorted_string(); - check_archived_flag_consistency(_archived_addmods_names, addmods_names, "jdk.module.addmods"); + check_archived_flag_consistency(_archived_addmods_names, get_addmods_names_as_sorted_string(), "jdk.module.addmods"); // Don't hold onto the pointer, in case we might decide to unmap the archive. _archived_addmods_names = nullptr; From 01ffa01536a5c05f9e25674a91eca116d61fc525 Mon Sep 17 00:00:00 2001 From: "zhengyu.gu" Date: Wed, 15 Jan 2025 14:18:41 -0500 Subject: [PATCH 4/7] Assertion in wrong place --- src/hotspot/share/classfile/modules.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/hotspot/share/classfile/modules.cpp b/src/hotspot/share/classfile/modules.cpp index c179b1654758e..52c845cd25f06 100644 --- a/src/hotspot/share/classfile/modules.cpp +++ b/src/hotspot/share/classfile/modules.cpp @@ -572,7 +572,6 @@ void Modules::dump_main_module_name() { } void Modules::check_archived_flag_consistency(char* archived_flag, const char* runtime_flag, const char* property) { - assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); log_info(cds)("%s %s", property, archived_flag != nullptr ? archived_flag : "(null)"); bool disable = false; @@ -615,7 +614,6 @@ void Modules::serialize_archived_module_info(SerializeClosure* soc) { } void Modules::serialize(SerializeClosure* soc) { - ResourceMark rm; soc->do_ptr(&_archived_main_module_name); if (soc->reading()) { const char* runtime_main_module = Arguments::get_property("jdk.module.main"); @@ -662,6 +660,7 @@ void Modules::dump_addmods_names() { } const char* Modules::get_addmods_names_as_sorted_string() { + assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); return get_numbered_property_as_sorted_string("jdk.module.addmods"); } From be5f90ff849b69c601228c3b9febdc07288c6ae0 Mon Sep 17 00:00:00 2001 From: "zhengyu.gu" Date: Thu, 16 Jan 2025 09:11:19 -0500 Subject: [PATCH 5/7] David's comment --- src/hotspot/share/classfile/modules.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/hotspot/share/classfile/modules.cpp b/src/hotspot/share/classfile/modules.cpp index 52c845cd25f06..1f05a0ed1a4aa 100644 --- a/src/hotspot/share/classfile/modules.cpp +++ b/src/hotspot/share/classfile/modules.cpp @@ -635,6 +635,7 @@ void Modules::dump_native_access_flag() { } } +// Caller needs ResourceMark const char* Modules::get_native_access_flags_as_sorted_string() { assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); return get_numbered_property_as_sorted_string("jdk.module.enable.native.access"); @@ -659,6 +660,7 @@ void Modules::dump_addmods_names() { } } +// Caller needs ResourceMark const char* Modules::get_addmods_names_as_sorted_string() { assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); return get_numbered_property_as_sorted_string("jdk.module.addmods"); @@ -675,6 +677,7 @@ void Modules::serialize_addmods_names(SerializeClosure* soc) { } } +// Caller needs ResourceMark const char* Modules::get_numbered_property_as_sorted_string(const char* property) { assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); // theoretical string size limit for decimal int, but the following loop will end much sooner due to From 5131735dc700352c0fe31ac906df88b873d8fd54 Mon Sep 17 00:00:00 2001 From: "zhengyu.gu" Date: Thu, 16 Jan 2025 14:04:31 -0500 Subject: [PATCH 6/7] Move RM close to use --- src/hotspot/share/classfile/modules.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/classfile/modules.cpp b/src/hotspot/share/classfile/modules.cpp index 1f05a0ed1a4aa..6b4049376f67a 100644 --- a/src/hotspot/share/classfile/modules.cpp +++ b/src/hotspot/share/classfile/modules.cpp @@ -642,9 +642,9 @@ const char* Modules::get_native_access_flags_as_sorted_string() { } void Modules::serialize_native_access_flags(SerializeClosure* soc) { - ResourceMark rm; soc->do_ptr(&_archived_native_access_flags); if (soc->reading()) { + ResourceMark rm; check_archived_flag_consistency(_archived_native_access_flags, get_native_access_flags_as_sorted_string(), "jdk.module.enable.native.access"); // Don't hold onto the pointer, in case we might decide to unmap the archive. @@ -667,9 +667,9 @@ const char* Modules::get_addmods_names_as_sorted_string() { } void Modules::serialize_addmods_names(SerializeClosure* soc) { - ResourceMark rm; soc->do_ptr(&_archived_addmods_names); if (soc->reading()) { + ResourceMark rm; check_archived_flag_consistency(_archived_addmods_names, get_addmods_names_as_sorted_string(), "jdk.module.addmods"); // Don't hold onto the pointer, in case we might decide to unmap the archive. From e970075d0361e5355481168b635869694cbaa072 Mon Sep 17 00:00:00 2001 From: "zhengyu.gu" Date: Sat, 18 Jan 2025 19:58:57 -0500 Subject: [PATCH 7/7] David's comment --- src/hotspot/share/classfile/modules.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/hotspot/share/classfile/modules.cpp b/src/hotspot/share/classfile/modules.cpp index 6b4049376f67a..a63fbc1eb747e 100644 --- a/src/hotspot/share/classfile/modules.cpp +++ b/src/hotspot/share/classfile/modules.cpp @@ -637,7 +637,6 @@ void Modules::dump_native_access_flag() { // Caller needs ResourceMark const char* Modules::get_native_access_flags_as_sorted_string() { - assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); return get_numbered_property_as_sorted_string("jdk.module.enable.native.access"); } @@ -662,7 +661,6 @@ void Modules::dump_addmods_names() { // Caller needs ResourceMark const char* Modules::get_addmods_names_as_sorted_string() { - assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); return get_numbered_property_as_sorted_string("jdk.module.addmods"); } @@ -679,7 +677,6 @@ void Modules::serialize_addmods_names(SerializeClosure* soc) { // Caller needs ResourceMark const char* Modules::get_numbered_property_as_sorted_string(const char* property) { - assert(Thread::current()->current_resource_mark() != nullptr, "Setup by caller"); // theoretical string size limit for decimal int, but the following loop will end much sooner due to // OS command-line size limit. const int max_digits = 10;