From 88b4e3b8539c2beb29ad92bd74b300002c2ef84b Mon Sep 17 00:00:00 2001 From: Calvin Cheung Date: Thu, 10 Aug 2023 20:02:27 +0000 Subject: [PATCH] 8304292: Memory leak related to ClassLoader::update_class_path_entry_list Reviewed-by: dholmes, iklam --- src/hotspot/share/classfile/classLoader.cpp | 16 ++++--- src/hotspot/share/classfile/classLoader.hpp | 2 +- .../runtime/cds/appcds/ClassPathAttr.java | 19 ++++++--- .../cds/appcds/DuplicateClassPaths.java | 42 +++++++++++++++++++ .../cds/appcds/test-classes/cpattr_dup.mf | 3 ++ 5 files changed, 70 insertions(+), 12 deletions(-) create mode 100644 test/hotspot/jtreg/runtime/cds/appcds/DuplicateClassPaths.java create mode 100644 test/hotspot/jtreg/runtime/cds/appcds/test-classes/cpattr_dup.mf diff --git a/src/hotspot/share/classfile/classLoader.cpp b/src/hotspot/share/classfile/classLoader.cpp index fcbfdeeb9cf..de06f1b94c7 100644 --- a/src/hotspot/share/classfile/classLoader.cpp +++ b/src/hotspot/share/classfile/classLoader.cpp @@ -523,7 +523,8 @@ void ClassLoader::setup_app_search_path(JavaThread* current, const char *class_p while (cp_stream.has_next()) { const char* path = cp_stream.get_next(); - update_class_path_entry_list(current, path, false, false, false); + update_class_path_entry_list(current, path, /* check_for_duplicates */ true, + /* is_boot_append */ false, /* from_class_path_attr */ false); } } @@ -668,7 +669,8 @@ void ClassLoader::setup_bootstrap_search_path_impl(JavaThread* current, const ch } else { // Every entry on the boot class path after the initial base piece, // which is set by os::set_boot_path(), is considered an appended entry. - update_class_path_entry_list(current, path, false, true, false); + update_class_path_entry_list(current, path, /* check_for_duplicates */ false, + /* is_boot_append */ true, /* from_class_path_attr */ false); } } } @@ -803,7 +805,7 @@ void ClassLoader::add_to_boot_append_entries(ClassPathEntry *new_entry) { // Note that at dump time, ClassLoader::_app_classpath_entries are NOT used for // loading app classes. Instead, the app class are loaded by the // jdk/internal/loader/ClassLoaders$AppClassLoader instance. -void ClassLoader::add_to_app_classpath_entries(JavaThread* current, +bool ClassLoader::add_to_app_classpath_entries(JavaThread* current, ClassPathEntry* entry, bool check_for_duplicates) { #if INCLUDE_CDS @@ -813,7 +815,7 @@ void ClassLoader::add_to_app_classpath_entries(JavaThread* current, while (e != nullptr) { if (strcmp(e->name(), entry->name()) == 0) { // entry already exists - return; + return false; } e = e->next(); } @@ -832,6 +834,7 @@ void ClassLoader::add_to_app_classpath_entries(JavaThread* current, ClassLoaderExt::process_jar_manifest(current, entry); } #endif + return true; } // Returns true IFF the file/dir exists and the entry was successfully created. @@ -854,7 +857,10 @@ bool ClassLoader::update_class_path_entry_list(JavaThread* current, if (is_boot_append) { add_to_boot_append_entries(new_entry); } else { - add_to_app_classpath_entries(current, new_entry, check_for_duplicates); + if (!add_to_app_classpath_entries(current, new_entry, check_for_duplicates)) { + // new_entry is not saved, free it now + delete new_entry; + } } return true; } else { diff --git a/src/hotspot/share/classfile/classLoader.hpp b/src/hotspot/share/classfile/classLoader.hpp index 0733a5a347c..5237e05b4b8 100644 --- a/src/hotspot/share/classfile/classLoader.hpp +++ b/src/hotspot/share/classfile/classLoader.hpp @@ -222,7 +222,7 @@ class ClassLoader: AllStatic { CDS_ONLY(static ClassPathEntry* _last_module_path_entry;) CDS_ONLY(static void setup_app_search_path(JavaThread* current, const char* class_path);) CDS_ONLY(static void setup_module_search_path(JavaThread* current, const char* path);) - static void add_to_app_classpath_entries(JavaThread* current, + static bool add_to_app_classpath_entries(JavaThread* current, ClassPathEntry* entry, bool check_for_duplicates); CDS_ONLY(static void add_to_module_path_entries(const char* path, diff --git a/test/hotspot/jtreg/runtime/cds/appcds/ClassPathAttr.java b/test/hotspot/jtreg/runtime/cds/appcds/ClassPathAttr.java index 5a3bb39b8b9..f0d797c09ae 100644 --- a/test/hotspot/jtreg/runtime/cds/appcds/ClassPathAttr.java +++ b/test/hotspot/jtreg/runtime/cds/appcds/ClassPathAttr.java @@ -55,9 +55,10 @@ public class ClassPathAttr { "CpAttr2", "CpAttr3", "CpAttr4", "CpAttr5"); buildCpAttr("cpattr5_123456789_223456789_323456789_423456789_523456789_623456789", "cpattr5_extra_long.mf", "CpAttr5", "CpAttr5"); + String[] classlist = { "CpAttr1", "CpAttr2", "CpAttr3", "CpAttr4", "CpAttr5"}; + String jar4 = TestCommon.getTestJar("cpattr4.jar"); for (int i=1; i<=2; i++) { String jar1 = TestCommon.getTestJar("cpattr1.jar"); - String jar4 = TestCommon.getTestJar("cpattr4.jar"); if (i == 2) { // Test case #2 -- same as #1, except we use cpattr1_long.jar, which has a super-long // Class-Path: attribute. @@ -65,11 +66,7 @@ public class ClassPathAttr { } String cp = jar1 + File.pathSeparator + jar4; - TestCommon.testDump(cp, TestCommon.list("CpAttr1", - "CpAttr2", - "CpAttr3", - "CpAttr4", - "CpAttr5")); + TestCommon.testDump(cp, classlist); TestCommon.run( "-cp", cp, @@ -86,6 +83,16 @@ public class ClassPathAttr { output.shouldMatch("checking shared classpath entry: .*cpattr3.jar"); }); } + + // test duplicate jars in the "Class-path" attribute in the jar manifest + buildCpAttr("cpattr_dup", "cpattr_dup.mf", "CpAttr1", "CpAttr1"); + String cp = TestCommon.getTestJar("cpattr_dup.jar") + File.pathSeparator + jar4; + TestCommon.testDump(cp, classlist); + + TestCommon.run( + "-cp", cp, + "CpAttr1") + .assertNormalExit(); } static void testNonExistentJars() throws Exception { diff --git a/test/hotspot/jtreg/runtime/cds/appcds/DuplicateClassPaths.java b/test/hotspot/jtreg/runtime/cds/appcds/DuplicateClassPaths.java new file mode 100644 index 00000000000..ce0e1804997 --- /dev/null +++ b/test/hotspot/jtreg/runtime/cds/appcds/DuplicateClassPaths.java @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2023, 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 + * 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 + * @summary duplicate class paths test + * @requires vm.cds + * @library /test/lib + * @compile test-classes/Hello.java + * @run driver DuplicateClassPaths + */ + +import java.io.File; + +public class DuplicateClassPaths { + public static void main(String[] args) throws Exception { + String appJar = JarBuilder.getOrCreateHelloJar(); + String jars = appJar + File.pathSeparator + appJar; + TestCommon.test(jars, TestCommon.list("Hello"), "Hello"); + } +} diff --git a/test/hotspot/jtreg/runtime/cds/appcds/test-classes/cpattr_dup.mf b/test/hotspot/jtreg/runtime/cds/appcds/test-classes/cpattr_dup.mf new file mode 100644 index 00000000000..ad1f974d82d --- /dev/null +++ b/test/hotspot/jtreg/runtime/cds/appcds/test-classes/cpattr_dup.mf @@ -0,0 +1,3 @@ +Manifest-Version: 1.0 +Class-Path: cpattr2.jar cpattr2.jar +Created-By: 1.9.0-internal (Oracle Corporation)