RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files

[Eirik Bjorsnos]

On Mon, 20 Feb 2023 11:28:29 GMT, Lance Andersen <lancea at openjdk.org> wrote:

>> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the number of compressed or uncompressed bytes read from the inflater is larger than the Zip64 magic value.
>> 
>> While the ZIP format  mandates that the data descriptor `SHOULD be stored in ZIP64 format (as 8 byte values) when a file's size exceeds 0xFFFFFFFF`, it also states that `ZIP64 format MAY be used regardless of the size of a file`. For such small entries, the above assumption does not hold.
>> 
>> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the ZipEntry includes a Zip64 extra information field. This brings ZipInputStream into alignment with the APPNOTE format spec:
>> 
>> 
>> When extracting, if the zip64 extended information extra 
>> field is present for the file the compressed and 
>> uncompressed sizes will be 8 byte values.
>> 
>> 
>> While small Zip64 files with 8-byte data descriptors are not commonly found in the wild, it is possible to create one using the Info-ZIP command line `-fd` flag:
>> 
>> `echo hello | zip -fd > hello.zip`
>> 
>> The PR also adds a test verifying that such a small Zip64 file can be parsed by ZipInputStream.
>
> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 578:
> 
>> 576:         if ((flag & 8) == 8) {
>> 577:             /* "Data Descriptor" present */
>> 578:             if (hasZip64Extra(e)
> 
> A comment would be useful here as well

Added a short comment here, although I'm not fully convinced of the usefulness.

> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 627:
> 
>> 625:         }
>> 626:     }
>> 627:     // Returns true if the ZipEntry has a ZIP64 extended information extra field
> 
> Please add a comment which clarifies the purpose of this method(such as what you are traversing...etc) and cite the section of the APP.NOTE that is being referenced for future maintainers (similar to the description you created)

Added a quite extensive explainer in the Javadocs of this method, including a citation of the relevant APPNOTE.txt spec.

> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 640:
> 
>> 638:                     break; // Invalid size
>> 639:                 }
>> 640:                 i += size + (2 * Short.BYTES);
> 
> Probably  could assign ` 2 * Short.BYTES ` to a variable or create a constant.

Extracted to a variable with a comment, also replaced the constant 4 earlier in the method.

> test/jdk/java/util/zip/ZipInputStream/Zip64DataDescriptor.java line 50:
> 
>> 48: public class Zip64DataDescriptor {
>> 49: 
>> 50:     private byte[] zip;
> 
> Please add a comment to the purpose of the field

Comment added.

> test/jdk/java/util/zip/ZipInputStream/Zip64DataDescriptor.java line 89:
> 
>> 87:                 0000b20000000000000001000000504b050600000000010001005c000000
>> 88:                 560000000000""";
>> 89: 
> 
> It would be useful to describe how the Zip was created in case it needs to be recreated at a future date

This Zip was produced using my ZIP stream transformation library. This is not in the JDK, so probably not so useful to reference. Let me see if I can rework the test to use a Zip created using the `zip` streaming mode I described instead, that would allow a more independent reproduction.

> test/jdk/java/util/zip/ZipInputStream/Zip64DataDescriptor.java line 140:
> 
>> 138:     }
>> 139: 
>> 140:     private void setExtraSize(short invalidSize) {
> 
> Please add a comment describing the method (and any others without).  As we add new tests, we are trying to. follow this practice the best that we can.

Added comments for these methods and also for the zip field which I renamed to zip64Zip.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1120734693
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1120734271
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1120733599
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1120736858
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1120736283
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1120736656