RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException)
Could you please review the JDK-8272746 bug fixes? Since the array index is of type int, the overflow occurs when the value of end.cenlen is too large because of too many entries. It is necessary to read a part of the CEN from the file to fix the problem fundamentally, but the way will require an extensive fix and degrade performance. In practical terms, the size of the central directory rarely grows that large. So, I fixed it to check the size of the central directory and throw an exception if it is too large. - Commit messages: - 8272746: ZipFile can't open big file (NegativeArraySizeException) Changes: https://git.openjdk.java.net/jdk/pull/6927/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6927&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272746 Stats: 80 lines in 2 files changed: 80 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6927.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6927/head:pull/6927 PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException)
On Thu, 23 Dec 2021 10:55:08 GMT, Masanori Yano wrote:
> Could you please review the JDK-8272746 bug fixes?
> Since the array index is of type int, the overflow occurs when the value of
> end.cenlen is too large because of too many entries.
> It is necessary to read a part of the CEN from the file to fix the problem
> fundamentally, but the way will require an extensive fix and degrade
> performance.
> In practical terms, the size of the central directory rarely grows that
> large. So, I fixed it to check the size of the central directory and throw an
> exception if it is too large.
src/java.base/share/classes/java/util/zip/ZipFile.java line 1501:
> 1499: // read in the CEN and END
> 1500: if (end.cenlen + ENDHDR >= Integer.MAX_VALUE) {
> 1501: zerror("invalid END header (too large central
> directory size)");
This check looks correct. It might be a bit clearer to say that "central
directory size too large" rather than "too large central directory size".
The bug report says that JDK 8 and the native zip handle these zip files, were
you able to check that?
-
PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v2]
On Thu, 23 Dec 2021 16:42:50 GMT, Alan Bateman wrote:
>> Masanori Yano has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8272746: ZipFile can't open big file (NegativeArraySizeException)
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1501:
>
>> 1499: // read in the CEN and END
>> 1500: if (end.cenlen + ENDHDR >= Integer.MAX_VALUE) {
>> 1501: zerror("invalid END header (too large central
>> directory size)");
>
> This check looks correct. It might be a bit clearer to say that "central
> directory size too large" rather than "too large central directory size".
>
> The bug report says that JDK 8 and the native zip handle these zip files,
> were you able to check that?
I will correct the message as you pointed out.
I checked the JDK8 and Linux unzip commands and found no exceptions or errors.
Since JDK9, [JDK-8145260](https://bugs.openjdk.java.net/browse/JDK-8145260) has
been modified to significantly change the way zip files are handled, resulting
in a different result than jdk8.
-
PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v2]
> Could you please review the JDK-8272746 bug fixes? > Since the array index is of type int, the overflow occurs when the value of > end.cenlen is too large because of too many entries. > It is necessary to read a part of the CEN from the file to fix the problem > fundamentally, but the way will require an extensive fix and degrade > performance. > In practical terms, the size of the central directory rarely grows that > large. So, I fixed it to check the size of the central directory and throw an > exception if it is too large. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8272746: ZipFile can't open big file (NegativeArraySizeException) - Changes: - all: https://git.openjdk.java.net/jdk/pull/6927/files - new: https://git.openjdk.java.net/jdk/pull/6927/files/a85ef0f5..c54c50eb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6927&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6927&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6927.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6927/head:pull/6927 PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v2]
On Thu, 23 Dec 2021 16:42:50 GMT, Alan Bateman wrote:
>> Masanori Yano has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8272746: ZipFile can't open big file (NegativeArraySizeException)
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1501:
>
>> 1499: // read in the CEN and END
>> 1500: if (end.cenlen + ENDHDR >= Integer.MAX_VALUE) {
>> 1501: zerror("invalid END header (too large central
>> directory size)");
>
> This check looks correct. It might be a bit clearer to say that "central
> directory size too large" rather than "too large central directory size".
>
> The bug report says that JDK 8 and the native zip handle these zip files,
> were you able to check that?
@AlanBateman Could you please review the above comments.
-
PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v2]
On Thu, 23 Dec 2021 16:42:50 GMT, Alan Bateman wrote:
>> Masanori Yano has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8272746: ZipFile can't open big file (NegativeArraySizeException)
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1501:
>
>> 1499: // read in the CEN and END
>> 1500: if (end.cenlen + ENDHDR >= Integer.MAX_VALUE) {
>> 1501: zerror("invalid END header (too large central
>> directory size)");
>
> This check looks correct. It might be a bit clearer to say that "central
> directory size too large" rather than "too large central directory size".
>
> The bug report says that JDK 8 and the native zip handle these zip files,
> were you able to check that?
> @AlanBateman Could you please review the above comments.
Thanks for changing the message, the update to ZipFile looks good to me
although I'm still surprised that the native tools support a CEN larger than
2Gb.
I'm slow to add myself as a Reviewer because I'm concerned about the test. I
ran it on a couple of systems and it take several minutes to create the ZIP
file. This test may potentially run concurrently with other I/O bound tests and
I'm concerned about intermittent timeouts. @LanceAndersen - do you have any
opinions on this?
-
PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v2]
On Thu, 6 Jan 2022 09:34:24 GMT, Alan Bateman wrote: > > @AlanBateman Could you please review the above comments. > > Thanks for changing the message, the update to ZipFile looks good to me > although I'm still surprised that the native tools support a CEN larger than > 2Gb. > > I'm slow to add myself as a Reviewer because I'm concerned about the test. I > ran it on a couple of systems and it take several minutes to create the ZIP > file. This test may potentially run concurrently with other I/O bound tests > and I'm concerned about intermittent timeouts. @LanceAndersen - do you have > any opinions on this? I haven't had a chance to look at this yet but will kick off some runs to see what the runtime via mach5. We might have issues with some of the test machines. > > @AlanBateman Could you please review the above comments. > > Thanks for changing the message, the update to ZipFile looks good to me > although I'm still surprised that the native tools support a CEN larger than > 2Gb. > > I'm slow to add myself as a Reviewer because I'm concerned about the test. I > ran it on a couple of systems and it take several minutes to create the ZIP > file. This test may potentially run concurrently with other I/O bound tests > and I'm concerned about intermittent timeouts. @LanceAndersen - do you have > any opinions on this? I have not had a chance to look at this yet in detail but in the meantime I will kick off a mach5 run to see what the runtime is on our test machines today A couple quick passing comments.The test should delete the file after the run/error given the potential size. Was there a reason you did not the test is not TestNG based? - PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v2]
On Fri, 24 Dec 2021 11:07:04 GMT, Masanori Yano wrote:
>> Could you please review the JDK-8272746 bug fixes?
>> Since the array index is of type int, the overflow occurs when the value of
>> end.cenlen is too large because of too many entries.
>> It is necessary to read a part of the CEN from the file to fix the problem
>> fundamentally, but the way will require an extensive fix and degrade
>> performance.
>> In practical terms, the size of the central directory rarely grows that
>> large. So, I fixed it to check the size of the central directory and throw
>> an exception if it is too large.
>
> Masanori Yano has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8272746: ZipFile can't open big file (NegativeArraySizeException)
Thank you for your work here. Have you also run your test using Apache
Commons?
I have run this test over 2000 times via Mach5 on various hardware with some
runs taking over 12 minutes for the test to complete. So unless you can
refactor the test to be more efficient, we need to make the test a manual test
as this is more of a corner case.
Please see the additional comments below
test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java line 28:
> 26: * @summary ZipFile can't open big file (NegativeArraySizeException)
> 27: * @requires (sun.arch.data.model == "64" & os.maxMemory > 8g)
> 28: * @run main/othervm/timeout=3600 -Xmx8g TestTooManyEntries
Please convert to a manual test and use TestNG for the test
test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java line 58:
> 56: if (System.currentTimeMillis() >= nextLog) {
> 57: nextLog = 30_000 + System.currentTimeMillis();
> 58: System.out.printf("Processed %s%% directories (%s),
> file size is %sMb (%ss)%n",
I would clarify the message to use "entries" vs "directories". as the issue
deals with the size of the CEN overall not whether the actual entry is for a
file or represents a directory.
You could probably simplify the time check by just outputting every "n"
entries written
test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java line 70:
> 68: ZipFile zip = new ZipFile(hugeZipFile);
> 69: } catch (ZipException ze) {
> 70: passed = true;
you can use assertThrows here
-
Changes requested by lancea (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v3]
> Could you please review the JDK-8272746 bug fixes? > Since the array index is of type int, the overflow occurs when the value of > end.cenlen is too large because of too many entries. > It is necessary to read a part of the CEN from the file to fix the problem > fundamentally, but the way will require an extensive fix and degrade > performance. > In practical terms, the size of the central directory rarely grows that > large. So, I fixed it to check the size of the central directory and throw an > exception if it is too large. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8272746: ZipFile can't open big file (NegativeArraySizeException) - Changes: - all: https://git.openjdk.java.net/jdk/pull/6927/files - new: https://git.openjdk.java.net/jdk/pull/6927/files/c54c50eb..621f1a68 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6927&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6927&range=01-02 Stats: 33 lines in 1 file changed: 10 ins; 9 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/6927.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6927/head:pull/6927 PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v2]
On Sat, 8 Jan 2022 16:09:59 GMT, Lance Andersen wrote: >> Masanori Yano has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8272746: ZipFile can't open big file (NegativeArraySizeException) > > Thank you for your work here. Have you also run your test using Apache > Commons? > > I have run this test over 2000 times via Mach5 on various hardware with some > runs taking over 12 minutes for the test to complete. So unless you can > refactor the test to be more efficient, we need to make the test a manual > test as this is more of a corner case. > > Please see the additional comments below @LanceAndersen Thank you for your detailed comments. I converted the test to use TestNG as you pointed out. Could you please review it again? - PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v3]
On Fri, 14 Jan 2022 11:05:06 GMT, Masanori Yano wrote: >> Could you please review the JDK-8272746 bug fixes? >> Since the array index is of type int, the overflow occurs when the value of >> end.cenlen is too large because of too many entries. >> It is necessary to read a part of the CEN from the file to fix the problem >> fundamentally, but the way will require an extensive fix and degrade >> performance. >> In practical terms, the size of the central directory rarely grows that >> large. So, I fixed it to check the size of the central directory and throw >> an exception if it is too large. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8272746: ZipFile can't open big file (NegativeArraySizeException) Thank you for making the changes. Overall it looks much better Please add comments describing your constants as well as a comment describing the intent of your setup and test methods - PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v4]
> Could you please review the JDK-8272746 bug fixes? > Since the array index is of type int, the overflow occurs when the value of > end.cenlen is too large because of too many entries. > It is necessary to read a part of the CEN from the file to fix the problem > fundamentally, but the way will require an extensive fix and degrade > performance. > In practical terms, the size of the central directory rarely grows that > large. So, I fixed it to check the size of the central directory and throw an > exception if it is too large. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8272746: ZipFile can't open big file (NegativeArraySizeException) - Changes: - all: https://git.openjdk.java.net/jdk/pull/6927/files - new: https://git.openjdk.java.net/jdk/pull/6927/files/621f1a68..9ebb2a7a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6927&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6927&range=02-03 Stats: 14 lines in 1 file changed: 12 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6927.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6927/head:pull/6927 PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v3]
On Fri, 14 Jan 2022 23:55:58 GMT, Lance Andersen wrote: >> Masanori Yano has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8272746: ZipFile can't open big file (NegativeArraySizeException) > > Thank you for making the changes. Overall it looks much better > > Please add comments describing your constants as well as a comment describing > the intent of your setup and test methods @LanceAndersen As you pointed out, I described the explanation of the test in the comment. Is there anything else I should fix? - PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v4]
On Mon, 17 Jan 2022 09:56:13 GMT, Masanori Yano wrote: >> Could you please review the JDK-8272746 bug fixes? >> Since the array index is of type int, the overflow occurs when the value of >> end.cenlen is too large because of too many entries. >> It is necessary to read a part of the CEN from the file to fix the problem >> fundamentally, but the way will require an extensive fix and degrade >> performance. >> In practical terms, the size of the central directory rarely grows that >> large. So, I fixed it to check the size of the central directory and throw >> an exception if it is too large. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8272746: ZipFile can't open big file (NegativeArraySizeException) Looks fine. One minor comment update but you can integrate after making the change test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java line 82: > 80: > 81: /** > 82: * Validates that an appropriate exception is thrown when the ZipFile > class Please change "appropriate" to "ZipException" - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v5]
> Could you please review the JDK-8272746 bug fixes? > Since the array index is of type int, the overflow occurs when the value of > end.cenlen is too large because of too many entries. > It is necessary to read a part of the CEN from the file to fix the problem > fundamentally, but the way will require an extensive fix and degrade > performance. > In practical terms, the size of the central directory rarely grows that > large. So, I fixed it to check the size of the central directory and throw an > exception if it is too large. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8272746: ZipFile can't open big file (NegativeArraySizeException) - Changes: - all: https://git.openjdk.java.net/jdk/pull/6927/files - new: https://git.openjdk.java.net/jdk/pull/6927/files/9ebb2a7a..c6b53732 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6927&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6927&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6927.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6927/head:pull/6927 PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v4]
On Mon, 17 Jan 2022 18:10:02 GMT, Lance Andersen wrote: >> Masanori Yano has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8272746: ZipFile can't open big file (NegativeArraySizeException) > > test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java line 82: > >> 80: >> 81: /** >> 82: * Validates that an appropriate exception is thrown when the >> ZipFile class > > Please change "appropriate" to "ZipException" Thank you very much. I fixed it. - PR: https://git.openjdk.java.net/jdk/pull/6927
