RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v4]

[Chen Liang]

On Thu, 2 May 2024 10:30:06 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only bytecode-level class verification.
>> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with additional class checks inspired by `hotspot/share/classfile/classFileParser.cpp`.
>> 
>> Also new `VerifierSelfTest::testParserVerifier` has been added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 28 commits:
> 
>  - Merge branch 'master' into JDK-8320396-verifier-extension
>  - added references to jvms
>  - Merge remote-tracking branch 'openjdk/master' into JDK-8320396-verifier-extension
>  - work in progress
>  - work in progress
>  - work in progress
>  - work in progress
>  - work in progress
>  - removed string templates from test
>  - work in progress
>  - ... and 18 more: https://git.openjdk.org/jdk/compare/ae82405f...3ebc780a

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java line 97:

> 95:                     check.accept(fre.owner()::asSymbol);
> 96:                     check.accept(fre::typeSymbol);
> 97:                     yield () -> verifyFieldName(fre.name().stringValue());

Nitpick, we should avoid capturing the check instance and just do something like:

                case FieldRefEntry fre -> () -> {
                    fre.owner().asSymbol();
                    fre.typeSymbol();
                    verifyFieldName(fre.name().stringValue());
                };

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java line 151:

> 149:         var fields = new HashSet<String>();
> 150:         for (var f : classModel.fields()) try {
> 151:             if (!fields.add(f.fieldName().stringValue() + f.fieldType().stringValue())) {

We should declare a local record, concat is not safe if we have fields like:

Loop foo;
oop fooL;

both will producce `fooLLoop;`

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java line 163:

> 161:         var methods = new HashSet<String>();
> 162:         for (var m : classModel.methods()) try {
> 163:             if (!methods.add(m.methodName().stringValue() + m.methodType().stringValue())) {

This one is safe as `(` is safe, but still preferable to use a local record as key

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java line 167:

> 165:             }
> 166:             if (m.methodName().equalsString(CLASS_INIT_NAME)
> 167:                     && !m.flags().has(AccessFlag.STATIC)) {

Do we verify it has void return and (since class file version JAVA 7) takes no args? The static requirement is since JAVA 7 too.

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java line 364:

> 362:                     className(),
> 363:                     m.methodName().stringValue(),
> 364:                     m.methodTypeSymbol().parameterList().stream().map(ClassDesc::displayName).collect(Collectors.joining(",")));

Suggestion:

                    m.methodTypeSymbol().displayDescriptor();

Can remove the parentheses above.

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/VerificationWrapper.java line 134:

> 132:         }
> 133: 
> 134:         String parameters() {

We should just use the mtd's displayDescriptor, a class file can have bridge methods with covariant return types and the bridge may be broken while the concrete method is ok.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592914387
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592917226
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592919450
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592922781
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592925732
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592926751