RFR 8219579: Remove redundant signature parsing from the verifier
harold.seigel at oracle.com
Fri Mar 1 13:59:18 UTC 2019
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:
>> 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:
>> 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.
> - 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"
> Thanks, I don't need to see another webrev.
More information about the hotspot-runtime-dev