RFR: 8303972: Make test/jdk/jdk/nio/zipfs/TestLocOffsetFromZip64EF.java independent of the zip command line [v4]

Lance Andersen lancea at openjdk.org
Fri Mar 17 16:48:28 UTC 2023


On Sat, 11 Mar 2023 07:28:14 GMT, Eirik Bjorsnos <duke 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.
>
> Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision:
> 
>   APPNOTE.txt actually specifies an 'unknown' tag, let's use that instead of 0x42

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

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 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: https://git.openjdk.org/jdk/pull/12979


More information about the nio-dev mailing list