On Thu, 2 May 2024 10:30:06 GMT, Adam Sotona <asot...@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
