On Wed, 28 Oct 2026 14:45:02 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). I've taken an initial pass through this and it seems reasonable in principle. A few initial queries. Thanks src/java.instrument/share/native/libinstrument/InvocationAdapter.c line 152: > 150: char * jarfile = NULL; > 151: char * options = NULL; > 152: jarAttribute* attributes = NULL; Suggestion: jarAttribute * attributes = NULL; For consistency with other declarations. (I see this was present in the original line that got moved.) src/java.instrument/share/native/libinstrument/InvocationAdapter.c line 259: > 257: releaseAndReturn: > 258: if (result != JNI_OK) { > 259: destroyJPLISAgent(agent); Is this a safe/valid call if the error came from `createNewJPLISAgent`? src/java.instrument/share/native/libinstrument/InvocationAdapter.c line 443: > 441: releaseAndReturn: > 442: if (result != JNI_OK) { > 443: destroyJPLISAgent(agent); Same query about safety here. src/java.instrument/share/native/libinstrument/JPLISAgent.c line 270: > 268: return JPLIS_INIT_ERROR_ALLOCATION_FAILURE; > 269: } > 270: strcpy((char *) agent->mJarfile, jarfile); strncpy for safety? ------------- PR Review: https://git.openjdk.org/jdk/pull/33608#pullrequestreview-5691592893 PR Review Comment: https://git.openjdk.org/jdk/pull/33608#discussion_r4407644492 PR Review Comment: https://git.openjdk.org/jdk/pull/33608#discussion_r4407659644 PR Review Comment: https://git.openjdk.org/jdk/pull/33608#discussion_r4407687338 PR Review Comment: https://git.openjdk.org/jdk/pull/33608#discussion_r4407701324
