Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes [v2]

[ExE Boss]

On Fri, 26 Apr 2024 07:43:01 GMT, Adam Sotona <asot...@openjdk.org> wrote:

>> ClassFile API dives into the nested constant pool entries without type 
>> restrictions, while parsing a class file. Validation of the entry is 
>> performed post-parsing. Specifically corrupted constant pool entry may cause 
>> infinite loop during parsing and throws SOE.
>> This patch resolves the issue by providing specific implementations for the 
>> nested CP entries parsing, instead of sharing the common (post-checking) 
>> code.
>> Added test simulates the situation on inner-looped method reference entry.
>> 
>> Please review.
>> 
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   implemented proposed simpler solution with lower and upper bound tags

Maybe also add a `readEntry` overload which takes a single expected `TAG_*` 
value for use by `readModuleEntry`, `readPackageEntry`, `readClassEntry`, 
`readNameAndTypeEntry`, and `readMethodHandleEntry`:

src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
line 437:

> 435:     }
> 436: 
> 437:     private <T extends PoolEntry> T readEntry(int pos, Class<T> cls, int 
> lowerBoundTag, int upperBoundTag) {

Suggestion:

    private <T extends PoolEntry> T readEntry(int pos, Class<T> cls, int 
expectedTag) {
        return readEntry(pos, cls, expectedTag, expectedTag);
    }

    private <T extends PoolEntry> T readEntry(int pos, Class<T> cls, int 
lowerBoundTag, int upperBoundTag) {

src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
line 469:

> 467:     @Override
> 468:     public ModuleEntry readModuleEntry(int pos) {
> 469:         return readEntry(pos, ModuleEntry.class, TAG_MODULE, TAG_MODULE);

Suggestion:

        return readEntry(pos, ModuleEntry.class, TAG_MODULE);

src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
line 474:

> 472:     @Override
> 473:     public PackageEntry readPackageEntry(int pos) {
> 474:         return readEntry(pos, PackageEntry.class, TAG_PACKAGE, 
> TAG_PACKAGE);

Suggestion:

        return readEntry(pos, PackageEntry.class, TAG_PACKAGE);

src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
line 479:

> 477:     @Override
> 478:     public ClassEntry readClassEntry(int pos) {
> 479:         return readEntry(pos, ClassEntry.class, TAG_CLASS, TAG_CLASS);

Suggestion:

        return readEntry(pos, ClassEntry.class, TAG_CLASS);

src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
line 484:

> 482:     @Override
> 483:     public NameAndTypeEntry readNameAndTypeEntry(int pos) {
> 484:         return readEntry(pos, NameAndTypeEntry.class, TAG_NAMEANDTYPE, 
> TAG_NAMEANDTYPE);

Suggestion:

        return readEntry(pos, NameAndTypeEntry.class, TAG_NAMEANDTYPE);

src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
line 489:

> 487:     @Override
> 488:     public MethodHandleEntry readMethodHandleEntry(int pos) {
> 489:         return readEntry(pos, MethodHandleEntry.class, TAG_METHODHANDLE, 
> TAG_METHODHANDLE);

Suggestion:

        return readEntry(pos, MethodHandleEntry.class, TAG_METHODHANDLE);

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

PR Review: https://git.openjdk.org/jdk/pull/18907#pullrequestreview-2024849063
PR Review Comment: https://git.openjdk.org/jdk/pull/18907#discussion_r1580909095
PR Review Comment: https://git.openjdk.org/jdk/pull/18907#discussion_r1580909748
PR Review Comment: https://git.openjdk.org/jdk/pull/18907#discussion_r1580909800
PR Review Comment: https://git.openjdk.org/jdk/pull/18907#discussion_r1580909914
PR Review Comment: https://git.openjdk.org/jdk/pull/18907#discussion_r1580910028
PR Review Comment: https://git.openjdk.org/jdk/pull/18907#discussion_r1580910141