10000 Remove constraint that zip64 field values be greater than 0 by JetForMe · Pull Request #319 · weichsel/ZIPFoundation · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove constraint that zip64 field values be greater than 0 #319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Remove constraint that zip64 field values be greater than 0 #319

wants to merge 1 commit into from

Conversation

JetForMe
Copy link

Fixes #318

Changes proposed in this PR

In ZIP64 archives, certain standard values (like compressed and uncompressed file sizes, and offsets within the archive) are set to 0xFFFFFFFF, and actual values are provided in the ZIP64 extra fields. The code was requiring these values be > 0. If a value was 0, the non-ZIP64 value was returned (0xFFFFFFFF), and thus almost guaranteed to be incorrect.

This patch removes that test for compressedSize, uncompressedSize, and relativeOffsetOfLocalHeader. While it’s unlikely for a file to have zero length, it’s not impossible, and it’s very likely for a local header to be at offset 0.

Tests performed

I tested this by seeing if I could iterate over the entries of the file in the referenced bug. Before, the iterator would immediately return nothing, now it returns the expected three entries. I didn’t want to add the referenced file to the repo, and wasn’t sure of the best way to add this test.

Further info for the reviewer

None.

Open Issues

None.

@JetForMe
Remove constraint that zip64 field values be greater than 0
a5f7263 
In ZIP64 archives, certain standard field values (like file sizes and offsets within the archive) are set to 0xFFFFFFFF, and actual values are provided in the ZIP64 extra fields. The code was requiring these values be > 0. If a value was 0, the non-ZIP64 value was returned (0xFFFFFFFF), and thus almost guaranteed to be incorrect.

This patch removes that test for `compressedSize`, `uncompressedSize`, and `relativeOffsetOfLocalHeader`. While it’s unlikely for a file to have zero length, it’s not impossible, and it’s very likely for a local header to be at offset 0.
@weichsel
Copy link
Owner
weichsel commented May 2, 2024

Thanks for looking into this issue.

There seem to be archives that require those size > 0 safe guards though (e.g. testExtractCompressedZIP64Entries fails for me). I haven't looked into the details, but simply removing those constraints won't work.

@JetForMe
Copy link
Author
JetForMe commented May 12, 2024 •
edited
Loading

There seem to be archives that require those size > 0 safe guards though (e.g. testExtractCompressedZIP64Entries fails for me). I haven't looked into the details, but simply removing those constraints won't work.

How do you create testExtractCompressedZIP64Entries.zip? It doesn't look like a valid ZIP64 file. macOS can unzip it, but testing the archive reveals issues:

% unzip -vT testExtractCompressedZIP64Entries.zip

note:  didn't find end-of-central-dir signature at end of central dir.

Looking at it in HexEdit, I see the EOCD (PKWare App Note section 4.3.16):

Field Value Notes
Signature 50 4B 05 06 Correct
Number of this disk 00 00 Should be 0xFFFF for ZIP64
Number of disk with start of CD 00 00 Should be 0xFFFF for ZIP64
Total number of entries in CD on this disk 01 00 Should be 0xFFFF for ZIP64
Total number of entries in CD 01 00 Should be 0xFFFF for ZIP64
Size of CD 77 00 00 00 Should be 0xFFFFFFFF for ZIP64
Offset of CD FF FF FF FF Correct
Comment length 00 00 Correct

The Zip64 end of central directory locator (4.3.15) appears just before this:

Field Value Notes
Signature 50 4B 06 07 Correct
Disk with start of EOCD64 00 00 00 00 Correct
Start of EOCD64 DA 01 00 00 00 00 00 00 (474) Correct
Total number of disks 01 00 00 00 Correct

The EOCDR64 (4.3.14) is:

Field Value Notes
Signature 50 4B 06 06 Correct
Size of EOCDR64 2C 00 00 00 00 00 00 00 (44) Correct
Version made by 1E 03
Version required 2D 00
Number of this disk 00 00 00 00 Correct
Number of disk with start of CD 00 00 00 00 Correct
Total number of entries this disk 01 00 00 00 00 00 00 00 Correct
Total number of entries 01 00 00 00 00 00 00 00 Correct
Size of the CD 77 00 00 00 00 00 00 00 (119) Correct
Offset to start of CD 63 01 00 00 00 00 00 00 (355) Correct
Extensible data None

The CD (4.3.12) is:

Field Value Notes
Signature 50 4B 01 02
Version made by 1E 03
Version required 2D 00
Flags 00 00
Compression Method 08 00
Last mod time 10 75
Last mod date A6 52
CRC-32 B7 9B 29 DC
Compressed size F0 00 00 00 (240) Should be 0xFFFFFFFF for ZIP64
Uncompressed size FF FF FF FF Correct
File name length 25 00 (37)
Extra field length 24 00 (36)
File comment length 00 00
Disk number start 00 00
Internal file attrs 00 00
External file attrs 00 00 A4 81
Local header rel offset 00 00 00 00
Filename 74 65 73 74 45 78 74 72 61 63 74 43 6F 6D 70 72 65 73 73 65 64 5A 49 50 36 34 45 6E 74 72 69 65 73 2E 70 6E 67
Extra field 55 54 05 00 03 4F 81 93 60 75 78 0B 00 01 04 F6 01 00 00 04 14 00 00 00 01 00 08 00 D0 03 00 00 00 00 00 00
File comment None

The extra field breaks down as:

Field Value Notes
Header ID 55 54 Extended timestamp
Data Size 05 00 (5)
Data 1 03 4F 81 93 60
Header ID 2 75 78 Unix UID/GID info
Data size 2 0B 00 (11)
Data 2 01 04 F6 01 00 00 04 14 00 00 00
Header ID 3 01 00 Zip64 extra info
Data Size 3 08 00 (8)
Uncompressed size D0 03 00 00 00 00 00 00 (976) Correct. But there should be more fields for this extra field (compressed size, offset, disk number)

I'm going to use this information to see if I can't refine how my patch gets the values.

@JetForMe JetForMe closed this May 12, 2024
@JetForMe JetForMe reopened this May 12, 2024
@weichsel
Copy link
Owner
weichsel commented Jan 6, 2025

How do you create testExtractCompressedZIP64Entries.zip? It doesn't look like a valid ZIP64 file. macOS can unzip it, but testing the archive reveals issues:

The ZIP64 support was provided by @Ckitakishi . It seems that the sheer existence of ZIP64 extended information is enough to treat the archive as ZIP64 (taking precedence over formally setting versionNeededToExtract).
@Ckitakishi:

  • Was this relaxation needed for archives created by other libraries/apps? Is this also the reason for the additional size check?
  • Do you recall how the test asset was created?

@JetForMe:
One thing I noticed with the archive you attached to the referenced issue:
When printing ZIP contents via zipinfo -v GO-M8010-6-P1.3mf, I get the following output for the last 2 entries:
There are an extra -8 bytes preceding this file. Very likely that we can't deal with that (and not sure if we should 🙈).

@Ckitakishi
Copy link
Contributor

@weichsel

Was this relaxation needed for archives created by other libraries/apps? Is this also the reason for the additional size check?

To be honest, I don't remember the reason clearly 😢
Regarding the extra size checking, if I remember correctly, there seems to be a issue with the current compression or decompression implementation, which can cause crash.

Do you recall how the test asset was created?

I think I created it via ZIPFoundation(by modifying the max size limit), because it's hard to include a large zip64 file in repository

Since I'm on vacation right now, I'll take a look at it again after that.

@Ckitakishi
Copy link
Contributor

Hello, recently I had a chance to investigate this PR again~

Original issue

First, let me explain why the file attached in #318 cannot be unarchived properly,

  • The ZIP64 Extended Information Extra Field can exist in Local File Header (LFH) or Central Directory (CD). According to the spec 4.5.3, this rule implies CD doesn't need to include both sizes, which makes sense, as the CD serves primarily as a lookup directory, from what I’ve learned, historically, space-saving was an important consideration back in 1989.

    4.5.3 ... This entry in the Local header MUST include BOTH original
    and compressed file size fields.

  • Based on this design, the values in the Extended Information fields are always greater than 0 in the CD, so the current implementation should work if the ZIP files are created follow this rule.

Compatibility improvement

Secondly, I think the proposed fix in this PR is reasonable. However, we should first consider improving the compatibility of ZIPFoundation’s ZIP64 implementation:

  • Currently, nil values are actually treated as 0, which is not elegant enough. A better approach might be to use Optional types to represent nil, rather than mixing it with 0.
  • However, this improvement is not easy, because currently we're using MemoryLayout<T> and similar approaches in multiple places to calculate type sizes. Changing a type to Optional will definitely break them.

About testExtractCompressedZIP64Entries.zip

To be honest, I don’t remember exactly how I created this file… I believe I generated it using ZIPFoundation by manually modifying the max size limit. But I think it conforms to the ZIP64 specification. Here are a few points:

Field Value Notes Comment
Signature 50 4B 05 06 Correct  
Number of this disk 00 00 Should be 0xFFFF for ZIP64 should be correct according to 4.4.1.4
Number of disk with start of CD 00 00 Should be 0xFFFF for ZIP64  
Total number of entries in CD on this disk 01 00 Should be 0xFFFF for ZIP64  
Total number of entries in CD 01 00 Should be 0xFFFF for ZIP64  
Size of CD 77 00 00 00 Should be 0xFFFFFFFF for ZIP64  
Offset of CD FF FF FF FF Correct  
Comment length 00 00 Correct  

  1. End of Central Directory (EOCD), according to the spec, it's not required that all fields in EOCD be set to -1 (0xFFFF or 0xFFFFFFFF) when an individual field exceeds its max value, and I couldn't find any other clues that imply we should set all values to max from the documentation.

    4.4.1.4 If one of the fields in the end of central directory
    record is too small to hold required data, the field SHOULD be
    set to -1 (0xFFFF or 0xFFFFFFFF) and the ZIP64 format record
    SHOULD be created.

  2. Central Directory (CD), as mentioned earlier, the ZIP64 Extended Information field in the Central Directory does not need to include both compressed and uncompressed sizes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

When I try to iterate over the entries of the attached file, nothing happens
3 participants
@JetForMe @weichsel @Ckitakishi
0