Skip to content

fix: zip64 creation of exactly 4GB without large file should crash#839

Open
Its-Just-Nans wants to merge 14 commits into
masterfrom
fix-zip64-without-large-file
Open

fix: zip64 creation of exactly 4GB without large file should crash#839
Its-Just-Nans wants to merge 14 commits into
masterfrom
fix-zip64-without-large-file

Conversation

@Its-Just-Nans
Copy link
Copy Markdown
Member

@Its-Just-Nans Its-Just-Nans commented May 15, 2026

While doing the extra field rewrite I found TWO little bug with zip64

  • the maximum is not u32::MAX but (u32::MAX -1), and we need to check for >= instead of >. linked with this line which looks wrong
    // Write a file that's just under 4GB (4GB - 1 byte)
    let size = u32::MAX;
  • the write_central_and_footer is not handling zip64

Bug 1

If we create a file of exactly 4GB, the header does not contains a zip 64 extended information extra field

000000000 LOCAL HEADER #1       04034B50 (67324752)
000000004 Extract Zip Spec      0A (10) '1.0'
000000005 Extract OS            00 (0) 'MS-DOS'
000000006 General Purpose Flag  0000 (0)
000000008 Compression Method    0000 (0) 'Stored'
00000000A Modification Time     5CAF560F (1554994703) 'Fri May 15 12:48:30 2026'
00000000E CRC                   00000000 (0)
000000012 Compressed Size       FFFFFFFF (4294967295)
000000016 Uncompressed Size     FFFFFFFF (4294967295)
00000001A Filename Length       000C (12)
00000001C Extra Length          0000 (0)
00000001E Filename              'close_to_4gb'
00000002A PAYLOAD

but it should

the fields MUST
only appear if the corresponding Local or Central
directory record field is set to 0xFFFF or 0xFFFFFFFF

this is due to the fact that our comparaison is > and not >=

Bug 2

On debug, we can have panic on drop because of a debug_asset!. Which is not great

@Its-Just-Nans Its-Just-Nans self-assigned this May 15, 2026
@Its-Just-Nans Its-Just-Nans requested a review from Pr0methean May 15, 2026 10:52
@Its-Just-Nans Its-Just-Nans changed the title fix: zip64 creation without large file should crash fix: zip64 creation of exactly 4GB without large file should crash May 15, 2026
Copy link
Copy Markdown
Contributor

@amazon-q-developer amazon-q-developer Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix correctly addresses the ZIP64 boundary condition bug. The comparison change from > to >= at line 2441 properly handles files of exactly 4GB, ensuring the ZIP64 extended information extra field is included as required by the ZIP specification. The added error handling and test coverage strengthen the implementation.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves error handling when updating local file headers and adjusts the Zip64 threshold comparison from > to >= to correctly handle files at the 4GB limit. It also includes a new test case for 4GB files. Feedback suggests that the Zip64 threshold change should be applied consistently across other modules, such as EOCD64 and central directory headers, to avoid potential corruption. Additionally, the error handling logic in src/write.rs should be refined to ensure the writer's state is correctly reset and errors are not swallowed if a seek operation fails.

Comment thread src/write.rs
Comment thread src/write.rs Outdated
Comment thread src/types.rs Fixed
@Its-Just-Nans
Copy link
Copy Markdown
Member Author

Its-Just-Nans commented May 15, 2026

@Pr0methean

I'm not a big fan of introducing internal_error but it's used because in the finalize() (called when drop/out of scope), a debug_assert crash the program

Also note that with my ongoing extra field rewrite, some of this code will be rewritten (but I still prefer to have a correct base)

@Its-Just-Nans
Copy link
Copy Markdown
Member Author

@Pr0methean

I'm not a big fan of introducing internal_error but it's used because in the finalize() (called when drop/out of scope), a debug_assert crash the program

Also note that with my ongoing extra field rewrite, some of this code will be rewritten (but I still prefer to have a correct base)

Okay I think I have fixed the "panic on drop" error that was forcing me to use a new variable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants