RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v4]

[Alan Bateman]

On Mon, 22 May 2023 23:25:28 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
>> 
>>  - Merge
>>  - Refresh package description
>>  - Merge
>>  - Tweak docs
>>  - Merge
>>  - Draft docs changes
>>  - Merge
>>  - Rename/cleanup
>>  - Merge
>>  - Initial commit
>
> src/java.instrument/share/native/libinstrument/InvocationAdapter.c line 501:
> 
>> 499: 
>> 500:     // create JPLISAgent with JVMTI environment
>> 501:     if (createNewJPLISAgent(vm, &agent, jarfile, JNI_FALSE) != JPLIS_INIT_ERROR_NONE) {
> 
> No warning is ever printed in this case since this is an agent being loaded from an executable JAR. Correct?

That's right, it's just for agents that are loaded into a running VM.

> test/jdk/com/sun/tools/attach/warnings/DynamicLoadWarningTest.java line 90:
> 
>> 88:         } else {
>> 89:             libname = "lib" + JVMTI_AGENT_LIB + ".so";
>> 90:         }
> 
> We have some test library support for this with Platform.sharedLibraryExt(). Unfortunately there is no "prefix" API, or better yet, one that builds out the whole file name, but the following is a bit shorter:
> 
> `libname = (Platform.isWindows() ? "" : "lib") + JVMTI_AGENT_LIB + "." + Platform.sharedLibraryExt();`
> 
> Up to you if you want to make this change

I wasn't aware of Platform.sharedLibraryExt(), it reduces the code a bit here.

> test/jdk/com/sun/tools/attach/warnings/DynamicLoadWarningTest.java line 108:
> 
>> 106:         // no warning should be printed
>> 107:         test((pid, vm) -> vm.loadAgentLibrary(JVMTI_AGENT_LIB), "-XX:+EnableDynamicAgentLoading")
>> 108:                 .shouldNotContain(message);
> 
> How about adding a test case for `-XX:-EnableDynamicAgentLoading`. Same thing in `testJCmdJvmtiAgentLoad()` and `testLoadJavaAgent()` below.

We can't currently test `jcmd JVMTI.agent_load` with `-XX:-EnableDynamicAgentLoading` due to JDK-8304438.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1202172236
PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1202174405
PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1202402863