On Mon, 20 Feb 2023 11:28:29 GMT, Lance Andersen <lan...@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
