RFR 8219579: Remove redundant signature parsing from the verifier

[Harold Seigel]

Hi David,

Please see comments inline.

On 3/1/2019 9:07 AM, David Holmes wrote:
> 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.

No.  The signature parsing checks in the CFP do not have to be 
unconditional because verification is conditional.  A class should 
either be strictly format checked (including signature parsing) and 
verified, or neither.  ClassFileParser currently 'enforces' that classes 
being verified must have been strictly format checked by otherwise not 
creating the verifier's stackmap table.

Conversely, as Coleen pointed out in her review of this change, 
ClassFileParser stores, in the instanceKlass, the value of the flag that 
indicates whether or not it did strict format checking. This flag is 
then used to determine whether to verify the class.

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

Strict format checking of a class is enabled by the same few flags and 
class loader that enables verification of that class. It is not complex:

    If neither local nor remote verification is enabled then no classes
    get strictly format checked nor verified.

    If both local and remote verification are enabled then all classes
    get strictly format checked and verified.

    If only remote verification is enabled then remote classes are
    strictly format checked and verified.  Local classes are neither.

    This leaves the one pathological case that is currently broken. 
    This change fixes that case.  Would you prefer I remove that portion
    of the change?

Note that the fix changes the verifier signature checking into asserts.  
This provides sanity checking ensuring that the signature checking is 
truly redundant.  If somehow a bogus signature slipped through 
ClassFileParser then the assert would trigger.  This hasn't happened in 
any of the testing of this change.

Thanks, Harold

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