RFR 8219579: Remove redundant signature parsing from the verifier

[Lois Foltan]

On 3/1/2019 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.

Glad to hear, thanks for the follow up!
Lois

>
>>
>> - 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