RFR: 8303972: (zipfs) Make test/jdk/jdk/nio/zipfs/TestLocOffsetFromZip64EF.java independent of the zip command line
Eirik Bjørsnøs
eirbjo at openjdk.org
Thu Feb 15 13:52:03 UTC 2024
On Fri, 10 Mar 2023 16:56:55 GMT, Lance Andersen <lancea at openjdk.org> wrote:
> 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
Yes, I just noticed that and currently looking into it. I think in particular it is sensitive to the ordering of the extended time stamp and the Zip64 entries, which is not the same as ZipOutputStream.
> I think I would include an additional entry in the zip just as an extra sanity check that we can navigate an additional entry
Added an additional entry as suggested and checked that is was navigated.
> 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
This stray opaque extra field should not be any issue, but just in case, I moved the ZipFIle.setExtra until after the LOC is written, so now only the CEN will have the Zip64 extra field.
> 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
This PR was approved by @LanceAndersen in March 2023, but for some reason it was never integrated. Most probably I was waiting for a final approval following the last review comments from Lance. The test was rewritten to JUnit in October 2023.
The test is useful in that it covers an aspect sensitive to the order of ZIP64 extended fields which is produced by Info-ZIP, but which ZipOutputStream cannot produce.
I think it would be good to bring this problem-listed test back to life and also make it platform independent.
> 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
I believe this should be well-described now between the constants and the `makeOpaqueExtraField` method.
> 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
I added a third DEFLATED entry, added asserts that ZipFile and ZipFileSystem finds the expected entries. The LOC traversal no longer works, so the CEN offset is now looked up from the EOC header instead.
> I would probably make 0x1 a constant like the unknown tag.
Extracted a constant with a comment.
> 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
Opted to extract a method to create the opaque extra field since that allows for more comments and declutters the calling method.
I dropped the 'disk start' field since it is not relevant and we don't set the corresponding magic value. So the Zip64 data size is now 24 instead of 28.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/12979#issuecomment-1464103188
PR Comment: https://git.openjdk.org/jdk/pull/12979#issuecomment-1474300281
PR Comment: https://git.openjdk.org/jdk/pull/12979#issuecomment-1933880614
PR Review Comment: https://git.openjdk.org/jdk/pull/12979#discussion_r1140626457
PR Review Comment: https://git.openjdk.org/jdk/pull/12979#discussion_r1147207105
PR Review Comment: https://git.openjdk.org/jdk/pull/12979#discussion_r1140625661
More information about the nio-dev
mailing list