On Thu, 7 Dec 2023 07:53:58 GMT, Adam Sotona <[email protected]> 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
