Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes [v3]
On Fri, 26 Apr 2024 13:34:08 GMT, Adam Sotona 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: > > Apply suggestions from code review > > Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Very nice. I think we got lucky this worked out :-) - Marked as reviewed by psandoz (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18907#pullrequestreview-2025411091
Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes [v3]
> 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: Apply suggestions from code review Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/18907/files - new: https://git.openjdk.org/jdk/pull/18907/files/e706346b..4a28694f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18907=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18907=01-02 Stats: 9 lines in 1 file changed: 4 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/18907.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18907/head:pull/18907 PR: https://git.openjdk.org/jdk/pull/18907
Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes [v2]
On Fri, 26 Apr 2024 07:43:01 GMT, Adam Sotona 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 readEntry(int pos, Class cls, int
> lowerBoundTag, int upperBoundTag) {
Suggestion:
private T readEntry(int pos, Class cls, int
expectedTag) {
return readEntry(pos, cls, expectedTag, expectedTag);
}
private T readEntry(int pos, Class 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
Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes [v2]
On Thu, 25 Apr 2024 20:16:09 GMT, Paul Sandoz wrote: > It could be two tags, a lower and upper bound, because TAG_FIELDREF, > TAG_METHODREF, and TAG_INTERFACEMETHODREF are consecutive values (9 to 11). OK, I've implemented it with lower and upper bound tags. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18907#issuecomment-2078828894
Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes [v2]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/18907/files - new: https://git.openjdk.org/jdk/pull/18907/files/ce3bd205..e706346b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18907=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18907=00-01 Stats: 91 lines in 1 file changed: 17 ins; 53 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/18907.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18907/head:pull/18907 PR: https://git.openjdk.org/jdk/pull/18907
Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
On Thu, 25 Apr 2024 00:51:41 GMT, Adam Sotona wrote: > Unfortunately it would have to be an expected tags list or an extra > constructed bit mask, due to the multiple tags allowed for MemberRefEntry and > it would slightly affect the performance. Ah yes, i missed that. It could be two tags, a lower and upper bound, because TAG_FIELDREF, TAG_METHODREF, and TAG_INTERFACEMETHODREF are consecutive values (9 to 11). - PR Comment: https://git.openjdk.org/jdk/pull/18907#issuecomment-2078099914
Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
On Wed, 24 Apr 2024 21:52:11 GMT, Paul Sandoz wrote: > Rather than duplicating some checks I wonder if it is possible to add a > private method `entryByIndex(int index, int expectedTag)` that the existing > `entryByIndex` defers to. If the `expectedTag` is non-negative then it checks > `tag` against `expectedTag` before proceeding to the switch expression. Then > the implementations of `readClassEntry` etc can be adjusted to pass along the > expected tag. Unfortunately it would have to be an expected tags list or an extra constructed bit mask, due to the multiple tags allowed for MemberRefEntry and it would slightly affect the performance. Also an additional info for the exception message would have to be passed down (or the exception would have to be re-thrown), as the tag mask is not very informative. - PR Comment: https://git.openjdk.org/jdk/pull/18907#issuecomment-2076114167
Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
On Tue, 23 Apr 2024 07:39:47 GMT, Adam Sotona 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 Rather than duplicating some checks I wonder if it is possible to add a private method `entryByIndex(int index, int expectedTag)` that the existing `entryByIndex` defers to. If the `expectedTag` is non-negative then it checks `tag` against `expectedTag` before proceeding to the switch expression. Then the implementations of `readClassEntry` etc can be adjusted to pass along the expected tag. - PR Review: https://git.openjdk.org/jdk/pull/18907#pullrequestreview-2021009969
RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
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 - Commit messages: - added bug# - 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytesactory.java Changes: https://git.openjdk.org/jdk/pull/18907/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18907=00 Issue: https://bugs.openjdk.org/browse/JDK-8330684 Stats: 84 lines in 2 files changed: 60 ins; 5 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/18907.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18907/head:pull/18907 PR: https://git.openjdk.org/jdk/pull/18907
