On Thu, 29 Oct 2026 07:31:38 GMT, Timofei Pushkin <[email protected]> wrote:
>> Fixes leaks I could identify on `Agent_OnAttach` failure paths. >> >> In `libinstrument`: >> - the changes to `Agent_OnAttach` are the important ones, while >> `DEF_Agent_OnLoad` and `loadAgent` are updated for hygiene and uniformity >> (when they fail the process gets killed anyway) >> - besides implementing a proper cleanup (the "Improve failure cleanup in >> libinstrument" commit), I also reordered a few agent initialization steps to >> not create the JVM TI environment and change the classpath before the >> provided JAR is validated (the "Reorder blocks to minimize unrecoverable >> changes" commit) >> >> The HotSpot change is adding a destructor to `JvmtiAgent` for the `delete` >> in `JvmtiAgentList::load_agent` executed when `Agent_OnAttach` fails. >> >> Tested by running `jdk_instrument`, `svc_tools`, `vmTestbase_nsk_aod`, >> `hotspot/vmTestbase/nsk/jvmti/AttachOnDemand`, >> `hotspot/serviceability/jvmti/RedefineClasse`. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Timofei Pushkin has updated the pull request incrementally with one > additional commit since the last revision: > > Address the review The fix looks good in general. I've posted a couple of questions. I wonder if a cleanup is needed for two more cases? src/java.instrument/share/native/libinstrument/InvocationAdapter.c line 435: > 433: result = AGENT_ERROR_STARTFAIL; > 434: /* Java code may retain a link to the native agent, even if an > exception was thrown */ > 435: goto releaseAndReturnAfterStartJavaAgent; Q: Does this case needs a cleanup as well? src/java.instrument/share/native/libinstrument/InvocationAdapter.c line 556: > 554: if (!startJavaAgent(agent, env, agentClassJNI, "", > agent->mAgentmainCaller)) { > 555: // Java code may retain a link to the native agent, even if an > exception was thrown > 556: goto releaseAndReturnAfterStartJavaAgent; Q: Does this case needs a cleanup as well? (same as at line 432) ------------- PR Comment: https://git.openjdk.org/jdk/pull/33608#issuecomment-6628511574 PR Review Comment: https://git.openjdk.org/jdk/pull/33608#discussion_r4466408248 PR Review Comment: https://git.openjdk.org/jdk/pull/33608#discussion_r4466413871
