threads / bug / 40054
Subject: bug: git-archive does not use the zip64 extension for archives with more than 16k entries
Hi,
for repositories with more than 16k files and folders, git-archive will create zip files which store the wrong number of entries. That is, it stores the number of entries modulo 16k. This will break unpackers that do not include code to support this brokenness.
Instead, git-archive should use the zip64 extension to handle more than 16k files and folders correctly.
Thanks!
cheers, josch
René ScharfeRe: bug: git-archive does not use the zip64 extension for archives with more than 16k entries
Am 11.08.2015 um 12:40 schrieb Johannes Schauer:
> Hi, > > for repositories with more than 16k files and folders, git-archive will create > zip files which store the wrong number of entries. That is, it stores the > number of entries modulo 16k. This will break unpackers that do not include > code to support this brokenness.
The limit is rather 65535 entries, isn't it? The entries field has two bytes, and they are used fully.
Which programs are affected? InfoZIP Zip 3.0 and 7-Zip 9.20 seem to handle an archive with more entries just fine. The built-in functionality of Windows 10 doesn't.
Besides, 64K entries should be enough for anybody. ;-)
Seriously, though: What kind of repository has that many files and uses the ZIP format to distribute snapshots? Just curious.
> Instead, git-archive should use the zip64 extension to handle more than 16k > files and folders correctly.
That seems to be what InfoZIP does and Windows 10 handles it just fine. If lower Windows versions and other popular extractors can unzip such archives as well then this might indeed be the way to go.
Thanks, René
Re: bug: git-archive does not use the zip64 extension for archives with more than 16k entries
Hi,
Quoting René Scharfe (2015-08-12 21:40:48)
> Am 11.08.2015 um 12:40 schrieb Johannes Schauer: > > for repositories with more than 16k files and folders, git-archive will create > > zip files which store the wrong number of entries. That is, it stores the > > number of entries modulo 16k. This will break unpackers that do not include > > code to support this brokenness. > > The limit is rather 65535 entries, isn't it? The entries field has two > bytes, and they are used fully.
seems to be 65535 indeed.
I just forwarded the number Dieter Baron (libzip contributor) told me when they replied (off list) to my bug report against libzip: http://nih.at/listarchive/libzip-discuss/msg00554.html
But reading https://en.wikipedia.org/wiki/Zip_(file_format)#ZIP64 the limit indeed seems to be 65535.
> Which programs are affected? InfoZIP Zip 3.0 and 7-Zip 9.20 seem to handle an > archive with more entries just fine. The built-in functionality of Windows > 10 doesn't.
In my case I discovered this because libzip http://nih.at/libzip does not implement reading an archive with more than 65535 entries without zip64.
> Besides, 64K entries should be enough for anybody. ;-)
:P
> Seriously, though: What kind of repository has that many files and uses the > ZIP format to distribute snapshots? Just curious.
I have not searched for any.
In my case I was using git to keep track of the modifications our tools do to a directory of files to detect regressions. These tools are also able to read data from zip archives instead from a directory. I created the zip archive using git-archive because the files already were in git so that seemed most convenient to me. That's when I discovered the problem because our tools use libzip. The easy workaround was to use another packager instead of git-archive. We use the zip format because Windows has support for it.
> > Instead, git-archive should use the zip64 extension to handle more than 16k > > files and folders correctly. > > That seems to be what InfoZIP does and Windows 10 handles it just fine. If > lower Windows versions and other popular extractors can unzip such archives > as well then this might indeed be the way to go.
The wikipedia page above claims that windows versions starting with vista have support for zip64. It also lists some other software with support for it.
Thanks!
cheers, josch
René Scharfe[PATCH 1/3] t5004: test ZIP archives with many entries
A ZIP file directory has a 16-bit field for the number of entries it contains. There are 64-bit extensions to deal with that. Demonstrate that git archive --format=zip currently doesn't use them and instead overflows the field.
InfoZIP's unzip doesn't care about this field and extracts all files anyway. Software that uses the directory for presenting a filesystem like view quickly -- notably Windows -- depends on it, but doesn't lend itself to an automatic test case easily. Use InfoZIP's zipinfo, which probably isn't available everywhere but at least can provides *some* way to check this field.
To speed things up a bit create and commit only a subset of the files and build a fake tree out of duplicates and pass that to git archive.
Signed-off-by: Rene Scharfe <l.s.r@web.de> --- t/t5004-archive-corner-cases.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh index 654adda..c6bd729 100755 --- a/t/t5004-archive-corner-cases.sh +++ b/t/t5004-archive-corner-cases.sh @@ -115,4 +115,44 @@ test_expect_success 'archive empty subtree by direct pathspec' ' check_dir extract sub ' +ZIPINFO=zipinfo + +test_lazy_prereq ZIPINFO ' + n=$("$ZIPINFO" "$TEST_DIRECTORY"/t5004/empty.zip | sed -n "2s/.* //p") + test "x$n" = "x0" +' + +test_expect_failure ZIPINFO 'zip archive with many entries' ' + # add a directory with 256 files + mkdir 00 && + for a in 0 1 2 3 4 5 6 7 8 9 a b c d e f + do + for b in 0 1 2 3 4 5 6 7 8 9 a b c d e f + do + : >00/$a$b + done + done && + git add 00 && + git commit -m "256 files in 1 directory" && + + # duplicate it to get 65536 files in 256 directories + subtree=$(git write-tree --prefix=00/) && + for c in 0 1 2 3 4 5 6 7 8 9 a b c d e f + do + for d in 0 1 2 3 4 5 6 7 8 9 a b c d e f + do + echo "040000 tree $subtree $c$d" + done + done >tree && + tree=$(git mktree <tree) && + + # zip them + git archive -o many.zip $tree && + + # check the number of entries in the ZIP file directory + expr 65536 + 256 >expect && + "$ZIPINFO" many.zip | head -2 | sed -n "2s/.* //p" >actual && + test_cmp expect actual +' + test_done
-- 2.5.0
Eric SunshineRe: [PATCH 1/3] t5004: test ZIP archives with many entries
On Sat, Aug 22, 2015 at 3:06 PM, René Scharfe <l.s.r@web.de> wrote:
> A ZIP file directory has a 16-bit field for the number of entries it
> contains. There are 64-bit extensions to deal with that. Demonstrate
> that git archive --format=zip currently doesn't use them and instead
> overflows the field.
>
> InfoZIP's unzip doesn't care about this field and extracts all files
> anyway. Software that uses the directory for presenting a filesystem
> like view quickly -- notably Windows -- depends on it, but doesn't
> lend itself to an automatic test case easily. Use InfoZIP's zipinfo,
> which probably isn't available everywhere but at least can provides
> *some* way to check this field.
>
> To speed things up a bit create and commit only a subset of the files
> and build a fake tree out of duplicates and pass that to git archive.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
> index 654adda..c6bd729 100755
> --- a/t/t5004-archive-corner-cases.sh
> +++ b/t/t5004-archive-corner-cases.sh
> @@ -115,4 +115,44 @@ test_expect_success 'archive empty subtree by direct pathspec' '
> check_dir extract sub
> '
>
> +ZIPINFO=zipinfo
> +
> +test_lazy_prereq ZIPINFO '
> + n=$("$ZIPINFO" "$TEST_DIRECTORY"/t5004/empty.zip | sed -n "2s/.* //p")
> + test "x$n" = "x0"
> +'Unfortunately, this sed expression isn't portable due to dissimilar output of various zipinfo implementations. On Linux, the output of zipinfo is:
$ zipinfo t/t5004/empty.zip
Archive: t/t5004/empty.zip
Zip file size: 62 bytes, number of entries: 0
Empty zipfile.
$however, on Mac OS X:
$ zipinfo t/t5004/empty.zip
Archive: t/t5004/empty.zip 62 bytes 0 files
Empty zipfile.
$and on FreeBSD, the zipinfo command seems to have been removed altogether in favor of "unzip -Z" (emulate zipinfo).
One might hope that "unzip -Z" would be a reasonable replacement for zipinfo, however, it is apparently only partially implemented on FreeBSD, and requires that -1 be passed, as well. Even with "unzip -Z -1", there are issues. The output on Linux and Mac OS X is:
$ unzip -Z -1 t/t5004/empty.zip
Empty zipfile.
$but FreeBSD differs:
$ unzip -Z -1 t/t5004/empty.zip
$With a non-empty zip file, the output is identical on all platforms:
$ unzip -Z -1 twofiles.zip
file1
file2
$So, if you combine that with "wc -l" or test_line_count, you may have a portable and reliable entry counter.
More below...
> +test_expect_failure ZIPINFO 'zip archive with many entries' ' > + # add a directory with 256 files > + mkdir 00 && > + for a in 0 1 2 3 4 5 6 7 8 9 a b c d e f > + do > + for b in 0 1 2 3 4 5 6 7 8 9 a b c d e f > + do > + : >00/$a$b > + done > + done && > + git add 00 && > + git commit -m "256 files in 1 directory" && > + > + # duplicate it to get 65536 files in 256 directories > + subtree=$(git write-tree --prefix=00/) && > + for c in 0 1 2 3 4 5 6 7 8 9 a b c d e f > + do > + for d in 0 1 2 3 4 5 6 7 8 9 a b c d e f > + do > + echo "040000 tree $subtree $c$d" > + done > + done >tree && > + tree=$(git mktree <tree) && > + > + # zip them > + git archive -o many.zip $tree && > + > + # check the number of entries in the ZIP file directory > + expr 65536 + 256 >expect && > + "$ZIPINFO" many.zip | head -2 | sed -n "2s/.* //p" >actual &&
With these three patches applied, Mac OS X has trouble with 'many.zip':
$ unzip -Z -1 many.zip
warning [many.zip]: 76 extra bytes at beginning or within zipfile
(attempting to process anyway)
error [many.zip]: reported length of central directory is
-76 bytes too long (Atari STZip zipfile? J.H.Holm ZIPSPLIT 1.1
zipfile?). Compensating...
00/
00/00
...
ff/ff
error: expected central file header signature not found (file
#65793). (please check that you have transferred or created the
zipfile in the appropriate BINARY mode and that you have compiled
UnZip properly)And FreeBSD doesn't like it either:
$ unzip -Z -1 many.zip
unzip: Invalid central directory signature
$> + test_cmp expect actual > +' > + > test_done > -- > 2.5.0
René ScharfeRe: [PATCH 1/3] t5004: test ZIP archives with many entries
Am 23.08.2015 um 07:54 schrieb Eric Sunshine:
> On Sat, Aug 22, 2015 at 3:06 PM, René Scharfe <l.s.r@web.de> wrote:
>> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
>> index 654adda..c6bd729 100755
>> --- a/t/t5004-archive-corner-cases.sh
>> +++ b/t/t5004-archive-corner-cases.sh
>> @@ -115,4 +115,44 @@ test_expect_success 'archive empty subtree by direct pathspec' '
>> check_dir extract sub
>> '
>>
>> +ZIPINFO=zipinfo
>> +
>> +test_lazy_prereq ZIPINFO '
>> + n=$("$ZIPINFO" "$TEST_DIRECTORY"/t5004/empty.zip | sed -n "2s/.* //p")
>> + test "x$n" = "x0"
>> +'
>
> Unfortunately, this sed expression isn't portable due to dissimilar
> output of various zipinfo implementations. On Linux, the output of
> zipinfo is:
>
> $ zipinfo t/t5004/empty.zip
> Archive: t/t5004/empty.zip
> Zip file size: 62 bytes, number of entries: 0
> Empty zipfile.
> $
>
> however, on Mac OS X:
>
> $ zipinfo t/t5004/empty.zip
> Archive: t/t5004/empty.zip 62 bytes 0 files
> Empty zipfile.
> $
>
> and on FreeBSD, the zipinfo command seems to have been removed
> altogether in favor of "unzip -Z" (emulate zipinfo).Thanks for your thorough checks!
I suspected that zipinfo's output might be formatted differently on different platforms and tried to guard against it by checking for the number zero there. Git's ZIP file creation is platform independent (modulo bugs), so having a test run at least somewhere should suffice. In theory.
We could add support for the one-line-summary variant on OS X easily, though.
> One might hope that "unzip -Z" would be a reasonable replacement for > zipinfo, however, it is apparently only partially implemented on > FreeBSD, and requires that -1 be passed, as well. Even with "unzip -Z > -1", there are issues. The output on Linux and Mac OS X is: > > $ unzip -Z -1 t/t5004/empty.zip > Empty zipfile. > $ > > but FreeBSD differs: > > $ unzip -Z -1 t/t5004/empty.zip > $ > > With a non-empty zip file, the output is identical on all platforms: > > $ unzip -Z -1 twofiles.zip > file1 > file2 > $ > > So, if you combine that with "wc -l" or test_line_count, you may have > a portable and reliable entry counter.
Counting all entries is slow, and more importantly it's not what we want. In this test we need the number of entries recorded in the ZIP directory, not the actual number of entries found by scanning the archive, or the directory.
On Linux "unzip -Z -1 many.zip | wc -l" reports 65792 even before adding ZIP64 support; only without -1 we get the interesting numbers (specifically with "unzip -Z many.zip | sed -n '2p;$p'"):
Zip file size: 6841366 bytes, number of entries: 256
65792 files, 0 bytes uncompressed, 0 bytes compressed: 0.0%> With these three patches applied, Mac OS X has trouble with 'many.zip': > > $ unzip -Z -1 many.zip > warning [many.zip]: 76 extra bytes at beginning or within zipfile > (attempting to process anyway) > error [many.zip]: reported length of central directory is > -76 bytes too long (Atari STZip zipfile? J.H.Holm ZIPSPLIT 1.1 > zipfile?). Compensating... > 00/ > 00/00 > ... > ff/ff > error: expected central file header signature not found (file > #65793). (please check that you have transferred or created the > zipfile in the appropriate BINARY mode and that you have compiled > UnZip properly) > > And FreeBSD doesn't like it either: > > $ unzip -Z -1 many.zip > unzip: Invalid central directory signature > $ >
Looks like they don't support ZIP64. Or I got some of the fields wrong after all.
https://en.wikipedia.org/wiki/Zip_%28file_format%29#ZIP64 says: "OS X Yosemite does support the creation of ZIP64 archives, but does not support unzipping these archives using the shipped unzip command-line utility or graphical Archive Utility.[citation needed]".
How does unzip react to a ZIP file with more than 65535 entries that was created natively on these platforms? And what does zipinfo (a real one, without -1) report at the top for such files?
Thanks, René
René ScharfeEric Sunshine mail delivery failure
Eric, hope you see this reply on the list. Direct replies to sunshine@sunshineco.com are rejected by my mail provider on submit in Thunderbird with the following message:
Requested action not taken: mailbox unavailable
invalid DNS MX or A/AAAA resource record.And with this one when using their web interface:
A message that you sent could not be delivered to one or more of
its recipients. This is a permanent error. The following address
failed: "sunshine@sunshineco.com":
no valid MX hosts foundIt seems web.de wants you to get an AAAA record before I'm allowed to send mails to you. Sounds crazy. Sorry about that. Time to find a better provider, I guess. :-(
René
Johannes LöthbergRe: Eric Sunshine mail delivery failure
On 23/08, René Scharfe wrote:
>Eric, hope you see this reply on the list. Direct replies to >sunshine@sunshineco.com are rejected by my mail provider on submit in >Thunderbird with the following message: > > Requested action not taken: mailbox unavailable > invalid DNS MX or A/AAAA resource record. > >And with this one when using their web interface: > > A message that you sent could not be delivered to one or more of > its recipients. This is a permanent error. The following address > failed: > > "sunshine@sunshineco.com": > no valid MX hosts found > >It seems web.de wants you to get an AAAA record before I'm allowed to >send mails to you. Sounds crazy. Sorry about that. Time to find a >better provider, I guess. :-( >
Just an A record would be enough. The issue is that mail.sunshineco.com has neither an A nor an AAAA record, it is a CNAME to sunshineco.com, which is invalid according to RFC2181.
-- Sincerely, Johannes Löthberg PGP Key ID: 0x50FB9B273A9D0BB5 https://theos.kyriasis.com/~kyrias/
Eric SunshineRe: Eric Sunshine mail delivery failure
On Sun, Aug 23, 2015 at 1:16 PM, Johannes Löthberg <johannes@kyriasis.com> wrote:
> On 23/08, René Scharfe wrote: >> Eric, hope you see this reply on the list. Direct replies to >> sunshine@sunshineco.com are rejected by my mail provider on submit in >> Thunderbird with the following message: >> >> Requested action not taken: mailbox unavailable >> invalid DNS MX or A/AAAA resource record. >> >> And with this one when using their web interface: >> >> A message that you sent could not be delivered to one or more of >> its recipients. This is a permanent error. The following address >> failed: >> >> "sunshine@sunshineco.com": >> no valid MX hosts found >> >> It seems web.de wants you to get an AAAA record before I'm allowed to send >> mails to you. > > Just an A record would be enough. The issue is that mail.sunshineco.com has > neither an A nor an AAAA record, it is a CNAME to sunshineco.com, which is > invalid according to RFC2181.
Interestingly, the default configuration for all domains managed by this service provider is for the mailhost to be a CNAME. While the restriction in section 10.3 of RFC2181 makes sense as a way to avoid extra "network burden", in practice, email services seem to be pretty relaxed about it, and follow the CNAME indirection as needed.
I suppose it's possible that web.de is being extra strict (although it seems that such strictness would be painful for its users), or this could just be a temporary DNS lookup failure. It's hard to tell based upon the errors René reported.
I did change the CNAME to an A just in case, though who knows how long it will take for the change to propagate over to web.de's server.
Eric SunshineRe: [PATCH 1/3] t5004: test ZIP archives with many entries
On Sun, Aug 23, 2015 at 5:29 AM, "René Scharfe" <l.s.r@web.de> wrote:
> Am 23.08.2015 um 07:54 schrieb Eric Sunshine:
>> On Sat, Aug 22, 2015 at 3:06 PM, René Scharfe <l.s.r@web.de> wrote:
>>> +test_lazy_prereq ZIPINFO '
>>> + n=$("$ZIPINFO" "$TEST_DIRECTORY"/t5004/empty.zip | sed -n "2s/.* //p")
>>> + test "x$n" = "x0"
>>> +'
>>
>> Unfortunately, this sed expression isn't portable due to dissimilar
>> output of various zipinfo implementations. On Linux, the output of
>> zipinfo is:
>>
>> $ zipinfo t/t5004/empty.zip
>> Archive: t/t5004/empty.zip
>> Zip file size: 62 bytes, number of entries: 0
>> Empty zipfile.
>> $
>>
>> however, on Mac OS X:
>>
>> $ zipinfo t/t5004/empty.zip
>> Archive: t/t5004/empty.zip 62 bytes 0 files
>> Empty zipfile.
>> $
>>
>> and on FreeBSD, the zipinfo command seems to have been removed
>> altogether in favor of "unzip -Z" (emulate zipinfo).
>
> I suspected that zipinfo's output might be formatted differently on
> different platforms and tried to guard against it by checking for the
> number zero there. Git's ZIP file creation is platform independent
> (modulo bugs), so having a test run at least somewhere should
> suffice. In theory.
>
> We could add support for the one-line-summary variant on OS X easily,
> though.Probably, although it's looking like testing on Mac OS X won't be fruitful (see below).
>> One might hope that "unzip -Z" would be a reasonable replacement for >> zipinfo, however, it is apparently only partially implemented on >> FreeBSD, and requires that -1 be passed, as well. Even with "unzip -Z >> -1", there are issues. The output on Linux and Mac OS X is: >> >> $ unzip -Z -1 t/t5004/empty.zip >> Empty zipfile. >> $ >> >> but FreeBSD differs: >> >> $ unzip -Z -1 t/t5004/empty.zip >> $ >> >> With a non-empty zip file, the output is identical on all platforms: >> >> $ unzip -Z -1 twofiles.zip >> file1 >> file2 >> $ >> >> So, if you combine that with "wc -l" or test_line_count, you may have >> a portable and reliable entry counter. > > Counting all entries is slow, and more importantly it's not what we > want. In this test we need the number of entries recorded in the ZIP > directory, not the actual number of entries found by scanning the > archive, or the directory.
Ah, right. The commit message did state this clearly enough...
> On Linux "unzip -Z -1 many.zip | wc -l" reports 65792 even before > adding ZIP64 support; only without -1 we get the interesting numbers > (specifically with "unzip -Z many.zip | sed -n '2p;$p'"): > > Zip file size: 6841366 bytes, number of entries: 256 > 65792 files, 0 bytes uncompressed, 0 bytes compressed: 0.0% > >> With these three patches applied, Mac OS X has trouble with 'many.zip': >> >> $ unzip -Z -1 many.zip >> warning [many.zip]: 76 extra bytes at beginning or within zipfile >> (attempting to process anyway) >> error [many.zip]: reported length of central directory is >> -76 bytes too long (Atari STZip zipfile? J.H.Holm ZIPSPLIT 1.1 >> zipfile?). Compensating... >> 00/ >> 00/00 >> ... >> ff/ff >> error: expected central file header signature not found (file >> #65793). (please check that you have transferred or created the >> zipfile in the appropriate BINARY mode and that you have compiled >> UnZip properly) >> >> And FreeBSD doesn't like it either: >> >> $ unzip -Z -1 many.zip >> unzip: Invalid central directory signature >> $ >> > > Looks like they don't support ZIP64. Or I got some of the fields wrong > after all.
A >65536 file zip created on Mac OS X with Mac's "zip" command given to "unzip" or "zipinfo" results in exactly the same warnings/errors as above (including the bit about "76 extra bytes" and "-76 bytes too long"), so it doesn't seem to be a problem with your implementation.
> https://en.wikipedia.org/wiki/Zip_%28file_format%29#ZIP64 says: "OS X > Yosemite does support the creation of ZIP64 archives, but does not > support unzipping these archives using the shipped unzip command-line > utility or graphical Archive Utility.[citation needed]". > > How does unzip react to a ZIP file with more than 65535 entries that > was created natively on these platforms? And what does zipinfo (a real > one, without -1) report at the top for such files?
On Mac OS X, unzip does extract all the files (although complains as noted above). zipinfo caps out at reporting 65535 for the number of files (although it lists them all fine). With the warnings/errors filtered out for clarity:
$ zipinfo biggy.zip
Archive: biggy.zip 9642874 bytes 65535 files
...
Junio C HamanoRe: [PATCH 1/3] t5004: test ZIP archives with many entries
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Sun, Aug 23, 2015 at 5:29 AM, "René Scharfe" <l.s.r@web.de> wrote: >> I suspected that zipinfo's output might be formatted differently on >> different platforms and tried to guard against it by checking for the >> number zero there. Git's ZIP file creation is platform independent >> (modulo bugs), so having a test run at least somewhere should >> suffice. In theory. >> >> We could add support for the one-line-summary variant on OS X easily, >> though. > > Probably, although it's looking like testing on Mac OS X won't be > fruitful (see below).
Can we move this topic forward by introducing a new prerequisite ZIPINFO and used at the beginning of these tests (make it a lazy prereq)? Run zipinfo on a trivial archive and see if its output is something we recognize to decide if the platform supports that ZIPINFO prerequisite and do this test only on them.
After all, what _is_ being tested, i.e. our archive creation, would not change across platforms, so having a test that runs on a known subset of platforms is better than not having anything at all.
Thanks.
Junio C HamanoRe: [PATCH 1/3] t5004: test ZIP archives with many entries
Junio C Hamano <gitster@pobox.com> writes:
> Eric Sunshine <sunshine@sunshineco.com> writes: > >> On Sun, Aug 23, 2015 at 5:29 AM, "René Scharfe" <l.s.r@web.de> wrote: >>> I suspected that zipinfo's output might be formatted differently on >>> different platforms and tried to guard against it by checking for the >>> number zero there. Git's ZIP file creation is platform independent >>> (modulo bugs), so having a test run at least somewhere should >>> suffice. In theory. >>> >>> We could add support for the one-line-summary variant on OS X easily, >>> though. >> >> Probably, although it's looking like testing on Mac OS X won't be >> fruitful (see below). > > Can we move this topic forward by introducing a new prerequisite > ZIPINFO and used at the beginning of these tests (make it a lazy > prereq)? Run zipinfo on a trivial archive and see if its output is > something we recognize to decide if the platform supports that > ZIPINFO prerequisite and do this test only on them.
Heh, that is exactly what the patch under discussion does. So...
> After all, what _is_ being tested, i.e. our archive creation, would > not change across platforms, so having a test that runs on a known > subset of platforms is better than not having anything at all. > > Thanks.
...I'd say we can take this patch as-is, and those who want to have a working test on MacOS can come up with an enhancement to the way the script parses output from zipinfo that would also work on their platforms.
Thanks and sorry for the noise ;-)
Eric SunshineRe: [PATCH 1/3] t5004: test ZIP archives with many entries
On Fri, Aug 28, 2015 at 11:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes: >> Eric Sunshine <sunshine@sunshineco.com> writes: >>> On Sun, Aug 23, 2015 at 5:29 AM, "René Scharfe" <l.s.r@web.de> wrote: >>>> I suspected that zipinfo's output might be formatted differently on >>>> different platforms and tried to guard against it by checking for the >>>> number zero there. Git's ZIP file creation is platform independent >>>> (modulo bugs), so having a test run at least somewhere should >>>> suffice. In theory. >>>> >>>> We could add support for the one-line-summary variant on OS X easily, >>>> though. >>> >>> Probably, although it's looking like testing on Mac OS X won't be >>> fruitful (see below). >> >> Can we move this topic forward by introducing a new prerequisite >> ZIPINFO and used at the beginning of these tests (make it a lazy >> prereq)? Run zipinfo on a trivial archive and see if its output is >> something we recognize to decide if the platform supports that >> ZIPINFO prerequisite and do this test only on them. > > Heh, that is exactly what the patch under discussion does. So... > >> After all, what _is_ being tested, i.e. our archive creation, would >> not change across platforms, so having a test that runs on a known >> subset of platforms is better than not having anything at all. > > ...I'd say we can take this patch as-is, and those who want to have > a working test on MacOS can come up with an enhancement to the way > the script parses output from zipinfo that would also work on their > platforms.
Right, the new test is correctly skipped on Mac OS X and FreeBSD, so the patch is suitable as-is. We might, however, want to augment the commit message with some of the knowledge learned from this thread. Perhaps modify the last sentence of the second paragraph and then insert additional information following it, like this?
... at least provides
*some* way to check this field, although presently only on Linux. zipinfo on current Mac OS X (Yosemite 10.10.5) does not support
this field, and, when encountered, caps the printed file count at
65535 (and spits out warnings and errors), thus is not useful for
testing. (Its output also differs from zipinfo on Linux, thus
requires changes to the 'sed' recognition and extraction
expressions, but that's a minor issue.) zipinfo on FreeBSD seems to have been retired altogether in favor
of "unzip -Z", however, only in the emasculated form "unzip -Z
-1" which lists archive entries but does not provide a file
count, thus is not useful for this test.(I also snuck a s/can// fix in there for the last sentence of the second paragraph.)
René Scharfe[PATCH 2/3] archive-zip: use a local variable to store the creator version
Use a simpler conditional right next to the code which makes a higher creator version necessary -- namely symlink handling and support for executable files -- instead of a long line with a ternary operator. The resulting code has more lines but is simpler and allows reuse of the value easily.
Signed-off-by: Rene Scharfe <l.s.r@web.de> --- archive-zip.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/archive-zip.c b/archive-zip.c index ae3d67f..2a76156 100644 --- a/archive-zip.c +++ b/archive-zip.c @@ -223,6 +223,7 @@ static int write_zip_entry(struct archiver_args *args, unsigned long size; int is_binary = -1; const char *path_without_prefix = path + args->baselen; + unsigned int creator_version = 0; crc = crc32(0, NULL, 0); @@ -251,6 +252,8 @@ static int write_zip_entry(struct archiver_args *args, method = 0; attr2 = S_ISLNK(mode) ? ((mode | 0777) << 16) : (mode & 0111) ? ((mode) << 16) : 0; + if (S_ISLNK(mode) || (mode & 0111)) + creator_version = 0x0317; if (S_ISREG(mode) && args->compression_level != 0 && size > 0) method = 8; @@ -303,8 +306,7 @@ static int write_zip_entry(struct archiver_args *args, } copy_le32(dirent.magic, 0x02014b50); - copy_le16(dirent.creator_version, - S_ISLNK(mode) || (S_ISREG(mode) && (mode & 0111)) ? 0x0317 : 0); + copy_le16(dirent.creator_version, creator_version); copy_le16(dirent.version, 10); copy_le16(dirent.flags, flags); copy_le16(dirent.compression_method, method);
-- 2.5.0
René Scharfe[PATCH 3/3] archive-zip: support more than 65535 entries
Support more than 65535 entries cleanly by writing a "zip64 end of central directory record" (with a 64-bit field for the number of entries) before the usual "end of central directory record" (which contains only a 16-bit field). InfoZIP's zip does the same. Archives with 65535 or less entries are not affected.
Programs that extract all files like InfoZIP's zip and 7-Zip ignored the field and could extract all files already. Software that relies on the ZIP file directory to show a list of contained files quickly to simulate to normal directory like Windows' built-in ZIP functionality only saw a subset of the included files.
Windows supports ZIP64 since Vista according to https://en.wikipedia.org/wiki/Zip_%28file_format%29#ZIP64.
Suggested-by: Johannes Schauer <josch@debian.org> Signed-off-by: Rene Scharfe <l.s.r@web.de> --- archive-zip.c | 93 +++++++++++++++++++++++++++++++++++++++-- t/t5004-archive-corner-cases.sh | 2 +- 2 files changed, 91 insertions(+), 4 deletions(-)
diff --git a/archive-zip.c b/archive-zip.c index 2a76156..9db4735 100644 --- a/archive-zip.c +++ b/archive-zip.c @@ -16,7 +16,9 @@ static unsigned int zip_dir_size; static unsigned int zip_offset; static unsigned int zip_dir_offset; -static unsigned int zip_dir_entries; +static uint64_t zip_dir_entries; + +static unsigned int max_creator_version; #define ZIP_DIRECTORY_MIN_SIZE (1024 * 1024) #define ZIP_STREAM (1 << 3) @@ -86,6 +88,28 @@ struct zip_extra_mtime { unsigned char _end[1]; }; +struct zip64_dir_trailer { + unsigned char magic[4]; + unsigned char record_size[8]; + unsigned char creator_version[2]; + unsigned char version[2]; + unsigned char disk[4]; + unsigned char directory_start_disk[4]; + unsigned char entries_on_this_disk[8]; + unsigned char entries[8]; + unsigned char size[8]; + unsigned char offset[8]; + unsigned char _end[1]; +}; + +struct zip64_dir_trailer_locator { + unsigned char magic[4]; + unsigned char disk[4]; + unsigned char offset[8]; + unsigned char number_of_disks[4]; + unsigned char _end[1]; +}; + /* * On ARM, padding is added at the end of the struct, so a simple * sizeof(struct ...) reports two bytes more than the payload size @@ -98,6 +122,12 @@ struct zip_extra_mtime { #define ZIP_EXTRA_MTIME_SIZE offsetof(struct zip_extra_mtime, _end) #define ZIP_EXTRA_MTIME_PAYLOAD_SIZE \ (ZIP_EXTRA_MTIME_SIZE - offsetof(struct zip_extra_mtime, flags)) +#define ZIP64_DIR_TRAILER_SIZE offsetof(struct zip64_dir_trailer, _end) +#define ZIP64_DIR_TRAILER_RECORD_SIZE \ + (ZIP64_DIR_TRAILER_SIZE - \ + offsetof(struct zip64_dir_trailer, creator_version)) +#define ZIP64_DIR_TRAILER_LOCATOR_SIZE \ + offsetof(struct zip64_dir_trailer_locator, _end) static void copy_le16(unsigned char *dest, unsigned int n) { @@ -113,6 +143,31 @@ static void copy_le32(unsigned char *dest, unsigned int n) dest[3] = 0xff & (n >> 030); } +static void copy_le64(unsigned char *dest, uint64_t n) +{ + dest[0] = 0xff & n; + dest[1] = 0xff & (n >> 010); + dest[2] = 0xff & (n >> 020); + dest[3] = 0xff & (n >> 030); + dest[4] = 0xff & (n >> 040); + dest[5] = 0xff & (n >> 050); + dest[6] = 0xff & (n >> 060); + dest[7] = 0xff & (n >> 070); +} + +static uint64_t clamp_max(uint64_t n, uint64_t max, int *clamped) +{ + if (n <= max) + return n; + *clamped = 1; + return max; +} + +static void copy_le16_clamp(unsigned char *dest, uint64_t n, int *clamped) +{ + copy_le16(dest, clamp_max(n, 0xffff, clamped)); +} + static void *zlib_deflate_raw(void *data, unsigned long size, int compression_level, unsigned long *compressed_size) @@ -282,6 +337,9 @@ static int write_zip_entry(struct archiver_args *args, sha1_to_hex(sha1)); } + if (creator_version > max_creator_version) + max_creator_version = creator_version; + if (buffer && method == 8) { out = deflated = zlib_deflate_raw(buffer, size, args->compression_level, @@ -439,20 +497,49 @@ static int write_zip_entry(struct archiver_args *args, return 0; } +static void write_zip64_trailer(void) +{ + struct zip64_dir_trailer trailer64; + struct zip64_dir_trailer_locator locator64; + + copy_le32(trailer64.magic, 0x06064b50); + copy_le64(trailer64.record_size, ZIP64_DIR_TRAILER_RECORD_SIZE); + copy_le16(trailer64.creator_version, max_creator_version); + copy_le16(trailer64.version, 45); + copy_le32(trailer64.disk, 0); + copy_le32(trailer64.directory_start_disk, 0); + copy_le64(trailer64.entries_on_this_disk, zip_dir_entries); + copy_le64(trailer64.entries, zip_dir_entries); + copy_le64(trailer64.size, zip_dir_offset); + copy_le64(trailer64.offset, zip_offset); + + copy_le32(locator64.magic, 0x07064b50); + copy_le32(locator64.disk, 0); + copy_le64(locator64.offset, zip_offset + zip_dir_offset); + copy_le32(locator64.number_of_disks, 1); + + write_or_die(1, &trailer64, ZIP64_DIR_TRAILER_SIZE); + write_or_die(1, &locator64, ZIP64_DIR_TRAILER_LOCATOR_SIZE); +} + static void write_zip_trailer(const unsigned char *sha1) { struct zip_dir_trailer trailer; + int clamped = 0; copy_le32(trailer.magic, 0x06054b50); copy_le16(trailer.disk, 0); copy_le16(trailer.directory_start_disk, 0); - copy_le16(trailer.entries_on_this_disk, zip_dir_entries); - copy_le16(trailer.entries, zip_dir_entries); + copy_le16_clamp(trailer.entries_on_this_disk, zip_dir_entries, + &clamped); + copy_le16_clamp(trailer.entries, zip_dir_entries, &clamped); copy_le32(trailer.size, zip_dir_offset); copy_le32(trailer.offset, zip_offset); copy_le16(trailer.comment_length, sha1 ? GIT_SHA1_HEXSZ : 0); write_or_die(1, zip_dir, zip_dir_offset); + if (clamped) + write_zip64_trailer(); write_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE); if (sha1) write_or_die(1, sha1_to_hex(sha1), GIT_SHA1_HEXSZ); diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh index c6bd729..cca2338 100755 --- a/t/t5004-archive-corner-cases.sh +++ b/t/t5004-archive-corner-cases.sh @@ -122,7 +122,7 @@ test_lazy_prereq ZIPINFO ' test "x$n" = "x0" ' -test_expect_failure ZIPINFO 'zip archive with many entries' ' +test_expect_success ZIPINFO 'zip archive with many entries' ' # add a directory with 256 files mkdir 00 && for a in 0 1 2 3 4 5 6 7 8 9 a b c d e f
-- 2.5.0
Duy NguyenRe: bug: git-archive does not use the zip64 extension for archives with more than 16k entries
On Thu, Aug 13, 2015 at 2:40 AM, René Scharfe <l.s.r@web.de> wrote:
> Seriously, though: What kind of repository has that many files and uses the > ZIP format to distribute snapshots? Just curious.
Not the "uses the zip format" part, but at least webkit and gentoo-x86 both exceed 64k limit. Even if we don't support this case, we probably should error out instead of producing corrupt archives.
-- Duy
Eric SunshineRe: Eric Sunshine mail delivery failure
On Sun, Aug 23, 2015 at 2:36 PM, Elia Pinto <gitter.spiros@gmail.com> wrote:
> Il 23/Ago/2015 20:26, "Eric Sunshine" <sunshine@sunshineco.com> ha scritto: >> On Sun, Aug 23, 2015 at 1:16 PM, Johannes Löthberg >> <johannes@kyriasis.com> wrote: >> > Just an A record would be enough. The issue is that mail.sunshineco.com >> > has >> > neither an A nor an AAAA record, it is a CNAME to sunshineco.com, which >> > is >> > invalid according to RFC2181. >> >> Interestingly, the default configuration for all domains managed by >> this service provider is for the mailhost to be a CNAME. While the >> restriction in section 10.3 of RFC2181 makes sense as a way to avoid >> extra "network burden", in practice, email services seem to be pretty >> relaxed about it, and follow the CNAME indirection as needed. >> >> I suppose it's possible that web.de is being extra strict (although it >> seems that such strictness would be painful for its users), or this >> could just be a temporary DNS lookup failure. It's hard to tell based >> upon the errors René reported. >> >> I did change the CNAME to an A just in case, though who knows how long >> it will take for the change to propagate over to web.de's server. > Anyone can check Here https://dnschecker.org/#CNAME/Mail.sunshineco.com > It would fail with your change
Interesting service; thanks for the pointer. However, since it's just querying a random set of DNS servers, it's not necessarily indicative of whether the change has actually propagated to the DNS server(s) answering web.de's mail server's queries. Local configuration (TTL's, etc.) on those servers or anywhere in between, as well as network conditions, could impact propagation to an unknown degree.
Eric SunshineRe: Eric Sunshine mail delivery failure
On Sun, Aug 23, 2015 at 2:48 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Aug 23, 2015 at 2:36 PM, Elia Pinto <gitter.spiros@gmail.com> wrote: >> Il 23/Ago/2015 20:26, "Eric Sunshine" <sunshine@sunshineco.com> ha scritto: >>> I did change the CNAME to an A just in case, though who knows how long >>> it will take for the change to propagate over to web.de's server. >> Anyone can check Here https://dnschecker.org/#CNAME/Mail.sunshineco.com >> It would fail with your change > > Interesting service; thanks for the pointer. However, since it's just > querying a random set of DNS servers, it's not necessarily indicative > of whether the change has actually propagated to the DNS server(s) > answering web.de's mail server's queries. Local configuration (TTL's, > etc.) on those servers or anywhere in between, as well as network > conditions, could impact propagation to an unknown degree.
Also, the propagation time of the A record can be quite different from the point at which the CNAME record finally expires (based upon its TTL, which may differ dramatically from server to server), so the above CNAME query may continue to succeed long after the A record has propagated.