Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v3]
On Thu, 1 Oct 2020 14:42:21 GMT, Jaikiran Pai 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. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Second round of review comments addressed Hi Jaikiran, Yes I think you are OK to move forward with the integration, - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
On Wed, 7 Oct 2020 21:40:43 GMT, Brent Christian wrote: >> I decided to slightly change the way this large manifest file was being >> created. I borrowed the idea from >> `Zip64SizeTest`[1] to create the file and set its length to a large value. I >> hope that is OK. If not, let me know, I >> will change this part. [1] >> https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java#L121 > > I did some automated test runs, and the duration of this test is sufficiently > improved, IMO. > > While not representative of a real MANIFEST.MF file, I think it works well > enough for this specific test. Thank you for the review Brent. - PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v3]
On Wed, 7 Oct 2020 21:40:57 GMT, Brent Christian wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Second round of review comments addressed > > Marked as reviewed by bchristi (Reviewer). Hello Lance, does the latest state of this PR look fine to you? If so, shall I trigger a integrate? - PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
On Thu, 1 Oct 2020 14:39:50 GMT, Jaikiran Pai wrote:
>> test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java line 78:
>>
>>> 76: bw.write("OOM-Test: ");
>>> 77: for (long i = 0; i < 2147483648L; i++) {
>>> 78: bw.write("a");
>>
>> As you probably noticed, this test takes a little while to run. One way to
>> speed it up a little would be to write more
>> characters at a time. While we're at it, we may as well make the Manifest
>> well-formed by breaking it into 72-byte
>> lines. See "Line length" under:
>> https://docs.oracle.com/en/java/javase/15/docs/specs/jar/jar.html#notes-on-manifest-and-signature-files
>> Just write
>> enough lines to exceed Integer.MAX_VALUE bytes.
>
> I decided to slightly change the way this large manifest file was being
> created. I borrowed the idea from
> `Zip64SizeTest`[1] to create the file and set its length to a large value. I
> hope that is OK. If not, let me know, I
> will change this part. [1]
> https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java#L121
I did some automated test runs, and the duration of this test is sufficiently
improved, IMO.
While not representative of a real MANIFEST.MF file, I think it works well
enough for this specific test.
-
PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v3]
On Thu, 1 Oct 2020 14:42:21 GMT, Jaikiran Pai 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. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Second round of review comments addressed Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
On Wed, 30 Sep 2020 17:21:14 GMT, Brent Christian wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address the review comments and introduce an array size check in
>> JarFile.getBytes() method itself
>
> src/java.base/share/classes/java/util/jar/JarFile.java line 161:
>
>> 159: * OutOfMemoryError: Required array size too large
>> 160: */
>> 161: private static final int MAX_BUFFER_SIZE = Integer.MAX_VALUE - 8;
>
> "/**" comments are generally only used for public documentation. For use
> here, probably a single line // comment would
> be sufficient to explain what this value is.
> This constant is also named, "MAX_ARRAY_SIZE" in various places, which seems
> more applicable to this case.
Hello Brent,
I've updated the PR with your suggested changes for this member variable name
and the comment.
> src/java.base/share/classes/java/util/jar/JarFile.java line 801:
>
>> 799: if (len > MAX_BUFFER_SIZE) {
>> 800: throw new OutOfMemoryError("Required array size too
>> large");
>> 801: }
>
> I would just add a new `long zeSize` to read and check `ze.getSize()`, and
> then (int) cast it into `len`, as before.
> Then I think no changes would be needed past L802, `int bytesRead;`
Done. Changed it based on your input.
> test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java line 78:
>
>> 76: bw.write("OOM-Test: ");
>> 77: for (long i = 0; i < 2147483648L; i++) {
>> 78: bw.write("a");
>
> As you probably noticed, this test takes a little while to run. One way to
> speed it up a little would be to write more
> characters at a time. While we're at it, we may as well make the Manifest
> well-formed by breaking it into 72-byte
> lines. See "Line length" under:
> https://docs.oracle.com/en/java/javase/15/docs/specs/jar/jar.html#notes-on-manifest-and-signature-files
> Just write
> enough lines to exceed Integer.MAX_VALUE bytes.
I decided to slightly change the way this large manifest file was being
created. I borrowed the idea from
`Zip64SizeTest`[1] to create the file and set its length to a large value. I
hope that is OK. If not, let me know, I
will change this part.
[1]
https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java#L121
-
PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v3]
> 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. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Second round of review comments addressed - Changes: - all: https://git.openjdk.java.net/jdk/pull/323/files - new: https://git.openjdk.java.net/jdk/pull/323/files/279c7c83..a011b0d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=323=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=323=01-02 Stats: 34 lines in 2 files changed: 5 ins; 15 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/323.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/323/head:pull/323 PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v3]
On Wed, 30 Sep 2020 18:38:40 GMT, Lance Andersen wrote: >> I think it's fine either way. > > If you are going to validate the message, which I probably would not, it > would be important to make sure it document > if the message is changed in JarFile::getBytes, that the test needs updated. > Otherwise it will be easy to miss. Hello Lance, I decided to remove the assertion on the exception message. I have updated the PR accordingly. - PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
On Wed, 30 Sep 2020 17:26:18 GMT, Brent Christian wrote:
>> 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.
>
> I think it's fine either way.
If you are going to validate the message, which I probably would not, it would
be important to make sure it document
if the message is changed in JarFile::getBytes, that the test needs updated.
Otherwise it will be easy to miss.
-
PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
On Wed, 23 Sep 2020 15:09:44 GMT, Jaikiran Pai wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address the review comments and introduce an array size check in
>> JarFile.getBytes() method itself
>
> 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.
I think it's fine either way.
-
PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
On Tue, 29 Sep 2020 11:39:20 GMT, Jaikiran Pai 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.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address the review comments and introduce an array size check in
> JarFile.getBytes() method itself
If there is only modest improvement in test duration, we may want to add a
jtreg timeout tag. Also, given the long
duration but relative low priority of testing this, it perhaps should be moved
out of Tier 1. I will look into those
things after your next update.
src/java.base/share/classes/java/util/jar/JarFile.java line 161:
> 159: * OutOfMemoryError: Required array size too large
> 160: */
> 161: private static final int MAX_BUFFER_SIZE = Integer.MAX_VALUE - 8;
"/**" comments are generally only used for public documentation. For use here,
probably a single line // comment would
be sufficient to explain what this value is.
This constant is also named, "MAX_ARRAY_SIZE" in various places, which seems
more applicable to this case.
src/java.base/share/classes/java/util/jar/JarFile.java line 801:
> 799: if (len > MAX_BUFFER_SIZE) {
> 800: throw new OutOfMemoryError("Required array size too
> large");
> 801: }
I would just add a new `long zeSize` to read and check `ze.getSize()`, and then
(int) cast it into `len`, as before.
Then I think no changes would be needed past L802, `int bytesRead;`
test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java line 45:
> 43: public class LargeManifestOOMTest {
> 44:
> 45:
Unneeded blank lines
test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java line 83:
> 81: }
> 82: }
> 83:
Extra line
test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java line 78:
> 76: bw.write("OOM-Test: ");
> 77: for (long i = 0; i < 2147483648L; i++) {
> 78: bw.write("a");
As you probably noticed, this test takes a little while to run. One way to
speed it up a little would be to write more
characters at a time. While we're at it, we may as well make the Manifest
well-formed by breaking it into 72-byte
lines. See "Line length" under:
https://docs.oracle.com/en/java/javase/15/docs/specs/jar/jar.html#notes-on-manifest-and-signature-files
Just write enough lines to exceed Integer.MAX_VALUE bytes.
-
Changes requested by bchristi (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
On Thu, 24 Sep 2020 16:36:28 GMT, Brent Christian wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address the review comments and introduce an array size check in
>> JarFile.getBytes() method itself
>
> src/java.base/share/classes/java/util/jar/JarFile.java line 791:
>
>> 789: private byte[] getBytes(ZipEntry ze) throws IOException {
>> 790: try (InputStream is = super.getInputStream(ze)) {
>> 791: int len = (int)ze.getSize();
>
> I think the main issue is the casting of the 'long' value from
> ZipEntry.getSize() into an 'int'. I think checking if
> the size is > the maximum array size and throwing an OOME here might be a
> sufficient fix on its own.
Hello Brent,
Thank you for the review and sorry about the delayed response - I was involved
in a few other things so couldn't get to
this sooner.
I have taken your input and updated this patch to address this part.
Specifically, I have introduced a new
`MAX_BUFFER_SIZE` within the `JarFile`. This `MAX_BUFFER_SIZE` is an actual
copy of the field (and value) of
`java.io.InputStream#MAX_BUFFER_SIZE`. I have done a minor change to the
javadoc of this field as compared to what is
in the javadoc of its `InputStream` counterpart. I did this so that the OOM
exception message being thrown matches the
comment in this javadoc (the `InputStream` has a mismatch in its javadoc and
the actual message that gets thrown).
Additionally, in this patch, the `if (len != -1 ...)` has been changed to `if
(len >= 0 ...)` to prevent the code from
entering this block when `Zip64` format is involved where (from what I can
gather) an uncompressed entry size value can
have 2^64 (unsigned) bytes.
-
PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
> 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. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Address the review comments and introduce an array size check in JarFile.getBytes() method itself - Changes: - all: https://git.openjdk.java.net/jdk/pull/323/files - new: https://git.openjdk.java.net/jdk/pull/323/files/76dcea76..279c7c83 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=323=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=323=00-01 Stats: 17 lines in 1 file changed: 10 ins; 2 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/323.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/323/head:pull/323 PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException
On Wed, 23 Sep 2020 15:06:55 GMT, Jaikiran Pai 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.
Changes requested by bchristi (Reviewer).
src/java.base/share/classes/java/util/jar/JarFile.java line 791:
> 789: private byte[] getBytes(ZipEntry ze) throws IOException {
> 790: try (InputStream is = super.getInputStream(ze)) {
> 791: int len = (int)ze.getSize();
I think the main issue is the casting of the 'long' value from
ZipEntry.getSize() into an 'int'. I think checking if
the size is > the maximum array size and throwing an OOME here might be a
sufficient fix on its own.
-
PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException
Hello Brent, Thank you for sponsoring this change. In the meantime, I triggered the pre-submit GitHub action job to run the "tier1" tests for a duplicate branch of this PR. That completed successfully https://github.com/jaikiran/jdk/actions/runs/269960940. I'll wait for the reviews, before initiating any /integrate command. -Jaikiran On 23/09/20 10:21 pm, Brent Christian wrote: > On Wed, 23 Sep 2020 15:12:58 GMT, Jaikiran Pai 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 > Hi, Jaikiran. I can sponsor this change. > > - > > PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException
On Wed, 23 Sep 2020 15:12:58 GMT, Jaikiran Pai 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 Hi, Jaikiran. I can sponsor this change. - PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException
On Wed, 23 Sep 2020 15:06:55 GMT, Jaikiran Pai 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
