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

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

[Johannes Schauer]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

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

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAABCAAGBQJVydE4AAoJEPLLpcePvYPhlV8QAJ5mAaqOTmJNvpb1hnlms9n8
1gRDKDi6zT0CYz3eJfPcuL31FNQ3u7fIsiyix2O+TEz6tzkPKSI7yCOA/B0gFDjs
Oy7srAFlpSHoTpTPfB40ANWDzCbN60YrfCjti1egnSz8qOa/VnZrHPpLQTNXE9eM
nmdZLWTylDeUjQvmfHgd8SuL4pfi2adSfo6duEgWhV5kMuQVN1SU8jVTm8vl+kbx
8KzY2bG5H+IXGC3wKXi/v6/e/1odRULS5j/4JDzfycD2+FHi4T4g0HZ9JKLsW7Jq
3y2qYA3WZddcR5rwEgiv81WP8utP1b/Hw/nRfrLUfqPKwZoIHtqwTxyMgytIprLi
HJZ7kvGjTRLMHJfXZ2N3EpE3aQBzJy5v1Pg4nSVh3GLgTvQf43vLAgoT64bpE/iz
oqoMC5fOAPJsPir4oZStXWa8tUmlKWHWP7otMsV3yv16FSW7F/9PZSPfBf2gNWMk
K+zInaDbaxSgPkU52JSc8MRhxjT5rL+p9cDv7kkMITquNkCMtjZ8F9x5yfW7uz4H
UE1qPRV5tLbizbgzkXH6HLyNCYSVPM7iSow10dZ4ZYVdgr8abnORZumCTZtus+rl
zqFvrSEal5m2UFYV/7Jw6ZA2D1MMcMjlSt57Hw8uXVhKDpEeR0QkQCVMPnRi5RBo
/o9kMU+joF8YjNYo4u9m
=ncn0
-----END PGP SIGNATURE-----

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

[René Scharfe]

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

[Johannes Schauer]

[-- Attachment #1: Type: text/plain, Size: 2435 bytes --]

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

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAABCAAGBQJVzAApAAoJEPLLpcePvYPhLUEP/i3r+wbhhHBrapMjLLeIupJy
4I7fItTXxzSWyX8g8MH1RLdd/ckKJ0zsMQZTMtSbKwjEatE56ChvEvm2PIadX8Qg
DDn51mTFABDpPaItl8IxSmMVDzHePnkzTnFRbZzFC/jaCgm2Rb4AGCr9GTTvvlNi
37XQ/YyE2af2WiGtn2TLGO8ZamHaxoov4n9DYBEcgi9yUVg3CUvL5+Reb7spf0sI
PEOMkeuLmaBKDQsb4N4IPz6c0vTGgvfe9rmD4dtUKuB0R/7S2Tz366IM1zuypW3y
Q3hg53Pq2OaXtB5ukYYQ3mi5/jEyUIciF8mklbXfTx+vaaPoFXUwEgjJde+49WOl
lHfIpqk1w2WqYyzgwgf328of7lUeEh3ABhFdpAdNPOYf2amSUAy+xl614cu4Y/ld
wcoKn28H2pspfiux/SPljVbo9xhQbkd2Po8p0J2lLVhwXqjJv5MLBITPUSH0sZRL
MuI+1wNpOiQaIZsaxyiSmNUpMVdmeb2ZHUw38jkKCcpvYhtfE3ItCDs8R28s6bP8
yUEWXVrZk1hKmID1plLkCPAc5sHSZJdA7n66xv8ZdTyWHbykto45FPY6Kt0Lc6/q
B+riiP6rEuX6TeUnOvHc3lQ49lwGXfBrRFJbYvY/XQs99izD/Xp9o0SnJ3p9InC4
xPYEMc1IsRSxfssr0wNa
=6mn4
-----END PGP SIGNATURE-----

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

[Duy Nguyen]

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

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

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

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

[René Scharfe]

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

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

[René Scharfe]

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

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

[Eric Sunshine]

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

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

["René Scharfe"]

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é

Eric Sunshine mail delivery failure

[René Scharfe]

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

René

Re: Eric Sunshine mail delivery failure

[Johannes Löthberg]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

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/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

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

[Eric Sunshine]

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

Re: Eric Sunshine mail delivery failure

[Eric Sunshine]

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.

Re: Eric Sunshine mail delivery failure

[Eric Sunshine]

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.

Re: Eric Sunshine mail delivery failure

[Eric Sunshine]

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.

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

[Eric Sunshine]

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