RFR: 8303972: (zipfs) Make test/jdk/jdk/nio/zipfs/TestLocOffsetFromZip64EF.java independent of the zip command line

[Lance Andersen]

On Fri, 10 Mar 2023 15:10:29 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

> Please review this PR which brings the currenly problem-listed test TestLocOffsetFromZip64EF back to life.
> 
> Instead of calling out to the `zip` command line, we produce a small-sized Zip64 file in the test itself. This file has the features  required to reproduce the ZipFileSystem issue, namely that the 'INFO-ZIP extended timestamp' field must come before  the 'Zip64 extended information'  field. (This would casue ZipFileSystem to read the LOC at position 0xFFFFFFFF).
> 
> This speed up the test (from 50s to 3s on my Macbook Pro), saves 4GB disk space during builds removes a dependency on the `zip` command in OS/distros.
> 
> Seee [JDK-8301183](https://bugs.openjdk.org/browse/JDK-8301183) for details on the problem-listing.

This test was leveraging a file created by info-zip intentionally so care needs to be taken to ensure that the zip that is being created is similar LOC and CEN output

Thank you for your work here Eirik, I think we are close.

I think I would include an additional entry in the zip just as an extra sanity check that we can navigate an additional entry

It is also worth noting that the LOC will still contain an extra field with the unknown tag of 0x9902 while the CEN is being changed to use Zip64, 0x1

A few more minor comments below.

I ran the current version of the test and it runs quickly on all platforms via mach5

Best
Lance

I think this looks good overall.   Please see the comment below and then I can run the test one more time and you should be good to go

test/jdk/jdk/nio/zipfs/TestLocOffsetFromZip64EF.java line 173:

> 171:             ByteBuffer buffer = ByteBuffer.wrap(zip64).order(ByteOrder.LITTLE_ENDIAN);
> 172:             buffer.putShort(UNKNOWN_TAG); // Opaque tag makes ZipEntry.setExtra ignore it
> 173:             buffer.putShort((short) (zip64.length - 2 * Short.BYTES)); // Data size

The length of the zip64 extra field is going to be 28.   I would suggest making this a bit clearer and adding a field/constant which is set to 28 for clarity to make it easier for those who might look at this code later

test/jdk/jdk/nio/zipfs/TestLocOffsetFromZip64EF.java line 191:

> 189:             e.setExtra(zip64);
> 190: 
> 191:             zo.finish(); // Write out CEN and END records

I might add one more entry after this one, perhaps with a compression method of DEFLATED

test/jdk/jdk/nio/zipfs/TestLocOffsetFromZip64EF.java line 218:

> 216: 
> 217:         // Update the Zip64 field with the real tag
> 218:         buffer.putShort((short) 0x1); //  Tag for Zip64

I would probably make 0x1 a constant like the unknown tag.

It would be good to beef up the comment as the byte array written by ZipEntry::setExtra is set to the unknown tag for the Tag and the length is set to 28 which includes all of the Zip64 extra fields

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

PR Review: https://git.openjdk.org/jdk/pull/12979#pullrequestreview-1335237759
PR Review: https://git.openjdk.org/jdk/pull/12979#pullrequestreview-1346294478
Marked as reviewed by lancea (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/12979#pullrequestreview-1355345890
PR Review Comment: https://git.openjdk.org/jdk/pull/12979#discussion_r1140464116
PR Review Comment: https://git.openjdk.org/jdk/pull/12979#discussion_r1146677274
PR Review Comment: https://git.openjdk.org/jdk/pull/12979#discussion_r1140457583