Reliably Reproducible Bad Packing of Objects

Hi,

when run on a 32 bit system (Linux system, don't know about other systems),

mkdir test && cd test && git init && touch someFile && git add someFile && git commit -m "Initial commit." && dd if=/dev/urandom of=bigBinaryFile bs=1MB count=4294 && git add bigBinaryFile && git commit -m "Introduced big biary file."

reliably produces this error message: "error: bad packed object CRC for"

Since git counts sizes in byte and uses ints for it, it can't handle files bigger than (2^32 - 1) byte. That's 4'294.967296 MB. If you give git a file bigger than it can handle, it usually just says "fatal:
Cannot handle files this big" without corrupting the repository. Btw.:
It'd be nice if the error message stated that this only occurs on 32 bit system and only with files 4 GiB in size or bigger.

To provoke the bug, the commands above creates a file which cannot be compressed slightly less than (2^32 - 1) byte big, probably resulting in a commit more than (2^32 - 1) byte big.

I was able to reproduce the bug on a Raspberry Pi Model 3 (ARMv7 CPU) and a virtual machine running Ubuntu 16.04 32 bit (which was specifically set up to test this, so it was a clean installation) on a host running Ubuntu 16.04 64 bit on an ARM 64 bit x86 CPU (i7-4720HQ).

Output on raspi: https://gist.github.com/m1cm1c/d874f03be5b12cbd8b86ced
79fa456d1
Output on virt. machine: https://gist.github.com/m1cm1c/d0dd47828386bb0
f1e001b9f750416e0

Note that on the raspi I concatenated git gc to the end to show that the object exists but is corrupt but you can already see that something went wrong before git gc is executed.

If you look at the output on the virtual machine, however, you will not see that something wrong until you read the part where I typed in git gc which I find particularly worrying.

When checking whether you get the same result, please make sure to use git 1.7.6 or newer. If you use an older version of git (older versions are still being distributed, for example for Ubuntu 14.04), git will try to memory-map a too big file resulting in a different error.

With kind regards
Christoph Michelbach

I tried with a 32-bit git build on 64-bit system and did not see this message. Maybe it's because the 64-bit system. Just too lazy to bring up a 32-bit VM to process 4GB of data.

But I did look around a bit, and the problem is probably number truncation. Could you try this patch?

-- 8< --

index d5e1121..cb571ac 100644
@@ -2281,7 +2281,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
 
 		if (do_check_packed_object_crc && p->index_version > 1) {
 			struct revindex_entry *revidx = find_pack_revindex(p, obj_offset);
-			unsigned long len = revidx[1].offset - obj_offset;
+			off_t len = revidx[1].offset - obj_offset;
 			if (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {
 				const unsigned char *sha1 =
 					nth_packed_object_sha1(p, revidx->nr);

-- 8< --

If your system has large file support (you must or things must have failed badly from the beginning), then both offsets in that subtraction expression are of type off_t, aka 64-bit. However "len" would be 32-bit and truncation can happen. check_pack_crc() takes off_t too, so it seems well prepared already (but I didn't test).

PS. There's a similar statement in pack-objects.c, but I don't think you have done anything to trigger it. OK, 'git gc' may.. --
Duy

Just FYI. There may be more truncation problems on 32-bit systems. "clang -Wshorten-64-to-32" reports a couple of spots that could be real problems (pack-objects, index-pack...) for handling 4+ GB files on these systems. Still working on it...

Duy

Since I now could reproduce the problem that Christoph showed, I decided to send the good patches out. To sum up, we use "unsigned long" in some places related to file size. On 32-bit systems, it's limited to 32 bits even though the system can handle files larger than that (off_t is 64-bit). This fixes it.

clang -Wshorten-64-to-32 is very helpful to spot these problems. I have a couple more patches to clean all these warnings, but some need more code study to see what is the right way to do.

Most of the rest seems harmless, except for the local variable "size" in builtin/pack-objects.c:write_one(). I might send 6/5 for that one.

Nguyễn Thái Ngọc Duy (5):
pack-objects: pass length to check_pack_crc() without truncation sha1_file.c: use type off_t* for object_info->disk_sizep
index-pack: correct "len" type in unpack_data()
index-pack: report correct bad object offsets even if they are large
index-pack: correct "offset" type in unpack_entry_data()

 builtin/cat-file.c     |  4 ++--
 builtin/index-pack.c   | 23 ++++++++++++-----------
 builtin/pack-objects.c |  2 +-
 cache.h                |  2 +-
 sha1_file.c            |  2 +-
 5 files changed, 17 insertions(+), 16 deletions(-)

2.8.2.537.g0965dd9

On 32 bit systems with large file support, unsigned long is 32-bit while the two offsets in the subtraction expression (pack-objects has the exact same expression as in sha1_file.c but not shown in diff) are in 64-bit. If an in-pack object is larger than 2^32 len/datalen is truncated and we get a misleading "error: bad packed object CRC for ..." as a result.

Use off_t for len and datalen. check_pack_crc() already accepts this argument as off_t and can deal with 4+ GB.

---
 builtin/pack-objects.c | 2 +-
 sha1_file.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 8f5e358..df6ecd5 100644
@@ -349,7 +349,7 @@ static unsigned long write_reuse_object(struct sha1file *f, struct object_entry
 	struct revindex_entry *revidx;
 	off_t offset;
 	enum object_type type = entry->type;
-	unsigned long datalen;
+	off_t datalen;
 	unsigned char header[10], dheader[10];
 	unsigned hdrlen;
 

index d5e1121..cb571ac 100644
@@ -2281,7 +2281,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
 
 		if (do_check_packed_object_crc && p->index_version > 1) {
 			struct revindex_entry *revidx = find_pack_revindex(p, obj_offset);
-			unsigned long len = revidx[1].offset - obj_offset;
+			off_t len = revidx[1].offset - obj_offset;
 			if (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {
 				const unsigned char *sha1 =
 					nth_packed_object_sha1(p, revidx->nr);

2.8.2.537.g0965dd9

unpack_entry_data() receives an off_t value from unpack_raw_entry(), which could be larger than unsigned long on 32-bit systems with large file support. Correct the type so truncation does not happen. This only affects bad object reporting though.

---
 builtin/index-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 73f7cd2..f35a9dc 100644
@@ -430,7 +430,7 @@ static int is_delta_type(enum object_type type)
 	return (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);
 }
 
-static void *unpack_entry_data(unsigned long offset, unsigned long size,
+static void *unpack_entry_data(off_t offset, unsigned long size,
 			       enum object_type type, unsigned char *sha1)
 {
 	static char fixed_buf[8192];

2.8.2.537.g0965dd9

Use the right type for offsets in this case, off_t, which makes a difference on 32-bit systems with large file support, and change formatting code accordingly.

---
 builtin/index-pack.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index cafaab7..73f7cd2 100644
@@ -338,10 +338,10 @@ static void parse_pack_header(void)
 	use(sizeof(struct pack_header));
 }
 
-static NORETURN void bad_object(unsigned long offset, const char *format,
+static NORETURN void bad_object(off_t offset, const char *format,
 		       ...) __attribute__((format (printf, 2, 3)));
 
-static NORETURN void bad_object(unsigned long offset, const char *format, ...)
+static NORETURN void bad_object(off_t offset, const char *format, ...)
 {
 	va_list params;
 	char buf[1024];

@@ -349,7 +349,8 @@ static NORETURN void bad_object(unsigned long offset, const char *format, ...)
 	va_start(params, format);
 	vsnprintf(buf, sizeof(buf), format, params);
 	va_end(params);
-	die(_("pack has bad object at offset %lu: %s"), offset, buf);
+	die(_("pack has bad object at offset %"PRIiMAX": %s"),
+	    (intmax_t)offset, buf);
 }
 
 static inline struct thread_local *get_thread_data(void)

2.8.2.537.g0965dd9

This field, filled by sha1_object_info() contains the on-disk size of an object, which could go over 4GB limit of unsigned long on 32-bit systems. Use off_t for it instead and update all callers.

---
 builtin/cat-file.c | 4 ++--
 cache.h            | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

index 618103f..5b34bd0 100644
@@ -131,7 +131,7 @@ struct expand_data {
 	unsigned char sha1[20];
 	enum object_type type;
 	unsigned long size;
-	unsigned long disk_size;
+	off_t disk_size;
 	const char *rest;
 	unsigned char delta_base_sha1[20];
 

@@ -191,7 +191,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 		if (data->mark_query)
 			data->info.disk_sizep = &data->disk_size;
 		else
-			strbuf_addf(sb, "%lu", data->disk_size);
+			strbuf_addf(sb, "%"PRIuMAX, data->disk_size);
 	} else if (is_atom("rest", atom, len)) {
 		if (data->mark_query)
 			data->split_on_whitespace = 1;

index c73becb..a4465cb 100644
@@ -1508,7 +1508,7 @@ struct object_info {
 	/* Request */
 	enum object_type *typep;
 	unsigned long *sizep;
-	unsigned long *disk_sizep;
+	off_t *disk_sizep;
 	unsigned char *delta_base_sha1;
 	struct strbuf *typename;
 

2.8.2.537.g0965dd9

On 32-bit systems with large file support, one entry could be larger than 4GB and overflow "len". Correct it so we can unpack a full entry.

---
 builtin/index-pack.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index e8c71fc..cafaab7 100644
@@ -549,13 +549,13 @@ static void *unpack_data(struct object_entry *obj,
 			 void *cb_data)
 {
 	off_t from = obj[0].idx.offset + obj[0].hdr_size;
-	unsigned long len = obj[1].idx.offset - from;
+	off_t len = obj[1].idx.offset - from;
 	unsigned char *data, *inbuf;
 	git_zstream stream;
 	int status;
 
 	data = xmallocz(consume ? 64*1024 : obj->size);
-	inbuf = xmalloc((len < 64*1024) ? len : 64*1024);
+	inbuf = xmalloc((len < 64*1024) ? (int)len : 64*1024);
 
 	memset(&stream, 0, sizeof(stream));
 	git_inflate_init(&stream);

@@ -563,15 +563,15 @@ static void *unpack_data(struct object_entry *obj,
 	stream.avail_out = consume ? 64*1024 : obj->size;
 
 	do {
-		ssize_t n = (len < 64*1024) ? len : 64*1024;
+		ssize_t n = (len < 64*1024) ? (ssize_t)len : 64*1024;
 		n = xpread(get_thread_data()->pack_fd, inbuf, n, from);
 		if (n < 0)
 			die_errno(_("cannot pread pack file"));
 		if (!n)
-			die(Q_("premature end of pack file, %lu byte missing",
-			       "premature end of pack file, %lu bytes missing",
-			       len),
-			    len);
+			die(Q_("premature end of pack file, %"PRIuMAX" byte missing",
+			       "premature end of pack file, %"PRIuMAX" bytes missing",
+			       (unsigned int)len),
+			    (uintmax_t)len);
 		from += n;
 		len -= n;
 		stream.next_in = inbuf;

2.8.2.537.g0965dd9

Thank you very much!

With kind regards
Christoph Michelbach

Am 05.07.2016 um 19:05 schrieb Nguyễn Thái Ngọc Duy:

What can I say, gettext is smart. In gc.c we already have this

die(_("gc is already running on machine '%s' pid %"PRIuMAX" (use --force if not)"), name, (uintmax_t)pid);

and vi.po shows

#: builtin/gc.c:397
#, c-format
msgid ""
"gc is already running on machine '%s' pid %<PRIuMAX> (use --force if not)"

Duy

And translators are expcted to keep "%<PRIuMAX>" in their translated .po files, and whatever translates .po into .mo knows what %<PRIuMAX> should be mapped to?

That is surprising.

On a related but not surprising tangent, I see this example in gettext.info that may be relevant.

About larger integer types, such as ‘uintmax_t’ or ‘unsigned long
long’: they can be handled by reducing the value to a range that fits in an ‘unsigned long’. Simply casting the value to ‘unsigned long’ would not do the right thing, since it would treat ‘ULONG_MAX + 1’ like zero, ‘ULONG_MAX + 2’ like singular, and the like. Here you can exploit the fact that all mentioned plural form formulas eventually become periodic, with a period that is a divisor of 100 (or 1000 or 1000000). So, when you reduce a large value to another one in the range [1000000, 1999999] that ends in the same 6 decimal digits, you can assume that it will lead to the same plural form selection. This code does this:

#include <inttypes.h>
uintmax_t nbytes = ...;
printf (ngettext ("The file has %"PRIuMAX" byte.",
"The file has %"PRIuMAX" bytes.", (nbytes > ULONG_MAX
? (nbytes % 1000000) + 1000000 : nbytes)),
nbytes);

gettext.info answered me ;-)

The ‘gettext’ tools and library have special support for these ‘<inttypes.h>’ macros. You can therefore simply write

printf (gettext ("The amount is %0" PRId64 "\n"), number);

The PO file will contain the string "The amount is %0<PRId64>\n". The translators will provide a translation containing "%0<PRId64>" as well, and at runtime the ‘gettext’ function’s result will contain the appropriate constant string, "d" or "ld" or "lld".

This works only for the predefined ‘<inttypes.h>’ macro...

Nice.

I now created a repo on a 64 bit system (same command as before), then duplicated it. One copy I applied git gc to on the 64 bit host system, the other copy I gave to my 32 bit virtual machine to apply git gc to it.

The 64 bit host uses git from the Ubuntu repositories, the 32 bit virtual machine uses git 2.9 from github with the patches applied.

git gc worked without problems on the host but I got

frank@frank-virtual-16-04-32-bit:~/g$ git gc
Counting objects: 6, done.
Compressing objects: 100% (3/3), done.
error: bad packed object CRC for
f595ad71c1a1ecc312ddcb32a84a4bfc4a2ed1c8
Writing objects: 100% (6/6), done.
Total 6 (delta 0), reused 0 (delta 0)
error: failed to validate delta base reference at offset 342896 from
.git/objects/pack/pack-630b5a3f28cd9d02a546462bf0d0bc640ffde784.pack
error: bad offset for revindex
error: failed to read object 4246d27f8e0149d45687b0cc23bc29a67f1f0c79
at offset 342887 from .git/objects/pack/pack-
630b5a3f28cd9d02a546462bf0d0bc640ffde784.pack
fatal: packed object 4246d27f8e0149d45687b0cc23bc29a67f1f0c79 (stored
in .git/objects/pack/pack-
630b5a3f28cd9d02a546462bf0d0bc640ffde784.pack) is corrupt
error: failed to run prune
frank@frank-virtual-16-04-32-bit:~/g$ 

on the virtual machine.

Not including the mailing list in CC wasn't intended.

With kind regards
Christoph Michelbach

Yup. So far I can only say that pack-objects generates incorrect pack :-( I hope it's the possible 6/5 patch I mentioned earlier but I'm not sure. Will continue tomorrow.

Duy

This truncated "size" produces incorrect .idx file and makes everybody else fail to read the (correct) .pack file. So 6/5 is coming shortly.

I also add 7/5 to be able to run fsck on 32-bit systems, but 7/5 could help improve performance a bit on 64-bit systems too.

Duy

A typical diff will not show what's going on and you need to see full functions. The core code is like this, at the end of of write_one()

	e->idx.offset = *offset;
	size = write_object(f, e, *offset);
	if (!size) {
		e->idx.offset = recursing;
		return WRITE_ONE_BREAK;
	}
	written_list[nr_written++] = &e->idx;

	/* make sure off_t is sufficiently large not to wrap */
	if (signed_add_overflows(*offset, size))
		die("pack too large for current definition of off_t");
	*offset += size;

Here we can see that the in-pack object size is returned by write_object (or indirectly by write_reuse_object). And it's used to calculate object offsets, which end up in the pack index file, generated at the end.

If "size" overflows (on 32-bit sytems, unsigned long is 32-bit while off_t can be 64-bit), we got wrong offsets and produce incorrect .idx file, which may make it look like the .pack file is corrupted.

---
 builtin/pack-objects.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

index df6ecd5..f854ca4 100644
@@ -341,8 +341,8 @@ static unsigned long write_no_reuse_object(struct sha1file *f, struct object_ent
 }
 
 /* Return 0 if we will bust the pack-size limit */
-static unsigned long write_reuse_object(struct sha1file *f, struct object_entry *entry,
-					unsigned long limit, int usable_delta)
+static off_t write_reuse_object(struct sha1file *f, struct object_entry *entry,
+				unsigned long limit, int usable_delta)
 {
 	struct packed_git *p = entry->in_pack;
 	struct pack_window *w_curs = NULL;

@@ -415,11 +415,12 @@ static unsigned long write_reuse_object(struct sha1file *f, struct object_entry
 }
 
 /* Return 0 if we will bust the pack-size limit */
-static unsigned long write_object(struct sha1file *f,
-				  struct object_entry *entry,
-				  off_t write_offset)
+static off_t write_object(struct sha1file *f,
+			  struct object_entry *entry,
+			  off_t write_offset)
 {
-	unsigned long limit, len;
+	unsigned long limit;
+	off_t len;
 	int usable_delta, to_reuse;
 
 	if (!pack_to_stdout)

@@ -491,7 +492,7 @@ static enum write_one_status write_one(struct sha1file *f,
 				       struct object_entry *e,
 				       off_t *offset)
 {
-	unsigned long size;
+	off_t size;
 	int recursing;
 
 	/*

2.8.2.537.g0965dd9

For blobs, we want to make sure the on-disk data is not corrupted (i.e. can be inflated and produce the expected SHA-1). Blob content is opaque, there's nothing else inside to check for.

For really large blobs, we may want to avoid unpacking the entire blob in memory, just to check whether it produces the same SHA-1. On 32-bit systems, we may not have enough virtual address space for such memory allocation. And even on 64-bit where it's not a problem, allocating a lot more memory could result in kicking other parts of systems to swap file, generating lots of I/O and slowing everything down.

For this particular operation, not unpacking the blob and letting check_sha1_signature, which supports streaming interface, do the job is sufficient. check_sha1_signature() is not shown in the diff, unfortunately. But if will be called when "data_valid && !data" is false.

We will call the callback function "fn" with NULL as "data". The only callback of this function is fsck_obj_buffer(), which does not touch "data" at all if it's a blob.

---
 pack-check.c | 15 +++++++++++++--
 pack.h       |  1 +
 2 files changed, 14 insertions(+), 2 deletions(-)

index 1da89a4..0777766 100644
@@ -105,6 +105,8 @@ static int verify_packfile(struct packed_git *p,
 		void *data;
 		enum object_type type;
 		unsigned long size;
+		off_t curpos;
+		int data_valid = 0;
 
 		if (p->index_version > 1) {
 			off_t offset = entries[i].offset;

@@ -116,8 +118,17 @@ static int verify_packfile(struct packed_git *p,
 					    sha1_to_hex(entries[i].sha1),
 					    p->pack_name, (uintmax_t)offset);
 		}
-		data = unpack_entry(p, entries[i].offset, &type, &size);
-		if (!data)
+
+		curpos = entries[i].offset;
+		type = unpack_object_header(p, w_curs, &curpos, &size);
+		unuse_pack(w_curs);
+
+		if (type != OBJ_BLOB || size < big_file_threshold) {
+			data = unpack_entry(p, entries[i].offset, &type, &size);
+			data_valid = 1;
+		}
+
+		if (data_valid && !data)
 			err = error("cannot unpack %s from %s at offset %"PRIuMAX"",
 				    sha1_to_hex(entries[i].sha1), p->pack_name,
 				    (uintmax_t)entries[i].offset);

index 3223f5a..0e77429 100644
@@ -74,6 +74,7 @@ struct pack_idx_entry {
 
 
 struct progress;
+/* Note, the data argument could be NULL if object type is blob */
 typedef int (*verify_fn)(const unsigned char*, enum object_type, unsigned long, void*, int*);
 
 extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, const struct pack_idx_option *, const unsigned char *sha1);

2.8.2.537.g0965dd9

Thanks.

That reasoning is sound, I would think, but...

... this may be a bit too magical that the assumption needs to be left in an in-code comment to warn people against temptation to using "data" over there, not just where the type of verify_fn is declared. Essentially, you are changing the expectation from existing functions of that type.

All looked nicely done. I'll queue with a few SQUASH??? (please ack or reject them) in between on 'pu' and push the result out later today.

Thanks.

But not quite correct yet ;-)

Here is what I'd propose to squash in. Even though data_valid protects the above if() condition from accessing an otherwise uninitialized "data", the call to check_sha1_signature() we have later will get noticed by the compiler that "data" is passed uninitialized.

More importantly, uninitialized data passed may be non-NULL, in which case it would not trigger the streaming interface.

 pack-check.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index 0777766..ac263fd 100644
@@ -123,7 +123,14 @@ static int verify_packfile(struct packed_git *p,
 		type = unpack_object_header(p, w_curs, &curpos, &size);
 		unuse_pack(w_curs);
 
-		if (type != OBJ_BLOB || size < big_file_threshold) {
+		if (type == OBJ_BLOB && big_file_threshold <= size) {
+			/*
+			 * Let check_sha1_signature() to check it with
+			 * the streaming interface; no point slurping
+			 * the data in-core only to discard.
+			 */
+			data = NULL;
+		} else {
 			data = unpack_entry(p, entries[i].offset, &type, &size);
 			data_valid = 1;
 		}

With 1-7/5 it appears t1050 is broken?

Oops, sorry I focused on testing big repos and forgot the test suite. But hehe.. it's a good thing. That test was added by 735efde, where fsck could keep on going after failing to inflate some blobs (instead of hitting malloc failure and abort). Now it's even better, fsck passes. I'll fix that up and resend with your squashes in.

Duy

Use the right type for offsets in this case, off_t, which makes a difference on 32-bit systems with large file support, and change formatting code accordingly.

---
 builtin/index-pack.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index cafaab7..e2d8ae4 100644
@@ -338,10 +338,10 @@ static void parse_pack_header(void)
 	use(sizeof(struct pack_header));
 }
 
-static NORETURN void bad_object(unsigned long offset, const char *format,
+static NORETURN void bad_object(off_t offset, const char *format,
 		       ...) __attribute__((format (printf, 2, 3)));
 
-static NORETURN void bad_object(unsigned long offset, const char *format, ...)
+static NORETURN void bad_object(off_t offset, const char *format, ...)
 {
 	va_list params;
 	char buf[1024];

@@ -349,7 +349,8 @@ static NORETURN void bad_object(unsigned long offset, const char *format, ...)
 	va_start(params, format);
 	vsnprintf(buf, sizeof(buf), format, params);
 	va_end(params);
-	die(_("pack has bad object at offset %lu: %s"), offset, buf);
+	die(_("pack has bad object at offset %"PRIuMAX": %s"),
+	    (uintmax_t)offset, buf);
 }
 
 static inline struct thread_local *get_thread_data(void)

2.9.1.564.gb2f7278

On 32-bit systems with large file support, one entry could be larger than 4GB and overflow "len". Correct it so we can unpack a full entry.

---
 builtin/index-pack.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index e8c71fc..cafaab7 100644
@@ -549,13 +549,13 @@ static void *unpack_data(struct object_entry *obj,
 			 void *cb_data)
 {
 	off_t from = obj[0].idx.offset + obj[0].hdr_size;
-	unsigned long len = obj[1].idx.offset - from;
+	off_t len = obj[1].idx.offset - from;
 	unsigned char *data, *inbuf;
 	git_zstream stream;
 	int status;
 
 	data = xmallocz(consume ? 64*1024 : obj->size);
-	inbuf = xmalloc((len < 64*1024) ? len : 64*1024);
+	inbuf = xmalloc((len < 64*1024) ? (int)len : 64*1024);
 
 	memset(&stream, 0, sizeof(stream));
 	git_inflate_init(&stream);

@@ -563,15 +563,15 @@ static void *unpack_data(struct object_entry *obj,
 	stream.avail_out = consume ? 64*1024 : obj->size;
 
 	do {
-		ssize_t n = (len < 64*1024) ? len : 64*1024;
+		ssize_t n = (len < 64*1024) ? (ssize_t)len : 64*1024;
 		n = xpread(get_thread_data()->pack_fd, inbuf, n, from);
 		if (n < 0)
 			die_errno(_("cannot pread pack file"));
 		if (!n)
-			die(Q_("premature end of pack file, %lu byte missing",
-			       "premature end of pack file, %lu bytes missing",
-			       len),
-			    len);
+			die(Q_("premature end of pack file, %"PRIuMAX" byte missing",
+			       "premature end of pack file, %"PRIuMAX" bytes missing",
+			       (unsigned int)len),
+			    (uintmax_t)len);
 		from += n;
 		len -= n;
 		stream.next_in = inbuf;

2.9.1.564.gb2f7278

This field, filled by sha1_object_info() contains the on-disk size of an object, which could go over 4GB limit of unsigned long on 32-bit systems. Use off_t for it instead and update all callers.

---
 builtin/cat-file.c | 4 ++--
 cache.h            | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

index 54db118..13ed944 100644
@@ -131,7 +131,7 @@ struct expand_data {
 	unsigned char sha1[20];
 	enum object_type type;
 	unsigned long size;
-	unsigned long disk_size;
+	off_t disk_size;
 	const char *rest;
 	unsigned char delta_base_sha1[20];
 

@@ -184,7 +184,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 		if (data->mark_query)
 			data->info.disk_sizep = &data->disk_size;
 		else
-			strbuf_addf(sb, "%lu", data->disk_size);
+			strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
 	} else if (is_atom("rest", atom, len)) {
 		if (data->mark_query)
 			data->split_on_whitespace = 1;

index 4ff196c..ea64b51 100644
@@ -1502,7 +1502,7 @@ struct object_info {
 	/* Request */
 	enum object_type *typep;
 	unsigned long *sizep;
-	unsigned long *disk_sizep;
+	off_t *disk_sizep;
 	unsigned char *delta_base_sha1;
 	struct strbuf *typename;
 

2.9.1.564.gb2f7278

A diff from nd/pack-ofs-4gb-limit can explain the changes better than me.

I did not add PRIdMAX or similar because that carries a risk to exotic platforms that people rarely test. Just casting to unsigned should be fine.

index 55eac75..b08bc8b 100644
@@ -356,6 +356,10 @@ static int fsck_sha1(const unsigned char *sha1)
 static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,
 			   unsigned long size, void *buffer, int *eaten)
 {
+	/*
+	 * Note, buffer may be NULL if type is OBJ_BLOB. See
+	 * verify_packfile(), data_valid variable for details.
+	 */
 	struct object *obj;
 	obj = parse_object_buffer(sha1, type, size, buffer, eaten);
 	if (!obj) {

index 14e8cb0..d123846 100644
@@ -106,7 +106,7 @@ static int verify_packfile(struct packed_git *p,
 		enum object_type type;
 		unsigned long size;
 		off_t curpos;
-		int data_valid = 0;
+		int data_valid;
 
 		if (p->index_version > 1) {
 			off_t offset = entries[i].offset;

@@ -130,6 +130,7 @@ static int verify_packfile(struct packed_git *p,
 			 * the data in-core only to discard.
 			 */
 			data = NULL;
+			data_valid = 0;
 		} else {
 			data = unpack_entry(p, entries[i].offset, &type, &size);
 			data_valid = 1;

index f9f3d13..096dbff 100755
@@ -177,10 +177,9 @@ test_expect_success 'zip achiving, deflate' '
 	git archive --format=zip HEAD >/dev/null
 '
 
-test_expect_success 'fsck' '
-	test_must_fail git fsck 2>err &&
-	n=$(grep "error: attempting to allocate .* over limit" err | wc -l) &&
-	test "$n" -gt 1
+test_expect_success 'fsck large blobs' '
+	git fsck 2>err &&
+	test_must_be_empty err
 '
 
 test_done

Nguyễn Thái Ngọc Duy (7):
pack-objects: pass length to check_pack_crc() without truncation sha1_file.c: use type off_t* for object_info->disk_sizep
index-pack: correct "len" type in unpack_data()
index-pack: report correct bad object offsets even if they are large
index-pack: correct "offset" type in unpack_entry_data()
pack-objects: do not truncate result in-pack object size on 32-bit systems
fsck: use streaming interface for large blobs in pack

 builtin/cat-file.c     |  4 ++--
 builtin/fsck.c         |  4 ++++
 builtin/index-pack.c   | 23 ++++++++++++-----------
 builtin/pack-objects.c | 17 +++++++++--------
 cache.h                |  2 +-
 pack-check.c           | 23 +++++++++++++++++++++--
 pack.h                 |  1 +
 sha1_file.c            |  2 +-
 t/t1050-large.sh       |  7 +++----
 9 files changed, 54 insertions(+), 29 deletions(-)

2.9.1.564.gb2f7278

unpack_entry_data() receives an off_t value from unpack_raw_entry(), which could be larger than unsigned long on 32-bit systems with large file support. Correct the type so truncation does not happen. This only affects bad object reporting though.

---
 builtin/index-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e2d8ae4..1008d7f 100644
@@ -430,7 +430,7 @@ static int is_delta_type(enum object_type type)
 	return (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);
 }
 
-static void *unpack_entry_data(unsigned long offset, unsigned long size,
+static void *unpack_entry_data(off_t offset, unsigned long size,
 			       enum object_type type, unsigned char *sha1)
 {
 	static char fixed_buf[8192];

2.9.1.564.gb2f7278

For blobs, we want to make sure the on-disk data is not corrupted (i.e. can be inflated and produce the expected SHA-1). Blob content is opaque, there's nothing else inside to check for.

For really large blobs, we may want to avoid unpacking the entire blob in memory, just to check whether it produces the same SHA-1. On 32-bit systems, we may not have enough virtual address space for such memory allocation. And even on 64-bit where it's not a problem, allocating a lot more memory could result in kicking other parts of systems to swap file, generating lots of I/O and slowing everything down.

For this particular operation, not unpacking the blob and letting check_sha1_signature, which supports streaming interface, do the job is sufficient. check_sha1_signature() is not shown in the diff, unfortunately. But if will be called when "data_valid && !data" is false.

We will call the callback function "fn" with NULL as "data". The only callback of this function is fsck_obj_buffer(), which does not touch "data" at all if it's a blob.

---
 builtin/fsck.c   |  4 ++++
 pack-check.c     | 23 +++++++++++++++++++++--
 pack.h           |  1 +
 t/t1050-large.sh |  7 +++----
 4 files changed, 29 insertions(+), 6 deletions(-)

index 55eac75..b08bc8b 100644
@@ -356,6 +356,10 @@ static int fsck_sha1(const unsigned char *sha1)
 static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,
 			   unsigned long size, void *buffer, int *eaten)
 {
+	/*
+	 * Note, buffer may be NULL if type is OBJ_BLOB. See
+	 * verify_packfile(), data_valid variable for details.
+	 */
 	struct object *obj;
 	obj = parse_object_buffer(sha1, type, size, buffer, eaten);
 	if (!obj) {

index 1da89a4..d123846 100644
@@ -105,6 +105,8 @@ static int verify_packfile(struct packed_git *p,
 		void *data;
 		enum object_type type;
 		unsigned long size;
+		off_t curpos;
+		int data_valid;
 
 		if (p->index_version > 1) {
 			off_t offset = entries[i].offset;

@@ -116,8 +118,25 @@ static int verify_packfile(struct packed_git *p,
 					    sha1_to_hex(entries[i].sha1),
 					    p->pack_name, (uintmax_t)offset);
 		}
-		data = unpack_entry(p, entries[i].offset, &type, &size);
-		if (!data)
+
+		curpos = entries[i].offset;
+		type = unpack_object_header(p, w_curs, &curpos, &size);
+		unuse_pack(w_curs);
+
+		if (type == OBJ_BLOB && big_file_threshold <= size) {
+			/*
+			 * Let check_sha1_signature() check it with
+			 * the streaming interface; no point slurping
+			 * the data in-core only to discard.
+			 */
+			data = NULL;
+			data_valid = 0;
+		} else {
+			data = unpack_entry(p, entries[i].offset, &type, &size);
+			data_valid = 1;
+		}
+
+		if (data_valid && !data)
 			err = error("cannot unpack %s from %s at offset %"PRIuMAX"",
 				    sha1_to_hex(entries[i].sha1), p->pack_name,
 				    (uintmax_t)entries[i].offset);

index 3223f5a..0e77429 100644
@@ -74,6 +74,7 @@ struct pack_idx_entry {
 
 
 struct progress;
+/* Note, the data argument could be NULL if object type is blob */
 typedef int (*verify_fn)(const unsigned char*, enum object_type, unsigned long, void*, int*);
 
 extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, const struct pack_idx_option *, const unsigned char *sha1);

index f9f3d13..096dbff 100755
@@ -177,10 +177,9 @@ test_expect_success 'zip achiving, deflate' '
 	git archive --format=zip HEAD >/dev/null
 '
 
-test_expect_success 'fsck' '
-	test_must_fail git fsck 2>err &&
-	n=$(grep "error: attempting to allocate .* over limit" err | wc -l) &&
-	test "$n" -gt 1
+test_expect_success 'fsck large blobs' '
+	git fsck 2>err &&
+	test_must_be_empty err
 '
 
 test_done

2.9.1.564.gb2f7278

A typical diff will not show what's going on and you need to see full functions. The core code is like this, at the end of of write_one()

	e->idx.offset = *offset;
	size = write_object(f, e, *offset);
	if (!size) {
		e->idx.offset = recursing;
		return WRITE_ONE_BREAK;
	}
	written_list[nr_written++] = &e->idx;

	/* make sure off_t is sufficiently large not to wrap */
	if (signed_add_overflows(*offset, size))
		die("pack too large for current definition of off_t");
	*offset += size;

Here we can see that the in-pack object size is returned by write_object (or indirectly by write_reuse_object). And it's used to calculate object offsets, which end up in the pack index file, generated at the end.

If "size" overflows (on 32-bit sytems, unsigned long is 32-bit while off_t can be 64-bit), we got wrong offsets and produce incorrect .idx file, which may make it look like the .pack file is corrupted.

---
 builtin/pack-objects.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

index a3a98c5..ac7a3a5 100644
@@ -341,8 +341,8 @@ static unsigned long write_no_reuse_object(struct sha1file *f, struct object_ent
 }
 
 /* Return 0 if we will bust the pack-size limit */
-static unsigned long write_reuse_object(struct sha1file *f, struct object_entry *entry,
-					unsigned long limit, int usable_delta)
+static off_t write_reuse_object(struct sha1file *f, struct object_entry *entry,
+				unsigned long limit, int usable_delta)
 {
 	struct packed_git *p = entry->in_pack;
 	struct pack_window *w_curs = NULL;

@@ -415,11 +415,12 @@ static unsigned long write_reuse_object(struct sha1file *f, struct object_entry
 }
 
 /* Return 0 if we will bust the pack-size limit */
-static unsigned long write_object(struct sha1file *f,
-				  struct object_entry *entry,
-				  off_t write_offset)
+static off_t write_object(struct sha1file *f,
+			  struct object_entry *entry,
+			  off_t write_offset)
 {
-	unsigned long limit, len;
+	unsigned long limit;
+	off_t len;
 	int usable_delta, to_reuse;
 
 	if (!pack_to_stdout)

@@ -491,7 +492,7 @@ static enum write_one_status write_one(struct sha1file *f,
 				       struct object_entry *e,
 				       off_t *offset)
 {
-	unsigned long size;
+	off_t size;
 	int recursing;
 
 	/*

2.9.1.564.gb2f7278

On 32 bit systems with large file support, unsigned long is 32-bit while the two offsets in the subtraction expression (pack-objects has the exact same expression as in sha1_file.c but not shown in diff) are in 64-bit. If an in-pack object is larger than 2^32 len/datalen is truncated and we get a misleading "error: bad packed object CRC for ..." as a result.

Use off_t for len and datalen. check_pack_crc() already accepts this argument as off_t and can deal with 4+ GB.

---
 builtin/pack-objects.c | 2 +-
 sha1_file.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index b6664ce..a3a98c5 100644
@@ -349,7 +349,7 @@ static unsigned long write_reuse_object(struct sha1file *f, struct object_entry
 	struct revindex_entry *revidx;
 	off_t offset;
 	enum object_type type = entry->type;
-	unsigned long datalen;
+	off_t datalen;
 	unsigned char header[10], dheader[10];
 	unsigned hdrlen;
 

index d0f2aa0..cd9b560 100644
@@ -2282,7 +2282,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
 
 		if (do_check_packed_object_crc && p->index_version > 1) {
 			struct revindex_entry *revidx = find_pack_revindex(p, obj_offset);
-			unsigned long len = revidx[1].offset - obj_offset;
+			off_t len = revidx[1].offset - obj_offset;
 			if (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {
 				const unsigned char *sha1 =
 					nth_packed_object_sha1(p, revidx->nr);

2.9.1.564.gb2f7278

Looks very sensible. Thanks.