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

2024-04-26 Thread Paul Sandoz
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]

2024-04-26 Thread Adam Sotona
> 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]

2024-04-26 Thread ExE Boss
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]

2024-04-26 Thread Adam Sotona
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]

2024-04-26 Thread Adam Sotona
> 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

2024-04-25 Thread Paul Sandoz
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

2024-04-24 Thread Adam Sotona
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

2024-04-24 Thread Paul Sandoz
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

2024-04-23 Thread Adam Sotona
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