RFR 8219579: Remove redundant signature parsing from the verifier

[David Holmes]

Hi Harold,

On 1/03/2019 11:59 pm, 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.

I don't think you can really separate these. If the parsing in the 
verifier is actually redundant then it must be that the parsing checks 
in the CFP are unconditional. Which means CFP relaxations need to be 
examined in the current context.

The possible permutations here seem more complex than the initial claim 
of simple redundancy. This bug doesn't look like it is simply removing 
redundancy, but will actually be reworking what is actually quite 
complex interactions between the CFP and the verifier that depend on 
several flags and the classloader involved.

Thanks,
David

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