RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]

[Brent Christian]

On Tue, 29 Sep 2020 11:39:20 GMT, Jaikiran Pai <jpai at 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.
>
> 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