archive/zip: zip64 extra headers problems #33116
Comments
The central-directory extra fields can contain either 8, 16 or 24 data bytes for ONLY the fields > uint32max (0xFFFFFFFF). They should not be all set when either CompressedSize, UncompressedSize or offset > uint32max Fixes golang#33116
|
Change https://golang.org/cl/186178 mentions this issue: |
Should be h.UncompressedSize64 > uint32max Fixes golang#33116
odeke-em
changed the title
|
Thank you for this report @philip-firstorder as well as for the CL, and welcome to the Go project! I shall kindly page @dsnet to take a look. |
odeke-em
added
the
NeedsInvestigation
|
@odeke-em Thank you for forwarding this. I will be using my fork in production till this gets released. |
|
Hi @odeke-em, I quoted the official documentation in the description of this issue. I also think I understand why the confusion with
This refers to the local file headers and data descriptors, which are usually ignored by legacy unarchivers in favor of the central directory records at the end of the zip file.
The bug I discovered was in the central directory code, which should follow the 4.5.3 chapter in the documentation, not 4.3.9.2. Does that make sense? |
|
Sure it does, but please make those changes in the CL/code body and the
commit message as well as well as please take a look at the feedback that I
provided for your change/PR and the test suggestion that I made.
…On Sun, Nov 10, 2019 at 5:55 AM Philip ***@***.***> wrote:
Hi @odeke-em <https://github.com/odeke-em>, I quoted the official
documentation in the description of this issue.
I also think I understand why the confusion with
4.3.9.2 which says:
However ZIP64 format MAY be used regardless of the size of a file.
This refers to the local file headers and data descriptors, which are
usually ignored by legacy unarchivers in favor of the central directory
records at the end of the zip file.
4.3.6 Overall .ZIP file format:
[local file header 1]
[encryption header 1]
[file data 1]
[data descriptor 1]
.
.
.
[local file header n]
[encryption header n]
[file data n]
[data descriptor n]
[archive decryption header]
[archive extra data record]
[central directory header 1]
.
.
.
[central directory header n]
[zip64 end of central directory record]
[zip64 end of central directory locator]
[end of central directory record]
The bug I discovered was in the central directory code, which should
follow the 4.5.3 chapter in the documentation, not 4.3.9.2.
Does that make sense?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33116?email_source=notifications&email_token=ABFL3V2X5R3NXDMODCQZ43DQTAHFXA5CNFSM4IDWJRE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDU5WJY#issuecomment-552196903>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFL3VYENIRHT3CPZCSAYY3QTAHFXANCNFSM4IDWJREQ>
.
|
|
How’s it going @philip-firstorder? Go1.15 development is currently in session. I left some feedback on your change. Please take a look, thanks! |
|
Hello @odeke-em. Unfortunately my knowledge in unit-testing in Go is limited. Can you please refer to the original programmer of the zip testing code? |
|
Hello Philip,
I actually created for you a test and added review feedback ages ago
when I last replied to the CL. Please take a look at that CL on Gerrit and
you’ll see the test and some code updates requests :)
…On Tue, Mar 3, 2020 at 5:18 AM Philip ***@***.***> wrote:
Hello @odeke-em <https://github.com/odeke-em>. Unfortunately my knowledge
in unit-testing in Go is limited. Can you please refer to the original
programmer of the zip testing code?
Since I implemented the fix in my code I never had an issue again, so I
can pretty mich guarantee that it will work.
Also, Keka, The Unarchiver and some other vendors all released fix updates
for this issue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33116?email_source=notifications&email_token=ABFL3V7RZAWEBP36QMSGVW3RFT7SJA5CNFSM4IDWJRE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENTOHFY#issuecomment-593945495>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFL3V4FPHKTEKTYP4W3JVDRFT7SJANCNFSM4IDWJREQ>
.
|
|
Alright, I will look over that and get back at you. |
|
For easy reference, here is the content that I suggested https://go-review.googlesource.com/c/go/+/186178/2#message-edf24149016442dc4300e59c92570de4d8c049d9 with the test and the commit message update, I wanted to say again, thank you for filing this issue, for investigating it and for owning it and sending a change fixing. You’ve got this and I look forward to interacting with you even more! |
odeke-em
added
NeedsFix
|
@philip-firstorder kindly pinging you as the Go development tree closes in 3 weeks, |
Hi, sorry for the delay but can some programmer at Google do the testing? My knowledge and time hinder me to further this issue. |
|
Hey Philip,
I actually provided you with the various tests and code review
suggestions to get your change up and accepted; please look at the attached
Gerrit CL or please look again at my comment in
#33116 (comment) which has
the link.
Addressing those updates should be painless :) and also to fix the commit
message, please make sure to edit the description on the GitHub PR. If
however you aren’t able to make it, please let me know and I can start the
CL from scratch then mention you as having reported the issue and also
started the CL.
Thank you.
…On Thu, Apr 9, 2020 at 3:03 AM Philip ***@***.***> wrote:
@philip-firstorder <https://github.com/philip-firstorder> kindly pinging
you as the Go development tree closes in 3 weeks,
and it has been more than a month since my last ping :)
Hi, sorry for the delay but can some programmer at Google do the testing?
My knowledge and time hinder me to further this issue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33116 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFL3V74PPDEU3WLNV5GFDLRLWMPZANCNFSM4IDWJREQ>
.
|
|
You could go right ahead and start the CL from scratch with the changes you want to make, no problem for me. Sorry I cannot be of more help here. |
|
source code Lines 127 to 131 in 9488741 A simple solutionI think the following code will solve the problem, I tested it and it works fine. if h.isZip64() || h.offset >= uint32max {
b.uint32(uint32max)
} else {
b.uint32(uint32(h.offset))
} |
Not enough, will probably not work with 7zip. My solution is in production for 1 year now and was tested against thousands archives without error. Also my solution was validated by the programmers from 7zip and Keka, so I'm quite confident it's the right one. |
Just ran across the same issue w/ >4gb files in zip archives created by The proposed fix from @philip-firstorder also looks reasonable to only write a field in the extended attribute if > 0xFFFFFFFF. How extensive is the compatibility and regression suite with existing built-in OS 'unzip'? (e.g. Windows 10 'Extract All..' from the explorer context menu). |
See also golang/go#33116 The offset must be written into the central directory as 0xFFFFFFFF if it's going to be written in the zip64 extended information field. https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT The following is the layout of the zip64 extended information "extra" block. If one of the size or offset fields in the Local or Central directory record is too small to hold the required data, a Zip64 extended information record is created. The order of the fields in the zip64 extended information record is fixed, but the fields MUST only appear if the corresponding Local or Central directory record field is set to 0xFFFF or 0xFFFFFFFF. Co-authored-by: Joshua Boelter <joshua.boelter@intel.com>
|
Hello, I have some issues too with the generation of big zips:
I have tested the proposed fixes but it does not change the outcome I have tested the integrity with 4 cli: unzipada (OK) zip (OK) unzip (OK) 7z (KO) Tools to repair the zip work fine
Sample code to reproduce f, err := os.Create("/tmp/test.zip")
if err != nil {
panic(err)
}
w := zip.NewWriter(f)
token := make([]byte, 0xffffffff+42)
rand.Read(token)
fw, err := w.CreateHeader(&zip.FileHeader{
Name: "bigfile.txt",
Method: zip.Store,
UncompressedSize64: uint64(len(token)),
CompressedSize64: uint64(len(token)),
})
if err != nil {
panic(err)
}
_, err = fw.Write(token)
if err != nil {
panic(err)
}
err = w.Close()
if err != nil {
panic(err)
} |
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
Created 2 blank files:
Archived them with Go/archive/zip in the EXACT ORDER as above using:
Then inspected the result in terminal with zipinfo -v and the contents with Hex Fiend:.
Then I tested the archive against all popular unarchivers and spend the last 2 weeks double-checking with their respective developers to confirm the problem:
What did you expect to see?
Document: https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT
Not documented here is what to do when the size is EXACTLY 0xFFFFFFFF. But since the main purpose of archiving files is to make them as small as possible, then it makes more sense to NOT add a useless zip64 with a 0 value taking 8 extra data bytes for nothing.
Thus the Central-directory extra fields should contain only 16 and 8 data bytes, see correct 7zip code, for ONLY the fields > 0xFFFFFFFF:
Entry 1. Only 16 bytes for compressed and uncompressed sizes
Entry 2. Only 8 bytes for the offset of local header
What did you see instead?
Go wrongly adds 24 data bytes in the extra fields for both files:
This causes errors or warning messages in some unarchivers.
Source code explanation
src/archive/zip/writer.go#L101
Proposed Solution
Replacing the above code with this should solve the problems.
The text was updated successfully, but these errors were encountered: