Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream #17

Closed
wants to merge 6 commits into from

Conversation

apavlyutkin
Copy link
Contributor

@apavlyutkin apavlyutkin commented Mar 18, 2022

Hi!

Please review the backport of JDK-8190753 to 8u. The patch fixes IllegalArgumentException that takes place on accessing large (>2^31 bytes) entries in zipfs.

The original patch

openjdk/jdk11u-dev#545

was applied with the following changes:

  • changes to absent DeflatingEntryOutputStream class were skipped (in 8u EntryOutputStream handles both RAW and deflating streaming);
  • the tests were updated to not use unsupported APIs like Path.of() and Map.of()
  • timeout value for ZipFSOutputStreamTest was increased because the test is too greedy and may fail by timeout on low performance hosts

The changes were previously reviewed: https://mail.openjdk.java.net/pipermail/jdk8u-dev/2021-December/014479.html

Verification/regression: jdk_other (20.04/amd64)


Progress

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

Issue

  • JDK-8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk8u-dev pull/17/head:pull/17
$ git checkout pull/17

Update a local copy of the PR:
$ git checkout pull/17
$ git pull https://git.openjdk.java.net/jdk8u-dev pull/17/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17

View PR using the GUI difftool:
$ git pr show -t 17

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk8u-dev/pull/17.diff

…egative initial size for ByteArrayOutputStream

Backport-of: 8a9cda2d84513ab49a54e1d2a7b530f0bae05c61
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 18, 2022

👋 Welcome back apavlyutkin! 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.

@openjdk openjdk bot changed the title 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a n… 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream Mar 18, 2022
@openjdk openjdk bot added the rfr label Mar 18, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 18, 2022

Webrevs

@apavlyutkin apavlyutkin changed the title 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream Backport 8a9cda2d84513ab49a54e1d2a7b530f0bae05c61 Mar 18, 2022
@openjdk openjdk bot changed the title Backport 8a9cda2d84513ab49a54e1d2a7b530f0bae05c61 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream Mar 18, 2022
@openjdk
Copy link

openjdk bot commented Mar 18, 2022

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added the backport label Mar 18, 2022
Copy link
Member

@phohensee phohensee left a comment

Lgtm.

@openjdk
Copy link

openjdk bot commented Mar 18, 2022

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

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

8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream

Reviewed-by: phh, andrew

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 15 new commits pushed to the master branch:

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 (@phohensee, @gnu-andrew) 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).

@openjdk openjdk bot added the ready label Mar 18, 2022
@gnu-andrew
Copy link
Member

gnu-andrew commented Apr 5, 2022

Sorry for the delay on sorting this one out. It seems to have been bounced across all three repositories during the Git transition.

It now looks correct to me from a technical perspective. However, I am seeing some very odd whitespace differences between the two new test files when compared with the versions in 11u (either from the patch or the 11u-dev repository) e.g

-  @AfterMethod
+    @AfterMethod
     public void tearDown() throws IOException {
-    deleteFiles();
-  }
+        deleteFiles();
+    }

I don't know how this has occurred - I don't recall it being present in the original review - but I think the simplest way to fix it would be to store a diff between your 8u versions and the 11u versions using -b, replace the 8u ones with the ones from 11u, and then reapply your changes for 8u from the diff i.e. something like

$ diff -ub ../jdk11u-dev/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java jdk/test/demo/zipfs/ZipFSOutputStreamTest.java  > /tmp/test1.patch
$ cp ../jdk11u-dev/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java jdk/test/demo/zipfs
$ <edit test1.patch to remove any remaining unneeded whitespace changes>
$ patch -Np0 < /tmp/test1.patch

@apavlyutkin
Copy link
Contributor Author

apavlyutkin commented Apr 5, 2022

Sorry for the delay on sorting this one out. It seems to have been bounced across all three repositories during the Git transition.

It now looks correct to me from a technical perspective. However, I am seeing some very odd whitespace differences between the two new test files when compared with the versions in 11u (either from the patch or the 11u-dev repository) e.g

-  @AfterMethod
+    @AfterMethod
     public void tearDown() throws IOException {
-    deleteFiles();
-  }
+        deleteFiles();
+    }

I don't know how this has occurred - I don't recall it being present in the original review - but I think the simplest way to fix it would be to store a diff between your 8u versions and the 11u versions using -b, replace the 8u ones with the ones from 11u, and then reapply your changes for 8u from the diff i.e. something like

$ diff -ub ../jdk11u-dev/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java jdk/test/demo/zipfs/ZipFSOutputStreamTest.java  > /tmp/test1.patch
$ cp ../jdk11u-dev/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java jdk/test/demo/zipfs
$ <edit test1.patch to remove any remaining unneeded whitespace changes>
$ patch -Np0 < /tmp/test1.patch

fixed

Copy link
Member

@gnu-andrew gnu-andrew left a comment

This version looks a lot better. There do still seem to be a few odd differences that have crept in, when compared to the patch in 11u, which I've noted in my review. These should be quick to fix, then we can get this in.

jdk/test/demo/zipfs/LargeCompressedEntrySizeTest.java Outdated Show resolved Hide resolved
jdk/test/demo/zipfs/ZipFSOutputStreamTest.java Outdated Show resolved Hide resolved
jdk/test/demo/zipfs/ZipFSOutputStreamTest.java Outdated Show resolved Hide resolved
jdk/test/demo/zipfs/ZipFSOutputStreamTest.java Outdated Show resolved Hide resolved
Copy link
Member

@gnu-andrew gnu-andrew left a comment

Brilliant, let's finally get this in.

@gnu-andrew
Copy link
Member

gnu-andrew commented Apr 8, 2022

I've added jdk8u-fix-yes to the bug.

@apavlyutkin
Copy link
Contributor Author

apavlyutkin commented Apr 8, 2022

/integrate

@openjdk openjdk bot added the sponsor label Apr 8, 2022
@openjdk
Copy link

openjdk bot commented Apr 8, 2022

@apavlyutkin
Your change (at version 8911a60) is now ready to be sponsored by a Committer.

@yan-too
Copy link
Contributor

yan-too commented Apr 8, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Apr 8, 2022

Going to push as commit 3647d98.
Since your change was applied there have been 15 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Apr 8, 2022
@openjdk openjdk bot closed this Apr 8, 2022
@openjdk openjdk bot removed the ready label Apr 8, 2022
@openjdk openjdk bot removed rfr sponsor labels Apr 8, 2022
@openjdk
Copy link

openjdk bot commented Apr 8, 2022

@yan-too @apavlyutkin Pushed as commit 3647d98.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated
4 participants