On Wed, 23 Sep 2020 15:06:55 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review and a sponsor for a fix for > https://bugs.openjdk.java.net/browse/JDK-8242882? > > As noted in that JBS issue, if the size of the Manifest entry in the jar > happens to be very large (such that it exceeds > the `Integer.MAX_VALUE`), then the current code in `JarFile#getBytes` can > lead to a `NegativeArraySizeException`. This > is due to the: if (len != -1 && len <= 65535) block which evaluates to > `true` when the size of the manifest entry is > larger than `Integer.MAX_VALUE`. As a result, this then ends up calling the > code which can lead to the > `NegativeArraySizeException`. The commit in this PR fixes that issue by > changing those `if/else` blocks to prevent > this issue and instead use a code path that leads to the > `InputStream#readAllBytes()` which internally has the > necessary checks to throw the expected `OutOfMemoryError`. This commit also > includes a jtreg test case which > reproduces the issue and verifies the fix. I had created a copy of this branch in my personal fork and included the `submit.yml` workflow as noted in this recent mail here[1] to try and run the `tier1` testing for this change. But it looks like that has failed for unrelated reasons[2]. So I'll go ahead and trigger the `tier1` tests locally instead. [1] https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html [2] https://github.com/jaikiran/jdk/actions/runs/268948812 test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java line 60: > 58: final OutOfMemoryError oome = > Assert.expectThrows(OutOfMemoryError.class, () -> jar.getManifest()); > 59: // additionally verify that the OOM was for the right/expected > reason > 60: if (!"Required array size too large".equals(oome.getMessage())) { I wasn't too sure if I should add this additional check on the message of the `OutOfMemoryError`, but decided to do it anyway, given that from what I remember there were some discussions in the `core-libs-dev` list a while back on the exact messages that such OOMs should throw. So I guessed that it might be OK to rely on those messages in the tests within this project. However, I am open to removing specific check if it's considered unnecessary. ------------- PR: https://git.openjdk.java.net/jdk/pull/323
