[PATCH] perf jit: close agent in Agent_OnLoad()

[PATCH] perf jit: close agent in Agent_OnLoad()

[Haoxiang Li]

close agent on error paths in Agent_OnLoad().

Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
---
 tools/perf/jvmti/libjvmti.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
index 82514e6532b8..fd7efae40102 100644
--- a/tools/perf/jvmti/libjvmti.c
+++ b/tools/perf/jvmti/libjvmti.c
@@ -355,7 +355,7 @@ Agent_OnLoad(JavaVM *jvm, char *options, void *reserved __maybe_unused)
 	ret = (*jvm)->GetEnv(jvm, (void *)&jvmti, JVMTI_VERSION_1);
 	if (ret != JNI_OK) {
 		warnx("jvmti: jvmti version 1 not supported");
-		return -1;
+		goto close_agent;
 	}
 
 	/*
@@ -368,7 +368,7 @@ Agent_OnLoad(JavaVM *jvm, char *options, void *reserved __maybe_unused)
 	ret = (*jvmti)->AddCapabilities(jvmti, &caps1);
 	if (ret != JVMTI_ERROR_NONE) {
 		print_error(jvmti, "AddCapabilities", ret);
-		return -1;
+		goto close_agent;
 	}
 	ret = (*jvmti)->GetJLocationFormat(jvmti, &format);
         if (ret == JVMTI_ERROR_NONE && format == JVMTI_JLOCATION_JVMBCI) {
@@ -390,23 +390,27 @@ Agent_OnLoad(JavaVM *jvm, char *options, void *reserved __maybe_unused)
 	ret = (*jvmti)->SetEventCallbacks(jvmti, &cb, sizeof(cb));
 	if (ret != JVMTI_ERROR_NONE) {
 		print_error(jvmti, "SetEventCallbacks", ret);
-		return -1;
+		goto close_agent;
 	}
 
 	ret = (*jvmti)->SetEventNotificationMode(jvmti, JVMTI_ENABLE,
 			JVMTI_EVENT_COMPILED_METHOD_LOAD, NULL);
 	if (ret != JVMTI_ERROR_NONE) {
 		print_error(jvmti, "SetEventNotificationMode(METHOD_LOAD)", ret);
-		return -1;
+		goto close_agent;
 	}
 
 	ret = (*jvmti)->SetEventNotificationMode(jvmti, JVMTI_ENABLE,
 			JVMTI_EVENT_DYNAMIC_CODE_GENERATED, NULL);
 	if (ret != JVMTI_ERROR_NONE) {
 		print_error(jvmti, "SetEventNotificationMode(CODE_GENERATED)", ret);
-		return -1;
+		goto close_agent;
 	}
 	return 0;
+
+close_agent:
+	jvmti_close(jvmti_agent);
+	return -1;
 }
 
 JNIEXPORT void JNICALL
-- 
2.25.1

Re: [PATCH] perf jit: close agent in Agent_OnLoad()

[James Clark]

On 24/12/2025 3:56 am, Haoxiang Li wrote:
> close agent on error paths in Agent_OnLoad().
> 
> Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
> ---
>   tools/perf/jvmti/libjvmti.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
> index 82514e6532b8..fd7efae40102 100644
> --- a/tools/perf/jvmti/libjvmti.c
> +++ b/tools/perf/jvmti/libjvmti.c
> @@ -355,7 +355,7 @@ Agent_OnLoad(JavaVM *jvm, char *options, void *reserved __maybe_unused)
>   	ret = (*jvm)->GetEnv(jvm, (void *)&jvmti, JVMTI_VERSION_1);
>   	if (ret != JNI_OK) {
>   		warnx("jvmti: jvmti version 1 not supported");
> -		return -1;
> +		goto close_agent;
>   	}
>   
>   	/*
> @@ -368,7 +368,7 @@ Agent_OnLoad(JavaVM *jvm, char *options, void *reserved __maybe_unused)
>   	ret = (*jvmti)->AddCapabilities(jvmti, &caps1);
>   	if (ret != JVMTI_ERROR_NONE) {
>   		print_error(jvmti, "AddCapabilities", ret);
> -		return -1;
> +		goto close_agent;
>   	}
>   	ret = (*jvmti)->GetJLocationFormat(jvmti, &format);
>           if (ret == JVMTI_ERROR_NONE && format == JVMTI_JLOCATION_JVMBCI) {
> @@ -390,23 +390,27 @@ Agent_OnLoad(JavaVM *jvm, char *options, void *reserved __maybe_unused)
>   	ret = (*jvmti)->SetEventCallbacks(jvmti, &cb, sizeof(cb));
>   	if (ret != JVMTI_ERROR_NONE) {
>   		print_error(jvmti, "SetEventCallbacks", ret);
> -		return -1;
> +		goto close_agent;
>   	}
>   
>   	ret = (*jvmti)->SetEventNotificationMode(jvmti, JVMTI_ENABLE,
>   			JVMTI_EVENT_COMPILED_METHOD_LOAD, NULL);
>   	if (ret != JVMTI_ERROR_NONE) {
>   		print_error(jvmti, "SetEventNotificationMode(METHOD_LOAD)", ret);
> -		return -1;
> +		goto close_agent;
>   	}
>   
>   	ret = (*jvmti)->SetEventNotificationMode(jvmti, JVMTI_ENABLE,
>   			JVMTI_EVENT_DYNAMIC_CODE_GENERATED, NULL);
>   	if (ret != JVMTI_ERROR_NONE) {
>   		print_error(jvmti, "SetEventNotificationMode(CODE_GENERATED)", ret);
> -		return -1;
> +		goto close_agent;
>   	}
>   	return 0;
> +
> +close_agent:
> +	jvmti_close(jvmti_agent);

Does this actually do anything? jvmti_close() is already called in 
Agent_OnUnload().

The commit message is lacking any details about how this was found or 
what the effect is.

> +	return -1;
>   }
>   
>   JNIEXPORT void JNICALL

Re: [PATCH] perf jit: close agent in Agent_OnLoad()

[Haoxiang Li]

On Wed, 24 Dec 2025 10:39:18 +0000, James Clark wrote:
> Does this actually do anything? jvmti_close() is already called in 
> Agent_OnUnload().

I think Agent_OnUnload() is not called if Agent_Onload() fails, so it
is necessary to release the resource.

> The commit message is lacking any details about how this was found or 
> what the effect is.

Sorry for that. I found it by a static analyzer prototype and comfirmed
by manual review. I think it leads to a resource leak.

If this is ok, I modify the changelog and resubmit it.

Thanks,
Haoxiang Li

Re: [PATCH] perf jit: close agent in Agent_OnLoad()

[James Clark]

On 24/12/2025 11:39 am, Haoxiang Li wrote:
> On Wed, 24 Dec 2025 10:39:18 +0000, James Clark wrote:
>> Does this actually do anything? jvmti_close() is already called in
>> Agent_OnUnload().
> 
> I think Agent_OnUnload() is not called if Agent_Onload() fails, so it
> is necessary to release the resource.
> 

The docs say otherwise and suggest that VMDeath() is actually the one 
that wouldn't be called if startup was unsuccessful. But 
Agent_OnUnload() is always called:

  this function will be called if some platform specific mechanism causes
  the unload (an unload mechanism is not specified in this document) or
  the library is (in effect) unloaded by the termination of the VM
  whether through normal termination or VM failure, including start-up
  failure. ...  Note the distinction between this function and the VM
  Death event: for the VM Death event to be sent, the VM must have run at
  least to the point of initialization

>> The commit message is lacking any details about how this was found or
>> what the effect is.
> 
> Sorry for that. I found it by a static analyzer prototype and comfirmed
> by manual review. I think it leads to a resource leak.
> 
> If this is ok, I modify the changelog and resubmit it.
> 

I don't think it's enough, you have to actually run the code that you 
submit. For all we know it results in some double free and makes it worse.

> Thanks,
> Haoxiang Li
>