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

[David Holmes]

On Mon, 9 Dec 2024 13:27:37 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/share/classfile/verifier.cpp line 137:
>> 
>>> 135:   return (should_verify_class &&
>>> 136:     // Override parameter (return false) if -Xverify:none
>>> 137:     BytecodeVerificationRemote &&
>> 
>> Sorry but I don't understand why the correct value for `should_verify` doesn't filter through from `Verifier::should_verify_for`??? Seems to me that is the only place where we should explicitly check the verification flags.
>
> 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.

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

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