On Tue, 29 Sep 2020 11:39:20 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. > > 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
