RFR 8219579: Remove redundant signature parsing from the verifier

[coleen.phillimore at oracle.com]

On 3/1/19 8:59 AM, Harold Seigel wrote:
> Hi Lois,
>
> Thanks for the review!
>
> Please see comments inline.
>
> On 2/28/2019 5:16 PM, Lois Foltan wrote:
>>
>> On 2/28/2019 3:57 PM, Harold Seigel wrote:
>>> Hi,
>>>
>>> Please review this fix to remove redundant field and method 
>>> signature parsing from the verifier.  The parsing is redundant 
>>> because field and method signatures are already checked by 
>>> ClassFileParser.
>>>
>>> Note that current behavior, in the odd case where a user enables 
>>> local verification but disables remote verification, is that both 
>>> local and remote classes get verified.  The change in arguments.cpp 
>>> does not change that.
>>>
>>> Open Webrev: 
>>> http://cr.openjdk.java.net/~hseigel/bug_8219579/webrev/index.html
>>>
>>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8219579
>>>
>>> The fix was regression tested by running Mach5 tiers 1 and 2 tests 
>>> and builds on Linux-x64, Solaris, Windows, and Mac OS X, running 
>>> tiers 3-5 tests on Linux-x64, and by running JCK-13 Lang and VM 
>>> tests on Linux-x64.
>>>
>>> Thanks, Harold
>>>
>>
>> Hi Harold,
>>
>> Looks good.  A couple of comments:
>>
>> - is there a way to remove the SignatureVerifier data structure since 
>> it is now only used in an ASSERT situation by verifier.cpp? Could you 
>> use ClassFileParser::verify_legal_method_signature and 
>> ClassFileParser::verify_legal_field_signature within 
>> classfile/verifier.cpp instead?
> I plan to do this in a subsequent cleanup enhancement that would 
> include the above and also reconciling ClassFileParser's _need_verify 
> and _relax_verify flags, and adding some consistency to what is a 
> trusted class loader.  For example, Verifier::should_verify_for() 
> thinks it's the boot loader. ClassFileParser::relax_format_check_for() 
> thinks it's the boot and platform loaders.

Harold,  I'm glad you're addressing this in future changes also, so that 
the three states possible are consistent across the verifier and the 
classfile parser.  It seems the verifier function that both the verifier 
and ClassFileParser call for this case is:

bool Verifier::should_verify_for(oop class_loader, bool 
should_verify_class) {
   return (class_loader == NULL || !should_verify_class) ?
     BytecodeVerificationLocal : BytecodeVerificationRemote;
}


The verifier calls it with:

bool Verifier::is_eligible_for_verification(InstanceKlass* klass, bool 
should_verify_class) {
...
   return (should_verify_for(klass->class_loader(), should_verify_class) &&
+ some special cases.

should_verify_class comes from

oops/instanceKlass.cpp:  return Verifier::verify(this, 
should_verify_class(), THREAD);

   // defineClass specified verification
   bool should_verify_class() const         {
     return (_misc_flags & _misc_should_verify_class) != 0;
   }

Which comes from:

classfile/classFileParser.cpp: ik->set_should_verify_class(_need_verify);


The verification in the ClassFileParser is done (ignoring 
DumpSharedSpaces) if:

     _need_verify = 
Verifier::should_verify_for(_loader_data->class_loader(),
stream->need_verify());

It appears like stream->need_verify() returns false when calls from the 
normal classLoader load_class call.

It looks like the condition to verify the method and field signatures in 
the ClassFileParser matches the condition to check in the verifier, 
except the special cases above.  I think if the check is done for more 
special cases in the ClassFileParser than the verifier, that's okay 
because it is how it's done now.  There are also some special cases from 
the CDS which I didn't walk through, but I assume you have.

This change looks good to me.  Plus good for performance.
Thanks,
Coleen


>
>>
>> - classfile/verifier.cpp
>>   line #2114 - can you change the comment in the assert back to 
>> something specific to condy like "Invalid type for dynamic constant"?
>>   line #2241 & 2717 - nit the other asserts use "Invalid" instead of 
>> "Bad".  My suggestion is to make them all consistently use "Invalid"
>
> Done.
>
> Thanks, Harold
>
>>
>> Thanks, I don't need to see another webrev.
>> Lois