RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]

[Jaikiran Pai]

On Thu, 7 Dec 2023 07:53:58 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is corrupted so the parser attempts to read beyond the classfile bounds.
>> General contract is that only `IllegalArgumentException` or its subclasses is expected when parser fails.
>> This patch wraps `IndexOutOfBoundsExceptions` thrown from all `ClassReaderImpl.buffer` manipulations into an  `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
>> Relevant tests are 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 three commits:
> 
>  - Merge branch 'master' into JDK-8320360-bounds
>    
>    # Conflicts:
>    #	test/jdk/jdk/classfile/LimitsTest.java
>  - added bug # to the test
>  - 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown

Hello Adam, the changes look good to me. Please update the copyright year on ClassReaderImpl.java. I have a couple of comments which I've added inline.

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

> 198:         try {
> 199:             return buffer[p] & 0xFF;
> 200:         } catch (IndexOutOfBoundsException e) {

This and all other catch blocks introduced in this change can be changed to the specific exception type `ArrayIndexOutOfBoundsException`, because all these operations are dealing with only array access. If you prefer catching this more generic `IndexOutOfBoundsException`, that's fine with me.

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

> 281:     public void copyBytesTo(BufWriter buf, int p, int len) {
> 282:         try {
> 283:             buf.writeBytes(buffer, p, len);

`java.lang.classfile.BufWriter` doesn't specify any `@throws` for its `writeXXX` methods. Should it be specified (of course in a separate PR)?

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

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16762#pullrequestreview-1800010401
PR Review Comment: https://git.openjdk.org/jdk/pull/16762#discussion_r1439194339
PR Review Comment: https://git.openjdk.org/jdk/pull/16762#discussion_r1439194853