On Wed, 30 Sep 2020 17:21:14 GMT, Brent Christian <bchri...@openjdk.org> 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
