8353189: [ASAN] memory leak after 8352184 #24299

sendaoYan · 2025-03-28T16:40:44Z

Hi all,

This PR will try to fix memory leak after JDK-8352184. which re-do JDK-8352184 using the original, purely static uses of the various description strings, as @jianglizhou had proposed.

Additional testing:

jtreg tests(include tier1/2/3 etc.) on linux-x64
jtreg tests(include tier1/2/3 etc.) on linux-aarch64
full java -version tests, the test shell script show as below.

JDK-8353189.sh.txt

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8353189: [ASAN] memory leak after 8352184 (Bug - P3)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Jiangli Zhou (@jianglizhou - Reviewer)

Contributors

Jiangli Zhou <jiangli@openjdk.org>
David Holmes <dholmes@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24299/head:pull/24299
$ git checkout pull/24299

Update a local copy of the PR:
$ git checkout pull/24299
$ git pull https://git.openjdk.org/jdk.git pull/24299/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24299

View PR using the GUI difftool:
$ git pr show -t 24299

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24299.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-28T16:41:29Z

👋 Welcome back syan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-03-28T16:42:12Z

@sendaoYan This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8353189: [ASAN] memory leak after 8352184

Co-authored-by: Jiangli Zhou <jiangli@openjdk.org>
Co-authored-by: David Holmes <dholmes@openjdk.org>
Reviewed-by: dholmes, jiangli

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 218 new commits pushed to the master branch:

f94a4f7: 8353847: Remove extra args to System.out.printf in open/test/jdk/java/net/httpclient tests
04e2a06: 8351660: C2: SIGFPE in unsigned_mod_value
73c8c75: 8351157: Clean up x86 GC barriers after 32-bit x86 removal
... and 215 more: https://git.openjdk.org/jdk/compare/d8090337ee8ea763bca4e8e3baaf8ee4dd3d6214...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-03-28T16:42:49Z

@sendaoYan The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-03-28T16:48:35Z

Webrevs

dholmes-ora · 2025-03-31T02:13:48Z

You can't just free the result as it may not have always been malloc'd. The intention/expectation was that this was a one-off allocation in VM_version_string that was never freed.

zhengyu123 · 2025-03-31T23:02:05Z

Maybe you want to cache version string, just as vm_release string here.

sendaoYan · 2025-04-01T01:19:42Z

Maybe you want to cache version string, just as vm_release string here.

Thanks, I will try it later.

sendaoYan · 2025-04-01T13:02:51Z

Maybe you want to cache version string, just as vm_release string here.

Below patch is try to cache version string, this patch will fix the memory leak error, but can't get the right version with -Xcomp mode. Before this PR, jvm call function Abstract_VM_Version::vm_info_string() at the third times then get compiled mode. After this PR, jvm will only cache the version string at the first time. So cache version string seems can't solve this memory leak issue.

diff --git a/src/hotspot/share/runtime/abstract_vm_version.cpp b/src/hotspot/share/runtime/abstract_vm_version.cpp
index 763e441fe54..cd81d89c31f 100644
--- a/src/hotspot/share/runtime/abstract_vm_version.cpp
+++ b/src/hotspot/share/runtime/abstract_vm_version.cpp
@@ -31,6 +31,7 @@
 
 const char* Abstract_VM_Version::_s_vm_release = Abstract_VM_Version::vm_release();
 const char* Abstract_VM_Version::_s_internal_vm_info_string = Abstract_VM_Version::internal_vm_info_string();
+const char* Abstract_VM_Version::_s_vm_info_string = Abstract_VM_Version::vm_info_string();
 
 uint64_t Abstract_VM_Version::_features = 0;
 const char* Abstract_VM_Version::_features_string = "";
diff --git a/src/hotspot/share/runtime/abstract_vm_version.hpp b/src/hotspot/share/runtime/abstract_vm_version.hpp
index 8cfc7031f97..0b8a3c662d0 100644
--- a/src/hotspot/share/runtime/abstract_vm_version.hpp
+++ b/src/hotspot/share/runtime/abstract_vm_version.hpp
@@ -50,6 +50,9 @@ class Abstract_VM_Version: AllStatic {
   friend class VMStructs;
   friend class JVMCIVMStructs;
 
+ public:
+  static const char*  _s_vm_info_string;
+
  protected:
   static const char*  _s_vm_release;
   static const char*  _s_internal_vm_info_string;
diff --git a/src/hotspot/share/runtime/arguments.cpp b/src/hotspot/share/runtime/arguments.cpp
index 8de6f427c3f..dae2199e738 100644
--- a/src/hotspot/share/runtime/arguments.cpp
+++ b/src/hotspot/share/runtime/arguments.cpp
@@ -390,7 +390,7 @@ void Arguments::init_system_properties() {
   PropertyList_add(&_system_properties, new SystemProperty("jdk.debug", VM_Version::jdk_debug_level(),  false));
 
   // Initialize the vm.info now, but it will need updating after argument parsing.
-  _vm_info = new SystemProperty("java.vm.info", VM_Version::vm_info_string(), true);
+  _vm_info = new SystemProperty("java.vm.info", VM_Version::_s_vm_info_string, true);
 
   // Following are JVMTI agent writable properties.
   // Properties values are set to nullptr and they are
@@ -1326,7 +1326,7 @@ void Arguments::set_mode_flags(Mode mode) {
   // Ensure Agent_OnLoad has the correct initial values.
   // This may not be the final mode; mode may change later in onload phase.
   PropertyList_unique_add(&_system_properties, "java.vm.info",
-                          VM_Version::vm_info_string(), AddProperty, UnwriteableProperty, ExternalProperty);
+                          VM_Version::_s_vm_info_string, AddProperty, UnwriteableProperty, ExternalProperty);
 
   UseInterpreter             = true;
   UseCompiler                = true;
diff --git a/src/hotspot/share/runtime/threads.cpp b/src/hotspot/share/runtime/threads.cpp
index 32859bf2718..319e238ee5f 100644
--- a/src/hotspot/share/runtime/threads.cpp
+++ b/src/hotspot/share/runtime/threads.cpp
@@ -651,7 +651,7 @@ jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) {
   // is initially computed. See Abstract_VM_Version::vm_info_string().
   // This update must happen before we initialize the java classes, but
   // after any initialization logic that might modify the flags.
-  Arguments::update_vm_info_property(VM_Version::vm_info_string());
+  Arguments::update_vm_info_property(VM_Version::_s_vm_info_string);
 
   JavaThread* THREAD = JavaThread::current(); // For exception macros.
   HandleMark hm(THREAD);
@@ -1334,7 +1334,7 @@ void Threads::print_on(outputStream* st, bool print_stacks,
   st->print_cr("Full thread dump %s (%s %s):",
                VM_Version::vm_name(),
                VM_Version::vm_release(),
-               VM_Version::vm_info_string());
+               VM_Version::_s_vm_info_string);
   st->cr();
 
 #if INCLUDE_SERVICES
diff --git a/src/hotspot/share/runtime/vmStructs.cpp b/src/hotspot/share/runtime/vmStructs.cpp
index c95dd709c84..854f46ff8df 100644
--- a/src/hotspot/share/runtime/vmStructs.cpp
+++ b/src/hotspot/share/runtime/vmStructs.cpp
@@ -702,6 +702,7 @@
                                                                                                                                      \
      static_field(Abstract_VM_Version,         _s_vm_release,                                 const char*)                           \
      static_field(Abstract_VM_Version,         _s_internal_vm_info_string,                    const char*)                           \
+     static_field(Abstract_VM_Version,         _s_vm_info_string,                             const char*)                           \
      static_field(Abstract_VM_Version,         _features,                                     uint64_t)                              \
      static_field(Abstract_VM_Version,         _features_string,                              const char*)                           \
      static_field(Abstract_VM_Version,         _vm_major_version,                             int)                                   \
diff --git a/src/hotspot/share/utilities/vmError.cpp b/src/hotspot/share/utilities/vmError.cpp
index bbf1fcf9d6f..ba2116ae205 100644
--- a/src/hotspot/share/utilities/vmError.cpp
+++ b/src/hotspot/share/utilities/vmError.cpp
@@ -513,7 +513,7 @@ static void report_vm_version(outputStream* st, char* buf, int buflen) {
                 (*vendor_version != '\0') ? " " : "", vendor_version,
                  jdk_debug_level,
                  VM_Version::vm_release(),
-                 VM_Version::vm_info_string(),
+                 VM_Version::_s_vm_info_string,
                  TieredCompilation ? ", tiered" : "",
 #if INCLUDE_JVMCI
                  EnableJVMCI ? ", jvmci" : "",

dholmes-ora · 2025-04-02T02:39:10Z

I was also going to suggest caching the vm_info string as it should be the same all the time. I think you have discovered a bug in the way the info is being requested before arguments (like Xcomp) have been processed. That would cause the wrong info string to be recorded by the early callers.

sendaoYan · 2025-04-02T02:54:57Z

I was also going to suggest caching the vm_info string as it should be the same all the time. I think you have discovered a bug in the way the info is being requested before arguments (like Xcomp) have been processed. That would cause the wrong info string to be recorded by the early callers.

Thanks @dholmes-ora , I think I need some time to investigate this issue.

dholmes-ora · 2025-04-02T07:25:36Z

I think I should file a separate bug to deal with the problem that the info string can be used before its true value is actually known.

dholmes-ora · 2025-04-06T22:01:20Z

After looking into the details (JDK-8353595) I don't think there is any choice but to re-do JDK-8352184 using the original, purely static uses of the various description strings, as [~jiangli] had proposed.

This reverts commit 71bc3ad.

jianglizhou · 2025-04-08T02:58:49Z

src/hotspot/share/runtime/abstract_vm_version.cpp

 }


 const char* Abstract_VM_Version::vm_info_string() {


For future benefit, how about also adding a comment explain why we avoid dynamic memory allocation for the vm_info_string here?

I have add a comment, but my English is not very well. If you have proper description, please let me known.

Looks ok to me, thanks. Please update the contributor properly since the change is from baff6b1.

Okey, that was my original plan also.

By the way, do you mind add David Holmes as contributor also, because he do lots of investigation on this issue?

Sounds good to me!

…he vm_info_string

openjdk · 2025-04-09T02:04:38Z

@sendaoYan Unknown command contribut - for a list of valid commands use /help.

sendaoYan · 2025-04-09T02:09:12Z

/contributor add @jianglizhou

openjdk · 2025-04-09T02:09:51Z

@sendaoYan
Contributor Jiangli Zhou <jiangli@openjdk.org> successfully added.

dholmes-ora

This looks good to me. Sorry for the delay in getting it reviewed.

Thanks for fixing this.

openjdk · 2025-04-10T08:39:48Z

@sendaoYan dholmes-ora was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

sendaoYan · 2025-04-10T08:41:15Z

/contributor add @dholmes-ora

openjdk · 2025-04-10T08:41:59Z

@sendaoYan
Contributor David Holmes <dholmes@openjdk.org> successfully added.

sendaoYan · 2025-04-10T08:42:35Z

Thanks for the reviews and suggestions @dholmes-ora @zhengyu123 @jianglizhou

/integrate

openjdk · 2025-04-10T08:42:49Z

Going to push as commit 6545e0d.
Since your change was applied there have been 218 commits pushed to the master branch:

f94a4f7: 8353847: Remove extra args to System.out.printf in open/test/jdk/java/net/httpclient tests
04e2a06: 8351660: C2: SIGFPE in unsigned_mod_value
73c8c75: 8351157: Clean up x86 GC barriers after 32-bit x86 removal
... and 215 more: https://git.openjdk.org/jdk/compare/d8090337ee8ea763bca4e8e3baaf8ee4dd3d6214...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-10T08:42:55Z

@sendaoYan Pushed as commit 6545e0d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8353189: [ASAN] memory leak after 8352184

71bc3ad

openjdk bot added the hotspot hotspot-dev@openjdk.org label Mar 28, 2025

sendaoYan marked this pull request as ready for review March 28, 2025 16:43

openjdk bot added the rfr Pull request is ready for review label Mar 28, 2025

sendaoYan marked this pull request as draft March 28, 2025 16:55

openjdk bot removed the rfr Pull request is ready for review label Mar 28, 2025

sendaoYan force-pushed the jbs8353189 branch from 8e42dcb to 71bc3ad Compare April 7, 2025 12:56

sendaoYan added 2 commits April 7, 2025 20:57

Revert "8353189: [ASAN] memory leak after 8352184"

1110385

This reverts commit 71bc3ad.

Use static static string instead of assemble string dynamic

9d039fd

sendaoYan marked this pull request as ready for review April 7, 2025 13:09

openjdk bot added the rfr Pull request is ready for review label Apr 7, 2025

jianglizhou reviewed Apr 8, 2025

View reviewed changes

add a comment to explain why we avoid dynamic memory allocation for t…

461b0f8

…he vm_info_string

dholmes-ora approved these changes Apr 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 9, 2025

jianglizhou approved these changes Apr 9, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 10, 2025

openjdk bot closed this Apr 10, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 10, 2025

sendaoYan deleted the jbs8353189 branch April 10, 2025 08:42

8353189: [ASAN] memory leak after 8352184 #24299

8353189: [ASAN] memory leak after 8352184 #24299

Uh oh!

Conversation

sendaoYan commented Mar 28, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Mar 28, 2025

Uh oh!

openjdk bot commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 28, 2025

Uh oh!

mlbridge bot commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora commented Mar 31, 2025

Uh oh!

zhengyu123 commented Mar 31, 2025

Uh oh!

sendaoYan commented Apr 1, 2025

Uh oh!

sendaoYan commented Apr 1, 2025

Uh oh!

dholmes-ora commented Apr 2, 2025

Uh oh!

sendaoYan commented Apr 2, 2025

Uh oh!

dholmes-ora commented Apr 2, 2025

Uh oh!

dholmes-ora commented Apr 6, 2025

Uh oh!

jianglizhou Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

sendaoYan Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

jianglizhou Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

sendaoYan Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jianglizhou Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Apr 9, 2025

Uh oh!

sendaoYan commented Apr 9, 2025

Uh oh!

openjdk bot commented Apr 9, 2025

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Apr 10, 2025

Uh oh!

sendaoYan commented Apr 10, 2025

Uh oh!

openjdk bot commented Apr 10, 2025

Uh oh!

sendaoYan commented Apr 10, 2025

Uh oh!

openjdk bot commented Apr 10, 2025

Uh oh!

openjdk bot commented Apr 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

sendaoYan commented Mar 28, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 28, 2025 •

edited

Loading

mlbridge bot commented Mar 28, 2025 •

edited

Loading

sendaoYan Apr 9, 2025 •

edited

Loading