RFR: 8304013: Add a fast, non-manual alternative to test/jdk/java/util/zip/ZipFile/TestTooManyEntries

[Lance Andersen]

On Thu, 26 Jan 2023 18:49:47 GMT, Eirik Bjorsnos <duke at 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