On Thu, 26 Jan 2023 18:49:47 GMT, Eirik Bjorsnos <d...@openjdk.org> wrote:
> The TestTooManyEntries test was originally added to validate that ZIP64 files
> with CEN sizes exceeding what ZipFile supports are rejected with a
> ZipException. The test does this by creating a large ZIP file (several
> gigabytes) with many enties. Because this is resource intensive, the test is
> currently tagged as manual. (See #6927)
>
> It would be useful to have a test which asserts the CEN size enforcement, but
> without the CPU, disk, memory and run time requirements of
> TestTooManyEntries. Such a fast test can run non-manual, without the
> @requires and manual tags as found in TestTooManyEntries.
>
> This PR adds the EndOfCenValidation test which creates sparse test ZIPs where
> the CEN is "inflated" such that is matches the size declared in the "End of
> central directory" records.
>
> While thee sparse files look large, they consume very little disk space on
> file systems supporting sparse files:
>
>
> 16 -rw-r--r-- 1 2147483702 Feb 6 18:54 bad-cen-offset.zip
> 16 -rw-r--r-- 1 2147483703 Feb 6 18:54 cen-size-too-large.zip
> 8 -rw-r--r-- 1 132 Feb 6 18:54 invalid-zen-size.zip
> ```
>
> For good measure, two new test methods are added to excercise the remaining
> ZipExceptions which ZipFile may throw during validation of the END record .
Thanks for the updates.
Please see the additional suggestions below
I think the changes are are looking good. Please see the comments below.
thank you again for your efforts here
test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 47:
> 45: import static org.testng.Assert.fail;
> 46:
> 47: public class CenSizeTooLarge {
I would add a comment which also points to the original test
test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 52:
> 50: private static final int CEN_SIZE_OFFSET = 12; // Offset of CEN size
> field within ENDHDR
> 51: private Path huge; // ZIP file with CEN size exceeding limit
> 52: private Path big; // ZIP file with CEN size exactly on the limit
You might consider more meaningful names for the Paths
test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 92:
> 90: private void assertRejected(Path zip, String expectedMsg) throws
> IOException {
> 91: try (ZipFile zf = new ZipFile(zip.toFile())) {
> 92: fail("Expected ZipFile to throw ZipException");
You could consider using expectThrows and then validation the value of
getMessage()
test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 107:
> 105: * to test that the large CEN size is rejected
> 106: */
> 107: private Path zipWithCenSize(String name, int cenSize) throws
> IOException {
You could speed this up a bit more by just creating the array representing the
original Zip 1 time and modify it each pass.(though it might not speed things
up that much)
test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java line 113:
> 111: try (ZipFile zf = new ZipFile(zip.toFile())) {
> 112: }
> 113: });
You can omit the try block
test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java line 133:
> 131: }
> 132: });
> 133:
Same comment as above.
test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java line 177:
> 175: Path zip) throws IOException {
> 176:
> 177: Files.deleteIfExists(zip);
Is this needed? I think I would just call the cleanup method from the setup
method
test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java line 219:
> 217: }
> 218:
> 219: // Produce a byte array of a ZIP with a single entry
Minor nit, I would use the same comment format for the methods as above and
probably add an @throws as needed so javadoc tag checks won't fail
test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java line 223:
> 221: ByteArrayOutputStream bout = new ByteArrayOutputStream();
> 222: try (ZipOutputStream zout = new ZipOutputStream(bout)) {
> 223: zout.putNextEntry(new ZipEntry("duke.txt"));
Perhaps clarify why you are not writing out any data...
-------------
PR Review: https://git.openjdk.org/jdk/pull/12231#pullrequestreview-1277002825
PR Review: https://git.openjdk.org/jdk/pull/12231#pullrequestreview-1355381891
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1091806763
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1091806173
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1091817123
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1091805101
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1146689004
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1146700985
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1146704271
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1146702440
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1146705913
