8242882: opening jar file with large manifest might throw NegativeArraySizeException #323

jaikiran · 2020-09-23T15:06:55Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8242882: opening jar file with large manifest might throw NegativeArraySizeException

Reviewers

Brent Christian (@bchristi-git - Reviewer)
Lance Andersen (@LanceAndersen - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/323/head:pull/323
$ git checkout pull/323

…aySizeException Reviewed-By:

bridgekeeper · 2020-09-23T15:08:32Z

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

jaikiran · 2020-09-23T15:09:44Z

test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java

+        final JarFile jar = new JarFile(jarFilePath.toFile());
+        final OutOfMemoryError oome = Assert.expectThrows(OutOfMemoryError.class, () -> jar.getManifest());
+        // additionally verify that the OOM was for the right/expected reason
+        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.

Hello Lance,
I decided to remove the assertion on the exception message. I have updated the PR accordingly.

jaikiran · 2020-09-23T15:12:58Z

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

openjdk · 2020-09-23T15:36:43Z

@jaikiran The following labels will be automatically applied to this pull request: core-libs security.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

mlbridge · 2020-09-23T15:42:03Z

Webrevs

bchristi-git · 2020-09-23T16:49:19Z

Hi, Jaikiran. I can sponsor this change.

mlbridge · 2020-09-24T07:07:07Z

Mailing list message from Jaikiran Pai on security-dev:

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:

bchristi-git · 2020-09-24T16:36:28Z

src/java.base/share/classes/java/util/jar/JarFile.java

@@ -791,8 +791,10 @@ private void initializeVerifier() {
            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.

…ile.getBytes() method itself

bchristi-git

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.

bchristi-git · 2020-09-30T17:21:14Z

src/java.base/share/classes/java/util/jar/JarFile.java

+    /**
+     * The maximum size of array to allocate.
+     * Some VMs reserve some header words in an array.
+     * Attempts to allocate larger arrays may result in
+     * OutOfMemoryError: Required array size too large
+     */
+    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.

bchristi-git · 2020-09-30T17:23:10Z

src/java.base/share/classes/java/util/jar/JarFile.java

-            int len = (int)ze.getSize();
+            long len = ze.getSize();
+            if (len > MAX_BUFFER_SIZE) {
+                throw new OutOfMemoryError("Required array size too large");
+            }


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.

bchristi-git · 2020-09-30T17:24:23Z

test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java

+
+

Unneeded blank lines

bchristi-git · 2020-09-30T17:26:18Z

test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java

+        final JarFile jar = new JarFile(jarFilePath.toFile());
+        final OutOfMemoryError oome = Assert.expectThrows(OutOfMemoryError.class, () -> jar.getManifest());
+        // additionally verify that the OOM was for the right/expected reason
+        if (!"Required array size too large".equals(oome.getMessage())) {


I think it's fine either way.

bchristi-git · 2020-09-30T17:26:27Z

test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java

+            bw.newLine();
+        }
+    }
+


bchristi-git · 2020-09-30T17:29:31Z

test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java

+            bw.write("OOM-Test: ");
+            for (long i = 0; i < 2147483648L; i++) {
+                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.

Thank you for the review Brent.

jaikiran · 2020-10-08T03:30:42Z

Hello Lance, does the latest state of this PR look fine to you? If so, shall I trigger a integrate?

openjdk · 2020-10-08T06:51:06Z

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8242882: opening jar file with large manifest might throw NegativeArraySizeException

Reviewed-by: bchristi, lancea

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 204 new commits pushed to the master branch:

f860372: 8253566: clazz.isAssignableFrom will return false for interface implementors
66f27b5: 8254015: copy_to_survivor_space should use in-hand klass for scanning
76a5852: 8253756: C2 CompilerThread0 crash in Node::add_req(Node*)
8f9e479: 8254144: Non-x86 Zero builds fail with return-type warning in os_linux_zero.cpp
7952c06: 8254166: Zero: return-type warning in zeroInterpreter_zero.cpp
894ec76: 8254027: gc/g1/TestHumongousConcurrentStartUndo.java failed with "'Concurrent Mark Cycle' missing from stdout/stderr"
bc23690: 8254173: Add Zero, Minimal hotspot targets to submit workflow
e1187c4: 8248262: Wrong link target in ModuleDescriptor#isAutomatic's API documentation
9cdfd0f: 8254096: remove jdk.test.lib.Utils::getMandatoryProperty(String) method
d1e94ee: 8253909: Implement detailed map file for CDS
... and 194 more: https://git.openjdk.java.net/jdk/compare/812b39f574326406fbbb891fc6bca7947465ff4e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@bchristi-git, @LanceAndersen) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

LanceAndersen

Hi Jaikiran,

Yes I think you are OK to move forward with the integration,

jaikiran · 2020-10-08T10:43:21Z

/integrate

openjdk · 2020-10-08T10:44:15Z

@jaikiran
Your change (at version a011b0d) is now ready to be sponsored by a Committer.

LanceAndersen · 2020-10-08T10:46:04Z

/sponsor

openjdk · 2020-10-08T10:46:57Z

@LanceAndersen @jaikiran Since your change was applied there have been 204 commits pushed to the master branch:

f860372: 8253566: clazz.isAssignableFrom will return false for interface implementors
66f27b5: 8254015: copy_to_survivor_space should use in-hand klass for scanning
76a5852: 8253756: C2 CompilerThread0 crash in Node::add_req(Node*)
8f9e479: 8254144: Non-x86 Zero builds fail with return-type warning in os_linux_zero.cpp
7952c06: 8254166: Zero: return-type warning in zeroInterpreter_zero.cpp
894ec76: 8254027: gc/g1/TestHumongousConcurrentStartUndo.java failed with "'Concurrent Mark Cycle' missing from stdout/stderr"
bc23690: 8254173: Add Zero, Minimal hotspot targets to submit workflow
e1187c4: 8248262: Wrong link target in ModuleDescriptor#isAutomatic's API documentation
9cdfd0f: 8254096: remove jdk.test.lib.Utils::getMandatoryProperty(String) method
d1e94ee: 8253909: Implement detailed map file for CDS
... and 194 more: https://git.openjdk.java.net/jdk/compare/812b39f574326406fbbb891fc6bca7947465ff4e...master

Your commit was automatically rebased without conflicts.

Pushed as commit 782d45b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8242882: opening jar file with large manifest might throw NegativeArr…

76dcea7

…aySizeException Reviewed-By:

jaikiran commented Sep 23, 2020

View reviewed changes

openjdk bot added the rfr Pull request is ready for review label Sep 23, 2020

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Sep 23, 2020

bchristi-git suggested changes Sep 24, 2020

View reviewed changes

Address the review comments and introduce an array size check in JarF…

279c7c8

…ile.getBytes() method itself

bchristi-git suggested changes Sep 30, 2020

View reviewed changes

Second round of review comments addressed

a011b0d

bchristi-git approved these changes Oct 7, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 8, 2020

LanceAndersen approved these changes Oct 8, 2020

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 8, 2020

openjdk bot closed this Oct 8, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 8, 2020

jaikiran deleted the 8242882 branch August 11, 2021 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8242882: opening jar file with large manifest might throw NegativeArraySizeException #323

8242882: opening jar file with large manifest might throw NegativeArraySizeException #323

jaikiran commented Sep 23, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 23, 2020

jaikiran Sep 23, 2020

bchristi-git Sep 30, 2020

LanceAndersen Sep 30, 2020

jaikiran Oct 1, 2020

jaikiran commented Sep 23, 2020

openjdk bot commented Sep 23, 2020

mlbridge bot commented Sep 23, 2020 •

edited

Loading

bchristi-git commented Sep 23, 2020

mlbridge bot commented Sep 24, 2020

bchristi-git Sep 24, 2020

jaikiran Sep 29, 2020

bchristi-git left a comment

bchristi-git Sep 30, 2020 •

edited

Loading

jaikiran Oct 1, 2020

bchristi-git Sep 30, 2020

jaikiran Oct 1, 2020

bchristi-git Sep 30, 2020

bchristi-git Sep 30, 2020

bchristi-git Sep 30, 2020

bchristi-git Sep 30, 2020

jaikiran Oct 1, 2020

bchristi-git Oct 7, 2020

jaikiran Oct 8, 2020

jaikiran commented Oct 8, 2020

openjdk bot commented Oct 8, 2020 •

edited

Loading

LanceAndersen left a comment

jaikiran commented Oct 8, 2020

openjdk bot commented Oct 8, 2020

LanceAndersen commented Oct 8, 2020

openjdk bot commented Oct 8, 2020

		@@ -791,8 +791,10 @@ private void initializeVerifier() {
		int len = (int)ze.getSize();

8242882: opening jar file with large manifest might throw NegativeArraySizeException #323

8242882: opening jar file with large manifest might throw NegativeArraySizeException #323

Conversation

jaikiran commented Sep 23, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaikiran commented Sep 23, 2020

openjdk bot commented Sep 23, 2020

mlbridge bot commented Sep 23, 2020 • edited Loading

Webrevs

bchristi-git commented Sep 23, 2020

mlbridge bot commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bchristi-git left a comment

Choose a reason for hiding this comment

bchristi-git Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaikiran commented Oct 8, 2020

openjdk bot commented Oct 8, 2020 • edited Loading

LanceAndersen left a comment

Choose a reason for hiding this comment

jaikiran commented Oct 8, 2020

openjdk bot commented Oct 8, 2020

LanceAndersen commented Oct 8, 2020

openjdk bot commented Oct 8, 2020

jaikiran commented Sep 23, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 23, 2020 •

edited

Loading

bchristi-git Sep 30, 2020 •

edited

Loading

openjdk bot commented Oct 8, 2020 •

edited

Loading