Skip to contentSkip to conversation

bug: git-archive does not use the zip64 extension for archives with more than 16k entries

PATCH In review Johannes Schauer 6 participants 18 messages Jun 15, 2016 1 message not in the archive

lore
Share & tools
lore

Johannes Schauer

author
#0 lore raw

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

#1 lore raw

Am 11.08.2015 um 12:40 schrieb Johannes Schauer:

Quoted 6 lines

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.

Quoted 2 lines

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é

author
#2 lore raw

Hi,

Quoting René Scharfe (2015-08-12 21:40:48)

Quoted 8 lines

Am 11.08.2015 um 12:40 schrieb Johannes Schauer:

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 1 line

Besides, 64K entries should be enough for anybody. ;-)

:P

Quoted 2 lines

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.

Quoted 6 lines
Quoted 2 lines

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

#3 lore raw

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 <[email protected]>
---
 t/t5004-archive-corner-cases.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
t/t5004-archive-corner-cases.sh +40 -0
index 654adda..c6bd729 100755
@@ -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
Signature · 1 line
2.5.0
#4 lore raw

Re: [PATCH 1/3] t5004: test ZIP archives with many entries

Quoted 22 lines · René Scharfe

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 <[email protected]>

---

t/t5004-archive-corner-cases.sh +0 -0
index 654adda..c6bd729 100755

t/t5004-archive-corner-cases.sh test_expect_success 'archive empty subtree by direct pathspec' ' · line 123

Quoted 9 lines
t/t5004-archive-corner-cases.sh +6 -0
@@ -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...

Quoted 20 lines
t/t5004-archive-corner-cases.sh +20 -0
+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"

t/t5004-archive-corner-cases.sh test_expect_success 'archive empty subtree by direct pathspec' ' · line 154

Quoted 10 lines
t/t5004-archive-corner-cases.sh +10 -0
+               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
    $
Quoted 6 lines
t/t5004-archive-corner-cases.sh +3 -1
+       test_cmp expect actual
+'
+
 test_done
--

2.5.0

#5 lore raw

Re: [PATCH 1/3] t5004: test ZIP archives with many entries

Am 23.08.2015 um 07:54 schrieb Eric Sunshine:

Quoted 28 lines · René Scharfe
Quoted 5 lines
t/t5004-archive-corner-cases.sh +0 -0
index 654adda..c6bd729 100755

t/t5004-archive-corner-cases.sh test_expect_success 'archive empty subtree by direct pathspec' ' · line 123

Quoted 9 lines
t/t5004-archive-corner-cases.sh +6 -0
@@ -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:

Quoted 7 lines
     $ 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.

Quoted 16 lines

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:

Quoted 7 lines
     $ 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%

Quoted 17 lines

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)
Quoted 6 lines

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é

#6 lore raw

Eric Sunshine mail delivery failure

Eric, hope you see this reply on the list. Direct replies to [email protected] 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:

"[email protected]":
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. :-(

René

#7 lore raw

Re: Eric Sunshine mail delivery failure

Quoted 11 lines · René Scharfe

Eric, hope you see this reply on the list. Direct replies to [email protected] 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:

Quoted 6 lines

"[email protected]":
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.

Signature · 4 lines
Sincerely,
 Johannes Löthberg
 PGP Key ID: 0x50FB9B273A9D0BB5
 https://theos.kyriasis.com/~kyrias/
#8 lore raw

Re: Eric Sunshine mail delivery failure

Quoted 17 lines · Johannes Löthberg
Quoted 16 lines · René Scharfe

Eric, hope you see this reply on the list. Direct replies to [email protected] 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:

"[email protected]":
no valid MX hosts found

Quoted 6 lines
Quoted 2 lines

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.

A message that is not in the archive (sent off-list or not yet received): CA+EOSBmk2cdQe3owaXgkYAgTZqpUFa=J8g5FYq28-=VhDcJ4EA@mail.gmail.com

#10 lore raw

Re: Eric Sunshine mail delivery failure

Quoted 20 lines · Elia Pinto

Il 23/Ago/2015 20:26, "Eric Sunshine" <[email protected]> ha scritto:

Quoted 19 lines
Quoted 5 lines · Johannes Löthberg

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.

Quoted 4 lines
Quoted 2 lines

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.

#11 lore raw

Re: Eric Sunshine mail delivery failure

Quoted 7 lines · Eric Sunshine
Quoted 5 lines · Elia Pinto

Il 23/Ago/2015 20:26, "Eric Sunshine" <[email protected]> ha scritto:

Quoted 2 lines

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

Quoted 6 lines

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.

#12 lore raw

Re: [PATCH 1/3] t5004: test ZIP archives with many entries

In reply to René Scharfe
Quoted 27 lines · René Scharfe

Am 23.08.2015 um 07:54 schrieb Eric Sunshine:

Quoted 18 lines · René Scharfe

t/t5004-archive-corner-cases.sh test_expect_success 'archive empty subtree by direct pathspec' ' · line 123

Quoted 4 lines
t/t5004-archive-corner-cases.sh +4 -0
+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:

Quoted 7 lines
     $ 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).

Quoted 8 lines

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).

Quoted 21 lines
Quoted 21 lines

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
     $
Quoted 7 lines
Quoted 2 lines

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...

Quoted 26 lines

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%

Quoted 19 lines

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:

Quoted 7 lines
Quoted 4 lines
     $ 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.

Quoted 8 lines

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
    ...
#13 lore raw

Re: [PATCH 1/3] t5004: test ZIP archives with many entries

Quoted 7 lines · Eric Sunshine
Quoted 6 lines · René Scharfe

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.

Quoted 5 lines
Quoted 2 lines

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.

#14 lore raw

Re: [PATCH 1/3] t5004: test ZIP archives with many entries

Quoted 12 lines · Junio C Hamano
Quoted 10 lines · Eric Sunshine
Quoted 8 lines · René Scharfe

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.

Quoted 8 lines
Quoted 2 lines

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...

Quoted 5 lines

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 ;-)

#15 lore raw

Re: [PATCH 1/3] t5004: test ZIP archives with many entries

Quoted 23 lines · Junio C Hamano
Quoted 11 lines · Junio C Hamano
Quoted 10 lines · Eric Sunshine
Quoted 8 lines · René Scharfe

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.

Quoted 8 lines
Quoted 2 lines

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...

Quoted 8 lines
Quoted 3 lines

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.)

#16 lore raw

PATCH 2/3 archive-zip: use a local variable to store the creator version

In reply to Johannes Schauer

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 <[email protected]>
---
 archive-zip.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
archive-zip.c +4 -2
index ae3d67f..2a76156 100644
@@ -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);
Signature · 1 line
2.5.0
#17 lore raw

PATCH 3/3 archive-zip: support more than 65535 entries

In reply to Johannes Schauer

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 <[email protected]>
Signed-off-by Rene Scharfe <[email protected]>
---
 archive-zip.c                   | 93 +++++++++++++++++++++++++++++++++++++++--
 t/t5004-archive-corner-cases.sh |  2 +-
 2 files changed, 91 insertions(+), 4 deletions(-)
archive-zip.c +90 -3
index 2a76156..9db4735 100644
@@ -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);
t/t5004-archive-corner-cases.sh +1 -1
index c6bd729..cca2338 100755
@@ -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
Signature · 1 line
2.5.0
#18 lore raw
In reply to René Scharfe
Quoted 2 lines · René Scharfe

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.

Signature · 1 line
Duy