RFR: 8351654: Agent transformer bytecodes should be verified

[David Holmes]

On Mon, 31 Mar 2025 16:31:47 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> This change enables verification for classes provided with the JVMTI ClassFileLoadHook.  The test was adapted from the one sent by the submitter @rjernst.
> Tested with tier1-4.

Functional changes look fine - some minor requests regarding comments.

I'm struggling a bit with the test logic.

src/hotspot/share/classfile/klassFactory.cpp line 79:

> 77:       ClassFileStream* stream = new ClassFileStream(ptr,
> 78:                                                     pointer_delta_as_int(end_ptr, ptr),
> 79:                                                     cfs->source(), false, /* from_class_file_load_hook */ true);

Can you comment what the `false` means as well please - looks odd to only document one of the boolean parameters.

src/hotspot/share/classfile/klassFactory.cpp line 158:

> 156:       stream = new ClassFileStream(ptr,
> 157:                                    pointer_delta_as_int(end_ptr, ptr),
> 158:                                    stream->source(), false, /* from_class_file_load_hook */ true);

Can you comment what the `false` means as well please - looks odd to only document one of the boolean parameters.

test/hotspot/jtreg/runtime/modules/PatchModule/PatchModuleJavaBaseVerify.java line 27:

> 25:  * @test
> 26:  * @bug 8351654
> 27:  * @summary Make sure --patch-module for java.base verifies the classfile.

?? But in this scenario the injected class is not verified.

test/hotspot/jtreg/runtime/verifier/CFLH/TestVerify.java line 25:

> 23: 
> 24: /*
> 25:  * @test Verifier should verify ClassFileLoadHook bytes even if on bootclasspath

Suggestion:

 * @test 
 * @summary Verifier should verify ClassFileLoadHook bytes even if on bootclasspath

test/hotspot/jtreg/runtime/verifier/CFLH/TestVerify.java line 71:

> 69:         @Override
> 70:         public byte[] transform(Module module, ClassLoader loader, String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) throws IllegalClassFormatException {
> 71:             //System.out.println("Maybe transforming module class " + className);

Suggestion:

          

Delete the commented out line

test/hotspot/jtreg/runtime/verifier/CFLH/TestVerify.java line 103:

> 101:                     if (DEBUG) Files.write(Path.of("bad.class"), bytes);
> 102:                 } catch (Throwable e) {
> 103:                     System.out.println(e);

Suggestion:



Delete the println here as it will just complicate debugging a failure.

test/hotspot/jtreg/runtime/verifier/CFLH/TestVerify.java line 176:

> 174:             // Now load the class for the VerifyError.
> 175:             System.out.println("1 hour is " + Duration.ofHours(1).getSeconds() + " seconds");
> 176:             if (!verifyErrorThrown) {

I'm a bit confused about the logic here - are there three possibilities:
1. the load threw VerifyError and will be caught below
2. Another part of the code (where?) threw VerifyError, caught it and set `verifyErrorThrown` to true
3. No verification error occurred at all.

How do we trigger case 1 versus case 2?

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24333#pullrequestreview-2745656854
PR Review Comment: https://git.openjdk.org/jdk/pull/24333#discussion_r2030541130
PR Review Comment: https://git.openjdk.org/jdk/pull/24333#discussion_r2030541367
PR Review Comment: https://git.openjdk.org/jdk/pull/24333#discussion_r2030543647
PR Review Comment: https://git.openjdk.org/jdk/pull/24333#discussion_r2030545472
PR Review Comment: https://git.openjdk.org/jdk/pull/24333#discussion_r2030547091
PR Review Comment: https://git.openjdk.org/jdk/pull/24333#discussion_r2030557496
PR Review Comment: https://git.openjdk.org/jdk/pull/24333#discussion_r2030559140