RFR: 8344922: Redefinition verifies the new klass when verification is disabled

[Coleen Phillimore]

On Tue, 10 Dec 2024 08:56:48 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> I could check it in redefinition instead, here:
>> 
>> https://github.com/coleenp/jdk/blob/master/src/hotspot/share/prims/jvmtiRedefineClasses.cpp#L1461
>> and
>> https://github.com/coleenp/jdk/blob/master/src/hotspot/share/prims/jvmtiRedefineClasses.cpp#L1493
>> 
>> Or have another call should_verify_for_redefinition() which verifies things by default true even if from the bootclass.  Whichever you prefer.  The point is that we do want to verify bytecodes given during redefinition even for the bootclass path because they could be wrong and mess up everything.  Unless, one runs with -Xverify:none in which case you don't want to verify.  Although I said in the bug, this seems like a bad idea and there should be no reason to do this.  So, also maybe that can be the resolution.
>
> I guess I am confused as to what the rules are here. The normal rules are that verification is based on whether the file is loaded by trusted loader or not, and the values of ByteCodeVerificationLocal and BytecodeVerificationRemote. But then we wanted to say that for redefinition we always verify even if the loader is trusted. But now we want to say, except if all verification has been explicitly disabled.
> 
> I think I'd rather see a `should_verify_for_redefinition` capture that, if that is what the rules should be.

How are these rules specified?  I checked the JVMTI specification and it doesn't say anything about except:

<br class="Apple-interchange-newline">[JVMTI_ERROR_FAILS_VERIFICATION](https://docs.oracle.com/en/java/javase/23/docs/specs/jvmti.html#JVMTI_ERROR_FAILS_VERIFICATION)	The class bytes fail verification.

The reason we don't verify on the bootclass loader is because we control the bytecodes and it was considered a performance improvement not to do so.  For now I'm ignoring trusted loaders for now because in the code "trusted" includes the application class loader, and that is used for package access and not whether we run the verifier or not.  These rules are not in the specification, but in the documentation for the deprecated -Xverify option which I can't actually find in searches right now.

When classes on the bootclass loader are redefined, we really do want to verify the bytecodes.  There's no performance reason not to, and a lot of bad things that can happen to the system as a whole if the bytecodes are invalid.  So that's why RedefineClasses has always passed "true" to the verifier, but it's been ignored up to now even though the expectation in the redefine classes code was that these bytecodes would be verified.

Jiangli noticed that -Xverify:none does not affect this.  Whether it should or not I guess depends on why verification is turned off.  If for performance, or just running tests in different modes, this is a benign difference in behavior.

What we can do is close this as WNF, and add a release note to [JDK-8330606](https://bugs.openjdk.org/browse/JDK-8330606).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22617#discussion_r1878036921