{"thread":{"id":"42706","subject":"[bug] Reliably Reproducible Bad Packing of Objects","startedAt":"2016-06-24T22:38:31Z","lastAt":"2016-07-13T16:19:29Z","messageCount":39,"participants":["Christoph Michelbach","Duy Nguyen","Nguyễn Thái Ngọc Duy","Johannes Sixt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"290073","messageId":"1466807902.28869.8.camel@gmail.com","threadId":"42706","inReplyTo":null,"subject":"[bug] Reliably Reproducible Bad Packing of Objects","fromName":"Christoph Michelbach","fromEmail":"michelbach94@gmail.com","sentAt":"2016-06-24T22:38:22Z","receivedAt":"2016-06-24T22:38:31Z","isPatch":false,"sender":{"key":"michelbach94@gmail.com","avatar":null},"body":"Hi,\n\nwhen run on a 32 bit system (Linux system, don't know about other\nsystems),\n\nmkdir test && cd test && git init && touch someFile && git add someFile\n&& git commit -m \"Initial commit.\" && dd if=/dev/urandom\nof=bigBinaryFile bs=1MB count=4294 && git add bigBinaryFile && git\ncommit -m \"Introduced big biary file.\"\n\nreliably produces this error message: \"error: bad packed object CRC\nfor\"\n\nSince git counts sizes in byte and uses ints for it, it can't handle\nfiles bigger than (2^32 - 1) byte. That's 4'294.967296 MB. If you give\ngit a file bigger than it can handle, it usually just says \"fatal:\nCannot handle files this big\" without corrupting the repository. Btw.:\nIt'd be nice if the error message stated that this only occurs on 32\nbit system and only with files 4 GiB in size or bigger.\n\nTo provoke the bug, the commands above creates a file which cannot be\ncompressed slightly less than (2^32 - 1) byte big, probably resulting\nin a commit more than (2^32 - 1) byte big.\n\nI was able to reproduce the bug on a Raspberry Pi Model 3 (ARMv7 CPU)\nand a virtual machine running Ubuntu 16.04 32 bit (which was\nspecifically set up to test this, so it was a clean installation) on a\nhost running Ubuntu 16.04 64 bit on an ARM 64 bit x86 CPU (i7-4720HQ).\n\nOutput on raspi: https://gist.github.com/m1cm1c/d874f03be5b12cbd8b86ced\n79fa456d1\nOutput on virt. machine: https://gist.github.com/m1cm1c/d0dd47828386bb0\nf1e001b9f750416e0\n\nNote that on the raspi I concatenated `git gc` to the end to show that\nthe object exists but is corrupt but you can already see that something\nwent wrong before `git gc` is executed.\n\nIf you look at the output on the virtual machine, however, you will not\nsee that something wrong until you read the part where I typed in `git\ngc` which I find particularly worrying.\n\nWhen checking whether you get the same result, please make sure to use\ngit 1.7.6 or newer. If you use an older version of git (older versions\nare still being distributed, for example for Ubuntu 14.04), git will\ntry to memory-map a too big file resulting in a different error.\n\n-- \nWith kind regards\nChristoph Michelbach\n\n"},{"id":"290742","messageId":"20160702091034.GA31852@duynguyen","threadId":"42706","inReplyTo":"1466807902.28869.8.camel@gmail.com","subject":"Re: [bug] Reliably Reproducible Bad Packing of Objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-02T09:10:34Z","receivedAt":"2016-07-02T09:11:57Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 25, 2016 at 12:38:22AM +0200, Christoph Michelbach wrote:\n> Hi,\n> \n> when run on a 32 bit system (Linux system, don't know about other\n> systems),\n> \n> mkdir test && cd test && git init && touch someFile && git add someFile\n> && git commit -m \"Initial commit.\" && dd if=/dev/urandom\n> of=bigBinaryFile bs=1MB count=4294 && git add bigBinaryFile && git\n> commit -m \"Introduced big biary file.\"\n> \n> reliably produces this error message: \"error: bad packed object CRC\n> for\"\n\n\nI tried with a 32-bit git build on 64-bit system and did not see this\nmessage. Maybe it's because the 64-bit system. Just too lazy to bring\nup a 32-bit VM to process 4GB of data.\n\nBut I did look around a bit, and the problem is probably number\ntruncation. Could you try this patch?\n\n-- 8< --\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d5e1121..cb571ac 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2281,7 +2281,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \n \t\tif (do_check_packed_object_crc && p->index_version > 1) {\n \t\t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n-\t\t\tunsigned long len = revidx[1].offset - obj_offset;\n+\t\t\toff_t len = revidx[1].offset - obj_offset;\n \t\t\tif (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {\n \t\t\t\tconst unsigned char *sha1 =\n \t\t\t\t\tnth_packed_object_sha1(p, revidx->nr);\n-- 8< --\n\n\nIf your system has large file support (you must or things must have\nfailed badly from the beginning), then both offsets in that\nsubtraction expression are of type off_t, aka 64-bit. However \"len\"\nwould be 32-bit and truncation can happen. check_pack_crc() takes\noff_t too, so it seems well prepared already (but I didn't test).\n\nPS. There's a similar statement in pack-objects.c, but I don't think\nyou have done anything to trigger it. OK, 'git gc' may..\n--\nDuy\n"},{"id":"290750","messageId":"CACsJy8AJxbnEXciP91_r0jtH3mv+T_dqQZt9pKnJvSK-s9Yamg@mail.gmail.com","threadId":"42706","inReplyTo":"20160702091034.GA31852@duynguyen","subject":"Re: [bug] Reliably Reproducible Bad Packing of Objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-02T14:35:04Z","receivedAt":"2016-07-02T14:35:38Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jul 2, 2016 at 11:10 AM, Duy Nguyen wrote:\n> On Sat, Jun 25, 2016 at 12:38:22AM +0200, Christoph Michelbach wrote:\n>> Hi,\n>>\n>> when run on a 32 bit system (Linux system, don't know about other\n>> systems),\n>>\n>> mkdir test && cd test && git init && touch someFile && git add someFile\n>> && git commit -m \"Initial commit.\" && dd if=/dev/urandom\n>> of=bigBinaryFile bs=1MB count=4294 && git add bigBinaryFile && git\n>> commit -m \"Introduced big biary file.\"\n>>\n>> reliably produces this error message: \"error: bad packed object CRC\n>> for\"\n>\n>\n> I tried with a 32-bit git build on 64-bit system and did not see this\n> message. Maybe it's because the 64-bit system. Just too lazy to bring\n> up a 32-bit VM to process 4GB of data.\n>\n> But I did look around a bit, and the problem is probably number\n> truncation. Could you try this patch?\n\nJust FYI. There may be more truncation problems on 32-bit systems.\n\"clang -Wshorten-64-to-32\" reports a couple of spots that could be\nreal problems (pack-objects, index-pack...) for handling 4+ GB files\non these systems. Still working on it...\n-- \nDuy\n"},{"id":"290846","messageId":"20160705170558.10906-1-pclouds@gmail.com","threadId":"42706","inReplyTo":"1466807902.28869.8.camel@gmail.com","subject":"[PATCH 0/5] Number truncation with 4+ GB files on 32-bit systems","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-05T17:05:53Z","receivedAt":"2016-07-05T17:06:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Since I now could reproduce the problem that Christoph showed, I\ndecided to send the good patches out. To sum up, we use \"unsigned\nlong\" in some places related to file size. On 32-bit systems, it's\nlimited to 32 bits even though the system can handle files larger than\nthat (off_t is 64-bit). This fixes it.\n\nclang -Wshorten-64-to-32 is very helpful to spot these problems. I\nhave a couple more patches to clean all these warnings, but some need\nmore code study to see what is the right way to do.\n\nMost of the rest seems harmless, except for the local variable \"size\"\nin builtin/pack-objects.c:write_one(). I might send 6/5 for that one.\n\nNguyễn Thái Ngọc Duy (5):\n pack-objects: pass length to check_pack_crc() without truncation\n sha1_file.c: use type off_t* for object_info->disk_sizep\n index-pack: correct \"len\" type in unpack_data()\n index-pack: report correct bad object offsets even if they are large\n index-pack: correct \"offset\" type in unpack_entry_data()\n\n builtin/cat-file.c | 4 ++--\n builtin/index-pack.c | 23 ++++++++++++-----------\n builtin/pack-objects.c | 2 +-\n cache.h | 2 +-\n sha1_file.c | 2 +-\n 5 files changed, 17 insertions(+), 16 deletions(-)\n\n-- \n2.8.2.537.g0965dd9\n\n"},{"id":"290847","messageId":"20160705170558.10906-2-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"[PATCH 1/5] pack-objects: pass length to check_pack_crc() without truncation","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-05T17:05:54Z","receivedAt":"2016-07-05T17:06:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 32 bit systems with large file support, unsigned long is 32-bit\nwhile the two offsets in the subtraction expression (pack-objects has\nthe exact same expression as in sha1_file.c but not shown in diff) are\nin 64-bit. If an in-pack object is larger than 2^32 len/datalen is\ntruncated and we get a misleading \"error: bad packed object CRC for\n...\" as a result.\n\nUse off_t for len and datalen. check_pack_crc() already accepts this\nargument as off_t and can deal with 4+ GB.\n\nNoticed-by: Christoph Michelbach \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/pack-objects.c | 2 +-\n sha1_file.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 8f5e358..df6ecd5 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -349,7 +349,7 @@ static unsigned long write_reuse_object(struct sha1file *f, struct object_entry\n \tstruct revindex_entry *revidx;\n \toff_t offset;\n \tenum object_type type = entry->type;\n-\tunsigned long datalen;\n+\toff_t datalen;\n \tunsigned char header[10], dheader[10];\n \tunsigned hdrlen;\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex d5e1121..cb571ac 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2281,7 +2281,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \n \t\tif (do_check_packed_object_crc && p->index_version > 1) {\n \t\t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n-\t\t\tunsigned long len = revidx[1].offset - obj_offset;\n+\t\t\toff_t len = revidx[1].offset - obj_offset;\n \t\t\tif (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {\n \t\t\t\tconst unsigned char *sha1 =\n \t\t\t\t\tnth_packed_object_sha1(p, revidx->nr);\n-- \n2.8.2.537.g0965dd9\n\n"},{"id":"290848","messageId":"20160705170558.10906-6-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"[PATCH 5/5] index-pack: correct \"offset\" type in unpack_entry_data()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-05T17:05:58Z","receivedAt":"2016-07-05T17:06:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"unpack_entry_data() receives an off_t value from unpack_raw_entry(),\nwhich could be larger than unsigned long on 32-bit systems with large\nfile support. Correct the type so truncation does not happen. This\nonly affects bad object reporting though.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/index-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 73f7cd2..f35a9dc 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -430,7 +430,7 @@ static int is_delta_type(enum object_type type)\n \treturn (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);\n }\n \n-static void *unpack_entry_data(unsigned long offset, unsigned long size,\n+static void *unpack_entry_data(off_t offset, unsigned long size,\n \t\t\t enum object_type type, unsigned char *sha1)\n {\n \tstatic char fixed_buf[8192];\n-- \n2.8.2.537.g0965dd9\n\n"},{"id":"290849","messageId":"20160705170558.10906-5-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"[PATCH 4/5] index-pack: report correct bad object offsets even if they are large","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-05T17:05:57Z","receivedAt":"2016-07-05T17:06:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Use the right type for offsets in this case, off_t, which makes a\ndifference on 32-bit systems with large file support, and change\nformatting code accordingly.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/index-pack.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex cafaab7..73f7cd2 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -338,10 +338,10 @@ static void parse_pack_header(void)\n \tuse(sizeof(struct pack_header));\n }\n \n-static NORETURN void bad_object(unsigned long offset, const char *format,\n+static NORETURN void bad_object(off_t offset, const char *format,\n \t\t ...) __attribute__((format (printf, 2, 3)));\n \n-static NORETURN void bad_object(unsigned long offset, const char *format, ...)\n+static NORETURN void bad_object(off_t offset, const char *format, ...)\n {\n \tva_list params;\n \tchar buf[1024];\n@@ -349,7 +349,8 @@ static NORETURN void bad_object(unsigned long offset, const char *format, ...)\n \tva_start(params, format);\n \tvsnprintf(buf, sizeof(buf), format, params);\n \tva_end(params);\n-\tdie(_(\"pack has bad object at offset %lu: %s\"), offset, buf);\n+\tdie(_(\"pack has bad object at offset %\"PRIiMAX\": %s\"),\n+\t (intmax_t)offset, buf);\n }\n \n static inline struct thread_local *get_thread_data(void)\n-- \n2.8.2.537.g0965dd9\n\n"},{"id":"290850","messageId":"20160705170558.10906-3-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"[PATCH 2/5] sha1_file.c: use type off_t* for object_info->disk_sizep","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-05T17:05:55Z","receivedAt":"2016-07-05T17:06:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This field, filled by sha1_object_info() contains the on-disk size of\nan object, which could go over 4GB limit of unsigned long on 32-bit\nsystems. Use off_t for it instead and update all callers.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/cat-file.c | 4 ++--\n cache.h | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 618103f..5b34bd0 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -131,7 +131,7 @@ struct expand_data {\n \tunsigned char sha1[20];\n \tenum object_type type;\n \tunsigned long size;\n-\tunsigned long disk_size;\n+\toff_t disk_size;\n \tconst char *rest;\n \tunsigned char delta_base_sha1[20];\n \n@@ -191,7 +191,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\tif (data->mark_query)\n \t\t\tdata->info.disk_sizep = &data->disk_size;\n \t\telse\n-\t\t\tstrbuf_addf(sb, \"%lu\", data->disk_size);\n+\t\t\tstrbuf_addf(sb, \"%\"PRIuMAX, data->disk_size);\n \t} else if (is_atom(\"rest\", atom, len)) {\n \t\tif (data->mark_query)\n \t\t\tdata->split_on_whitespace = 1;\ndiff --git a/cache.h b/cache.h\nindex c73becb..a4465cb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1508,7 +1508,7 @@ struct object_info {\n \t/* Request */\n \tenum object_type *typep;\n \tunsigned long *sizep;\n-\tunsigned long *disk_sizep;\n+\toff_t *disk_sizep;\n \tunsigned char *delta_base_sha1;\n \tstruct strbuf *typename;\n \n-- \n2.8.2.537.g0965dd9\n\n"},{"id":"290851","messageId":"20160705170558.10906-4-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"[PATCH 3/5] index-pack: correct \"len\" type in unpack_data()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-05T17:05:56Z","receivedAt":"2016-07-05T17:06:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 32-bit systems with large file support, one entry could be larger\nthan 4GB and overflow \"len\". Correct it so we can unpack a full entry.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/index-pack.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex e8c71fc..cafaab7 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -549,13 +549,13 @@ static void *unpack_data(struct object_entry *obj,\n \t\t\t void *cb_data)\n {\n \toff_t from = obj[0].idx.offset + obj[0].hdr_size;\n-\tunsigned long len = obj[1].idx.offset - from;\n+\toff_t len = obj[1].idx.offset - from;\n \tunsigned char *data, *inbuf;\n \tgit_zstream stream;\n \tint status;\n \n \tdata = xmallocz(consume ? 64*1024 : obj->size);\n-\tinbuf = xmalloc((len < 64*1024) ? len : 64*1024);\n+\tinbuf = xmalloc((len < 64*1024) ? (int)len : 64*1024);\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n@@ -563,15 +563,15 @@ static void *unpack_data(struct object_entry *obj,\n \tstream.avail_out = consume ? 64*1024 : obj->size;\n \n \tdo {\n-\t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n+\t\tssize_t n = (len < 64*1024) ? (ssize_t)len : 64*1024;\n \t\tn = xpread(get_thread_data()->pack_fd, inbuf, n, from);\n \t\tif (n < 0)\n \t\t\tdie_errno(_(\"cannot pread pack file\"));\n \t\tif (!n)\n-\t\t\tdie(Q_(\"premature end of pack file, %lu byte missing\",\n-\t\t\t \"premature end of pack file, %lu bytes missing\",\n-\t\t\t len),\n-\t\t\t len);\n+\t\t\tdie(Q_(\"premature end of pack file, %\"PRIuMAX\" byte missing\",\n+\t\t\t \"premature end of pack file, %\"PRIuMAX\" bytes missing\",\n+\t\t\t (unsigned int)len),\n+\t\t\t (uintmax_t)len);\n \t\tfrom += n;\n \t\tlen -= n;\n \t\tstream.next_in = inbuf;\n-- \n2.8.2.537.g0965dd9\n\n"},{"id":"290856","messageId":"1467742293.3047.3.camel@gmail.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"Re: [PATCH 0/5] Number truncation with 4+ GB files on 32-bit systems","fromName":"Christoph Michelbach","fromEmail":"michelbach94@gmail.com","sentAt":"2016-07-05T18:11:33Z","receivedAt":"2016-07-05T18:11:41Z","isPatch":true,"sender":{"key":"michelbach94@gmail.com","avatar":null},"body":"Thank you very much!\n\n-- \nWith kind regards\nChristoph Michelbach\n\nOn Tue, 2016-07-05 at 19:05 +0200, Nguyễn Thái Ngọc Duy wrote:\n> Since I now could reproduce the problem that Christoph showed, I\n> decided to send the good patches out. To sum up, we use \"unsigned\n> long\" in some places related to file size. On 32-bit systems, it's\n> limited to 32 bits even though the system can handle files larger\n> than\n> that (off_t is 64-bit). This fixes it.\n> \n> clang -Wshorten-64-to-32 is very helpful to spot these problems. I\n> have a couple more patches to clean all these warnings, but some need\n> more code study to see what is the right way to do.\n> \n> Most of the rest seems harmless, except for the local variable \"size\"\n> in builtin/pack-objects.c:write_one(). I might send 6/5 for that one.\n> \n> Nguyễn Thái Ngọc Duy (5):\n>   pack-objects: pass length to check_pack_crc() without truncation\n>   sha1_file.c: use type off_t* for object_info->disk_sizep\n>   index-pack: correct \"len\" type in unpack_data()\n>   index-pack: report correct bad object offsets even if they are\n> large\n>   index-pack: correct \"offset\" type in unpack_entry_data()\n> \n>  builtin/cat-file.c     |  4 ++--\n>  builtin/index-pack.c   | 23 ++++++++++++-----------\n>  builtin/pack-objects.c |  2 +-\n>  cache.h                |  2 +-\n>  sha1_file.c            |  2 +-\n>  5 files changed, 17 insertions(+), 16 deletions(-)\n> \n"},{"id":"290859","messageId":"577C17D4.6080708@kdbg.org","threadId":"42706","inReplyTo":"20160705170558.10906-4-pclouds@gmail.com","subject":"Re: [PATCH 3/5] index-pack: correct \"len\" type in unpack_data()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-07-05T20:25:56Z","receivedAt":"2016-07-05T20:26:04Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.07.2016 um 19:05 schrieb Nguyễn Thái Ngọc Duy:\n> +\t\t\tdie(Q_(\"premature end of pack file, %\"PRIuMAX\" byte missing\",\n> +\t\t\t \"premature end of pack file, %\"PRIuMAX\" bytes missing\",\n\nI would be surprised if this form of translation worked...\n\n> +\t\t\t (unsigned int)len),\n> +\t\t\t (uintmax_t)len);\n\n-- Hannes\n\n"},{"id":"290879","messageId":"CACsJy8ASYszXU-ErPas99EpW_J_E-FQVk197W7KqURJge_RBRQ@mail.gmail.com","threadId":"42706","inReplyTo":"577C17D4.6080708@kdbg.org","subject":"Re: [PATCH 3/5] index-pack: correct \"len\" type in unpack_data()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-06T15:25:29Z","receivedAt":"2016-07-06T15:26:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 5, 2016 at 10:25 PM, Johannes Sixt wrote:\n> Am 05.07.2016 um 19:05 schrieb Nguyễn Thái Ngọc Duy:\n>>\n>> + die(Q_(\"premature end of pack file, %\"PRIuMAX\"\n>> byte missing\",\n>> + \"premature end of pack file, %\"PRIuMAX\"\n>> bytes missing\",\n>\n>\n> I would be surprised if this form of translation worked...\n\nWhat can I say, gettext is smart. In gc.c we already have this\n\ndie(_(\"gc is already running on machine '%s' pid %\"PRIuMAX\" (use\n--force if not)\"), name, (uintmax_t)pid);\n\nand vi.po shows\n\n#: builtin/gc.c:397\n#, c-format\nmsgid \"\"\n\"gc is already running on machine '%s' pid % (use --force if not)\"\n-- \nDuy\n"},{"id":"290886","messageId":"xmqqmvluu50w.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"CACsJy8ASYszXU-ErPas99EpW_J_E-FQVk197W7KqURJge_RBRQ@mail.gmail.com","subject":"Re: [PATCH 3/5] index-pack: correct \"len\" type in unpack_data()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-06T16:04:31Z","receivedAt":"2016-07-06T16:04:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> What can I say, gettext is smart. In gc.c we already have this\n>\n> die(_(\"gc is already running on machine '%s' pid %\"PRIuMAX\" (use\n> --force if not)\"), name, (uintmax_t)pid);\n>\n> and vi.po shows\n>\n> #: builtin/gc.c:397\n> #, c-format\n> msgid \"\"\n> \"gc is already running on machine '%s' pid % (use --force if not)\"\n\nAnd translators are expcted to keep \"%\" in their translated\n.po files, and whatever translates .po into .mo knows what\n% should be mapped to?\n\nThat is surprising.\n\nOn a related but not surprising tangent, I see this example in\ngettext.info that may be relevant.\n\n About larger integer types, such as ‘uintmax_t’ or ‘unsigned long\n long’: they can be handled by reducing the value to a range that fits in\n an ‘unsigned long’. Simply casting the value to ‘unsigned long’ would\n not do the right thing, since it would treat ‘ULONG_MAX + 1’ like zero,\n ‘ULONG_MAX + 2’ like singular, and the like. Here you can exploit the\n fact that all mentioned plural form formulas eventually become periodic,\n with a period that is a divisor of 100 (or 1000 or 1000000). So, when\n you reduce a large value to another one in the range [1000000, 1999999]\n that ends in the same 6 decimal digits, you can assume that it will lead\n to the same plural form selection. This code does this:\n\n #include \n uintmax_t nbytes = ...;\n printf (ngettext (\"The file has %\"PRIuMAX\" byte.\",\n \"The file has %\"PRIuMAX\" bytes.\",\n (nbytes > ULONG_MAX\n ? (nbytes % 1000000) + 1000000\n : nbytes)),\n nbytes);\n\n\n\n"},{"id":"290887","messageId":"xmqqinwiu4tg.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"xmqqmvluu50w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/5] index-pack: correct \"len\" type in unpack_data()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-06T16:08:59Z","receivedAt":"2016-07-06T16:09:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Duy Nguyen writes:\n>\n>> What can I say, gettext is smart. In gc.c we already have this\n>>\n>> die(_(\"gc is already running on machine '%s' pid %\"PRIuMAX\" (use\n>> --force if not)\"), name, (uintmax_t)pid);\n>>\n>> and vi.po shows\n>>\n>> #: builtin/gc.c:397\n>> #, c-format\n>> msgid \"\"\n>> \"gc is already running on machine '%s' pid % (use --force if not)\"\n>\n> And translators are expcted to keep \"%\" in their translated\n> .po files, and whatever translates .po into .mo knows what\n> % should be mapped to?\n>\n> That is surprising.\n\ngettext.info answered me ;-)\n\n The ‘gettext’ tools and library have special support for these\n ‘’ macros. You can therefore simply write\n\n printf (gettext (\"The amount is %0\" PRId64 \"\\n\"), number);\n\n The PO file will contain the string \"The amount is %0\\n\". The\n translators will provide a translation containing \"%0\" as well,\n and at runtime the ‘gettext’ function’s result will contain the\n appropriate constant string, \"d\" or \"ld\" or \"lld\".\n\n This works only for the predefined ‘’ macro...\n\nNice.\n"},{"id":"290895","messageId":"1467828168.6286.5.camel@gmail.com","threadId":"42706","inReplyTo":"CACsJy8BDQbanGsf=3z3K-OuH0++EuqQFEB22udXJT+WZnFKSBg@mail.gmail.com","subject":"Re: [PATCH 0/5] Number truncation with 4+ GB files on 32-bit systems","fromName":"Christoph Michelbach","fromEmail":"michelbach94@gmail.com","sentAt":"2016-07-06T18:02:48Z","receivedAt":"2016-07-06T18:02:59Z","isPatch":true,"sender":{"key":"michelbach94@gmail.com","avatar":null},"body":"I now created a repo on a 64 bit system (same command as before), then\nduplicated it. One copy I applied git gc to on the 64 bit host system,\nthe other copy I gave to my 32 bit virtual machine to apply git gc to\nit.\n\nThe 64 bit host uses git from the Ubuntu repositories, the 32 bit\nvirtual machine uses git 2.9 from github with the patches applied.\n\ngit gc worked without problems on the host but I got\n\nfrank@frank-virtual-16-04-32-bit:~/g$ git gc\nCounting objects: 6, done.\nCompressing objects: 100% (3/3), done.\nerror: bad packed object CRC for\nf595ad71c1a1ecc312ddcb32a84a4bfc4a2ed1c8\nWriting objects: 100% (6/6), done.\nTotal 6 (delta 0), reused 0 (delta 0)\nerror: failed to validate delta base reference at offset 342896 from\n.git/objects/pack/pack-630b5a3f28cd9d02a546462bf0d0bc640ffde784.pack\nerror: bad offset for revindex\nerror: failed to read object 4246d27f8e0149d45687b0cc23bc29a67f1f0c79\nat offset 342887 from .git/objects/pack/pack-\n630b5a3f28cd9d02a546462bf0d0bc640ffde784.pack\nfatal: packed object 4246d27f8e0149d45687b0cc23bc29a67f1f0c79 (stored\nin .git/objects/pack/pack-\n630b5a3f28cd9d02a546462bf0d0bc640ffde784.pack) is corrupt\nerror: failed to run prune\nfrank@frank-virtual-16-04-32-bit:~/g$ \n\non the virtual machine.\n\nNot including the mailing list in CC wasn't intended.\n\n-- \nWith kind regards\nChristoph Michelbach\n\nOn Wed, 2016-07-06 at 17:23 +0200, Duy Nguyen wrote:\n> On Wed, Jul 6, 2016 at 12:14 AM, Christoph Michelbach\n> wrote:\n> > \n> > I now tried git gc again and it failed (in a different way, though;\n> > and\n> > the error message only appeared once git gc terminated).\n> > \n> > Full input and output:\n> > \n> > christoph@virt-16-04-32-bit:~$ mkdir test && cd test && git init &&\n> > touch someFile && git add someFile && git commit -m \"Initial\n> > commit.\"\n> > && dd if=/dev/urandom of=bigBinaryFile bs=1MB count=4294 && git add\n> > bigBinaryFile && git commit -m \"Introduced big biary file.\"\n> > Initialized empty Git repository in /home/christoph/test/.git/\n> > [master (root-commit) 20507ef] Initial commit.\n> >  1 file changed, 0 insertions(+), 0 deletions(-)\n> >  create mode 100644 someFile\n> > 4294+0 records in\n> > 4294+0 records out\n> > 4294000000 bytes (4.3 GB, 4.0 GiB) copied, 435.236 s, 9.9 MB/s\n> > [master 88e5dbb] Introduced big biary file.\n> >  1 file changed, 0 insertions(+), 0 deletions(-)\n> >  create mode 100644 bigBinaryFile\n> > christoph@virt-16-04-32-bit:~/test$ git gc\n> > Counting objects: 6, done.\n> > Compressing objects: 100% (3/3), done.\n> > Writing objects: 100% (6/6), done.\n> > Total 6 (delta 0), reused 1 (delta 0)\n> > error: inflate: data stream error (incorrect header check)\n> > error: failed to read object\n> > 705f438ccb845871ffba9d4b56f16ac763652937\n> Sigh.. I'll try again :) BTW if you have a 64-bit machine too, try\n> create the repo with it (so we are sure the repo is not corrupted in\n> the first place) then you can run tests like this on 32-bit systems.\n> The previous \"bad crc\" message was a fault in the code, not the data.\n> \n> PS. I have just let gc run till the end (last time I tried\n> pack-objects, which is only part of gc) and got a(nother) error.\n> Checking... BTW, you should send these mails to git@ as well, no need\n> for private message exchange.\n"},{"id":"290902","messageId":"CACsJy8DxT1=9if3UksTJcvpJS73uCDoAojsBC9Gg4R9DvZHdjQ@mail.gmail.com","threadId":"42706","inReplyTo":"1467828168.6286.5.camel@gmail.com","subject":"Re: [PATCH 0/5] Number truncation with 4+ GB files on 32-bit systems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-06T18:54:21Z","receivedAt":"2016-07-06T18:54:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 6, 2016 at 8:02 PM, Christoph Michelbach\n wrote:\n> I now created a repo on a 64 bit system (same command as before), then\n> duplicated it. One copy I applied git gc to on the 64 bit host system,\n> the other copy I gave to my 32 bit virtual machine to apply git gc to\n> it.\n>\n> The 64 bit host uses git from the Ubuntu repositories, the 32 bit\n> virtual machine uses git 2.9 from github with the patches applied.\n>\n> git gc worked without problems on the host but I got\n>\n> frank@frank-virtual-16-04-32-bit:~/g$ git gc\n> Counting objects: 6, done.\n> Compressing objects: 100% (3/3), done.\n> error: bad packed object CRC for\n> f595ad71c1a1ecc312ddcb32a84a4bfc4a2ed1c8\n> Writing objects: 100% (6/6), done.\n> Total 6 (delta 0), reused 0 (delta 0)\n> error: failed to validate delta base reference at offset 342896 from\n> .git/objects/pack/pack-630b5a3f28cd9d02a546462bf0d0bc640ffde784.pack\n\nYup. So far I can only say that pack-objects generates incorrect pack\n:-( I hope it's the possible 6/5 patch I mentioned earlier but I'm\nnot sure. Will continue tomorrow.\n\n> error: bad offset for revindex\n> error: failed to read object 4246d27f8e0149d45687b0cc23bc29a67f1f0c79\n> at offset 342887 from .git/objects/pack/pack-\n> 630b5a3f28cd9d02a546462bf0d0bc640ffde784.pack\n> fatal: packed object 4246d27f8e0149d45687b0cc23bc29a67f1f0c79 (stored\n> in .git/objects/pack/pack-\n> 630b5a3f28cd9d02a546462bf0d0bc640ffde784.pack) is corrupt\n> error: failed to run prune\n> frank@frank-virtual-16-04-32-bit:~/g$\n>\n> on the virtual machine.\n>\n> Not including the mailing list in CC wasn't intended.\n>\n> --\n> With kind regards\n> Christoph Michelbach\n>\n> On Wed, 2016-07-06 at 17:23 +0200, Duy Nguyen wrote:\n>> On Wed, Jul 6, 2016 at 12:14 AM, Christoph Michelbach\n>> wrote:\n>> >\n>> > I now tried git gc again and it failed (in a different way, though;\n>> > and\n>> > the error message only appeared once git gc terminated).\n>> >\n>> > Full input and output:\n>> >\n>> > christoph@virt-16-04-32-bit:~$ mkdir test && cd test && git init &&\n>> > touch someFile && git add someFile && git commit -m \"Initial\n>> > commit.\"\n>> > && dd if=/dev/urandom of=bigBinaryFile bs=1MB count=4294 && git add\n>> > bigBinaryFile && git commit -m \"Introduced big biary file.\"\n>> > Initialized empty Git repository in /home/christoph/test/.git/\n>> > [master (root-commit) 20507ef] Initial commit.\n>> > 1 file changed, 0 insertions(+), 0 deletions(-)\n>> > create mode 100644 someFile\n>> > 4294+0 records in\n>> > 4294+0 records out\n>> > 4294000000 bytes (4.3 GB, 4.0 GiB) copied, 435.236 s, 9.9 MB/s\n>> > [master 88e5dbb] Introduced big biary file.\n>> > 1 file changed, 0 insertions(+), 0 deletions(-)\n>> > create mode 100644 bigBinaryFile\n>> > christoph@virt-16-04-32-bit:~/test$ git gc\n>> > Counting objects: 6, done.\n>> > Compressing objects: 100% (3/3), done.\n>> > Writing objects: 100% (6/6), done.\n>> > Total 6 (delta 0), reused 1 (delta 0)\n>> > error: inflate: data stream error (incorrect header check)\n>> > error: failed to read object\n>> > 705f438ccb845871ffba9d4b56f16ac763652937\n>> Sigh.. I'll try again :) BTW if you have a 64-bit machine too, try\n>> create the repo with it (so we are sure the repo is not corrupted in\n>> the first place) then you can run tests like this on 32-bit systems.\n>> The previous \"bad crc\" message was a fault in the code, not the data.\n>>\n>> PS. I have just let gc run till the end (last time I tried\n>> pack-objects, which is only part of gc) and got a(nother) error.\n>> Checking... BTW, you should send these mails to git@ as well, no need\n>> for private message exchange.\n\n\n\n-- \nDuy\n"},{"id":"291126","messageId":"CACsJy8CHsMXJOtr14XYLeU8J6eAwk5rahtkppwxV0JpeXL5yWw@mail.gmail.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"Re: [PATCH 0/5] Number truncation with 4+ GB files on 32-bit systems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-10T10:41:42Z","receivedAt":"2016-07-10T10:43:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 5, 2016 at 7:05 PM, Nguyễn Thái Ngọc Duy wrote:\n> Most of the rest seems harmless, except for the local variable \"size\"\n> in builtin/pack-objects.c:write_one(). I might send 6/5 for that one.\n\nThis truncated \"size\" produces incorrect .idx file and makes everybody\nelse fail to read the (correct) .pack file. So 6/5 is coming shortly.\n\nI also add 7/5 to be able to run fsck on 32-bit systems, but 7/5 could\nhelp improve performance a bit on 64-bit systems too.\n-- \nDuy\n"},{"id":"291127","messageId":"20160710104251.27335-1-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"[PATCH 6/5] pack-objects: do not truncate result in-pack object size on 32-bit systems","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-10T10:42:51Z","receivedAt":"2016-07-10T10:43:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A typical diff will not show what's going on and you need to see full\nfunctions. The core code is like this, at the end of of write_one()\n\n\te->idx.offset = *offset;\n\tsize = write_object(f, e, *offset);\n\tif (!size) {\n\t\te->idx.offset = recursing;\n\t\treturn WRITE_ONE_BREAK;\n\t}\n\twritten_list[nr_written++] = &e->idx;\n\n\t/* make sure off_t is sufficiently large not to wrap */\n\tif (signed_add_overflows(*offset, size))\n\t\tdie(\"pack too large for current definition of off_t\");\n\t*offset += size;\n\nHere we can see that the in-pack object size is returned by\nwrite_object (or indirectly by write_reuse_object). And it's used to\ncalculate object offsets, which end up in the pack index file,\ngenerated at the end.\n\nIf \"size\" overflows (on 32-bit sytems, unsigned long is 32-bit while\noff_t can be 64-bit), we got wrong offsets and produce incorrect .idx\nfile, which may make it look like the .pack file is corrupted.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/pack-objects.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex df6ecd5..f854ca4 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -341,8 +341,8 @@ static unsigned long write_no_reuse_object(struct sha1file *f, struct object_ent\n }\n \n /* Return 0 if we will bust the pack-size limit */\n-static unsigned long write_reuse_object(struct sha1file *f, struct object_entry *entry,\n-\t\t\t\t\tunsigned long limit, int usable_delta)\n+static off_t write_reuse_object(struct sha1file *f, struct object_entry *entry,\n+\t\t\t\tunsigned long limit, int usable_delta)\n {\n \tstruct packed_git *p = entry->in_pack;\n \tstruct pack_window *w_curs = NULL;\n@@ -415,11 +415,12 @@ static unsigned long write_reuse_object(struct sha1file *f, struct object_entry\n }\n \n /* Return 0 if we will bust the pack-size limit */\n-static unsigned long write_object(struct sha1file *f,\n-\t\t\t\t struct object_entry *entry,\n-\t\t\t\t off_t write_offset)\n+static off_t write_object(struct sha1file *f,\n+\t\t\t struct object_entry *entry,\n+\t\t\t off_t write_offset)\n {\n-\tunsigned long limit, len;\n+\tunsigned long limit;\n+\toff_t len;\n \tint usable_delta, to_reuse;\n \n \tif (!pack_to_stdout)\n@@ -491,7 +492,7 @@ static enum write_one_status write_one(struct sha1file *f,\n \t\t\t\t struct object_entry *e,\n \t\t\t\t off_t *offset)\n {\n-\tunsigned long size;\n+\toff_t size;\n \tint recursing;\n \n \t/*\n-- \n2.8.2.537.g0965dd9\n\n"},{"id":"291128","messageId":"20160710104555.27478-1-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"[PATCH 7/5] fsck: use streaming interface for large blobs in pack","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-10T10:45:55Z","receivedAt":"2016-07-10T10:46:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"For blobs, we want to make sure the on-disk data is not corrupted\n(i.e. can be inflated and produce the expected SHA-1). Blob content is\nopaque, there's nothing else inside to check for.\n\nFor really large blobs, we may want to avoid unpacking the entire blob\nin memory, just to check whether it produces the same SHA-1. On 32-bit\nsystems, we may not have enough virtual address space for such memory\nallocation. And even on 64-bit where it's not a problem, allocating a\nlot more memory could result in kicking other parts of systems to swap\nfile, generating lots of I/O and slowing everything down.\n\nFor this particular operation, not unpacking the blob and letting\ncheck_sha1_signature, which supports streaming interface, do the job\nis sufficient. check_sha1_signature() is not shown in the diff,\nunfortunately. But if will be called when \"data_valid && !data\" is\nfalse.\n\nWe will call the callback function \"fn\" with NULL as \"data\". The only\ncallback of this function is fsck_obj_buffer(), which does not touch\n\"data\" at all if it's a blob.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n pack-check.c | 15 +++++++++++++--\n pack.h | 1 +\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex 1da89a4..0777766 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -105,6 +105,8 @@ static int verify_packfile(struct packed_git *p,\n \t\tvoid *data;\n \t\tenum object_type type;\n \t\tunsigned long size;\n+\t\toff_t curpos;\n+\t\tint data_valid = 0;\n \n \t\tif (p->index_version > 1) {\n \t\t\toff_t offset = entries[i].offset;\n@@ -116,8 +118,17 @@ static int verify_packfile(struct packed_git *p,\n \t\t\t\t\t sha1_to_hex(entries[i].sha1),\n \t\t\t\t\t p->pack_name, (uintmax_t)offset);\n \t\t}\n-\t\tdata = unpack_entry(p, entries[i].offset, &type, &size);\n-\t\tif (!data)\n+\n+\t\tcurpos = entries[i].offset;\n+\t\ttype = unpack_object_header(p, w_curs, &curpos, &size);\n+\t\tunuse_pack(w_curs);\n+\n+\t\tif (type != OBJ_BLOB || size < big_file_threshold) {\n+\t\t\tdata = unpack_entry(p, entries[i].offset, &type, &size);\n+\t\t\tdata_valid = 1;\n+\t\t}\n+\n+\t\tif (data_valid && !data)\n \t\t\terr = error(\"cannot unpack %s from %s at offset %\"PRIuMAX\"\",\n \t\t\t\t sha1_to_hex(entries[i].sha1), p->pack_name,\n \t\t\t\t (uintmax_t)entries[i].offset);\ndiff --git a/pack.h b/pack.h\nindex 3223f5a..0e77429 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -74,6 +74,7 @@ struct pack_idx_entry {\n \n \n struct progress;\n+/* Note, the data argument could be NULL if object type is blob */\n typedef int (*verify_fn)(const unsigned char*, enum object_type, unsigned long, void*, int*);\n \n 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);\n-- \n2.8.2.537.g0965dd9\n\n"},{"id":"291284","messageId":"xmqqshve6azl.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"Re: [PATCH 0/5] Number truncation with 4+ GB files on 32-bit systems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-12T17:07:42Z","receivedAt":"2016-07-12T17:07:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Since I now could reproduce the problem that Christoph showed, I\n> decided to send the good patches out. To sum up, we use \"unsigned\n> long\" in some places related to file size. On 32-bit systems, it's\n> limited to 32 bits even though the system can handle files larger than\n> that (off_t is 64-bit). This fixes it.\n>\n> clang -Wshorten-64-to-32 is very helpful to spot these problems. I\n> have a couple more patches to clean all these warnings, but some need\n> more code study to see what is the right way to do.\n>\n> Most of the rest seems harmless, except for the local variable \"size\"\n> in builtin/pack-objects.c:write_one(). I might send 6/5 for that one.\n\n\nThanks.\n\n> Nguyễn Thái Ngọc Duy (5):\n> pack-objects: pass length to check_pack_crc() without truncation\n> sha1_file.c: use type off_t* for object_info->disk_sizep\n> index-pack: correct \"len\" type in unpack_data()\n> index-pack: report correct bad object offsets even if they are large\n> index-pack: correct \"offset\" type in unpack_entry_data()\n>\n> builtin/cat-file.c | 4 ++--\n> builtin/index-pack.c | 23 ++++++++++++-----------\n> builtin/pack-objects.c | 2 +-\n> cache.h | 2 +-\n> sha1_file.c | 2 +-\n> 5 files changed, 17 insertions(+), 16 deletions(-)\n"},{"id":"291285","messageId":"xmqqoa626alg.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"20160705170558.10906-2-pclouds@gmail.com","subject":"Re: [PATCH 1/5] pack-objects: pass length to check_pack_crc() without truncation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-12T17:16:11Z","receivedAt":"2016-07-12T17:16:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 8f5e358..df6ecd5 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -349,7 +349,7 @@ static unsigned long write_reuse_object(struct sha1file *f, struct object_entry\n> \tstruct revindex_entry *revidx;\n> \toff_t offset;\n> \tenum object_type type = entry->type;\n> -\tunsigned long datalen;\n> +\toff_t datalen;\n> \tunsigned char header[10], dheader[10];\n> \tunsigned hdrlen;\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index d5e1121..cb571ac 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2281,7 +2281,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n> \n> \t\tif (do_check_packed_object_crc && p->index_version > 1) {\n> \t\t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n> -\t\t\tunsigned long len = revidx[1].offset - obj_offset;\n> +\t\t\toff_t len = revidx[1].offset - obj_offset;\n> \t\t\tif (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) \n\nIt is sad that check_pack_crc() already knew to take off_t here and\nwe (unknowingly) had a deliberate truncation by using ulong.\nLooks obvioiusly correct.\n\nThanks.\n\n{\n> \t\t\t\tconst unsigned char *sha1 =\n> \t\t\t\t\tnth_packed_object_sha1(p, revidx->nr);\n"},{"id":"291286","messageId":"xmqqk2gq6af1.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"20160705170558.10906-3-pclouds@gmail.com","subject":"Re: [PATCH 2/5] sha1_file.c: use type off_t* for object_info->disk_sizep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-12T17:20:02Z","receivedAt":"2016-07-12T17:20:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> This field, filled by sha1_object_info() contains the on-disk size of\n> an object, which could go over 4GB limit of unsigned long on 32-bit\n> systems. Use off_t for it instead and update all callers.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/cat-file.c | 4 ++--\n> cache.h | 2 +-\n> 2 files changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 618103f..5b34bd0 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -131,7 +131,7 @@ struct expand_data {\n> \tunsigned char sha1[20];\n> \tenum object_type type;\n> \tunsigned long size;\n> -\tunsigned long disk_size;\n> +\toff_t disk_size;\n> \tconst char *rest;\n> \tunsigned char delta_base_sha1[20];\n> \n> @@ -191,7 +191,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n> \t\tif (data->mark_query)\n> \t\t\tdata->info.disk_sizep = &data->disk_size;\n> \t\telse\n> -\t\t\tstrbuf_addf(sb, \"%lu\", data->disk_size);\n> +\t\t\tstrbuf_addf(sb, \"%\"PRIuMAX, data->disk_size);\n\nDoesn't this now need a cast?\n\n> \t} else if (is_atom(\"rest\", atom, len)) {\n> \t\tif (data->mark_query)\n> \t\t\tdata->split_on_whitespace = 1;\n> diff --git a/cache.h b/cache.h\n> index c73becb..a4465cb 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1508,7 +1508,7 @@ struct object_info {\n> \t/* Request */\n> \tenum object_type *typep;\n> \tunsigned long *sizep;\n> -\tunsigned long *disk_sizep;\n> +\toff_t *disk_sizep;\n> \tunsigned char *delta_base_sha1;\n> \tstruct strbuf *typename;\n"},{"id":"291287","messageId":"xmqqfure6a7b.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"20160705170558.10906-5-pclouds@gmail.com","subject":"Re: [PATCH 4/5] index-pack: report correct bad object offsets even if they are large","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-12T17:24:40Z","receivedAt":"2016-07-12T17:24:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Use the right type for offsets in this case, off_t, which makes a\n> difference on 32-bit systems with large file support, and change\n> formatting code accordingly.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/index-pack.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index cafaab7..73f7cd2 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -338,10 +338,10 @@ static void parse_pack_header(void)\n> \tuse(sizeof(struct pack_header));\n> }\n> \n> -static NORETURN void bad_object(unsigned long offset, const char *format,\n> +static NORETURN void bad_object(off_t offset, const char *format,\n> \t\t ...) __attribute__((format (printf, 2, 3)));\n> \n> -static NORETURN void bad_object(unsigned long offset, const char *format, ...)\n> +static NORETURN void bad_object(off_t offset, const char *format, ...)\n> {\n> \tva_list params;\n> \tchar buf[1024];\n> @@ -349,7 +349,8 @@ static NORETURN void bad_object(unsigned long offset, const char *format, ...)\n> \tva_start(params, format);\n> \tvsnprintf(buf, sizeof(buf), format, params);\n> \tva_end(params);\n> -\tdie(_(\"pack has bad object at offset %lu: %s\"), offset, buf);\n> +\tdie(_(\"pack has bad object at offset %\"PRIiMAX\": %s\"),\n> +\t (intmax_t)offset, buf);\n> }\n\nWe seem to have a fallback definition only for PRIuMAX.\n\noff_t is supposed to be a signed integer type [*1*], but we know\nthis is a positive offset when we issue this warning, so PRIuMAX\nwould probably be fine.\n\n\n[Reference]\n\n*1* http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html\n"},{"id":"291292","messageId":"xmqq7fcq66q6.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"20160710104555.27478-1-pclouds@gmail.com","subject":"Re: [PATCH 7/5] fsck: use streaming interface for large blobs in pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-12T18:39:45Z","receivedAt":"2016-07-12T18:39:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> For this particular operation, not unpacking the blob and letting\n> check_sha1_signature, which supports streaming interface, do the job\n> is sufficient.\n\nThat reasoning is sound, I would think, but...\n\n> We will call the callback function \"fn\" with NULL as \"data\". The only\n> callback of this function is fsck_obj_buffer(), which does not touch\n> \"data\" at all if it's a blob.\n\n... this may be a bit too magical that the assumption needs to be\nleft in an in-code comment to warn people against temptation to\nusing \"data\" over there, not just where the type of verify_fn is\ndeclared. Essentially, you are changing the expectation from\nexisting functions of that type.\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> pack-check.c | 15 +++++++++++++--\n> pack.h | 1 +\n> 2 files changed, 14 insertions(+), 2 deletions(-)\n>\n> diff --git a/pack-check.c b/pack-check.c\n> index 1da89a4..0777766 100644\n> --- a/pack-check.c\n> +++ b/pack-check.c\n> @@ -105,6 +105,8 @@ static int verify_packfile(struct packed_git *p,\n> \t\tvoid *data;\n> \t\tenum object_type type;\n> \t\tunsigned long size;\n> +\t\toff_t curpos;\n> +\t\tint data_valid = 0;\n> \n> \t\tif (p->index_version > 1) {\n> \t\t\toff_t offset = entries[i].offset;\n> @@ -116,8 +118,17 @@ static int verify_packfile(struct packed_git *p,\n> \t\t\t\t\t sha1_to_hex(entries[i].sha1),\n> \t\t\t\t\t p->pack_name, (uintmax_t)offset);\n> \t\t}\n> -\t\tdata = unpack_entry(p, entries[i].offset, &type, &size);\n> -\t\tif (!data)\n> +\n> +\t\tcurpos = entries[i].offset;\n> +\t\ttype = unpack_object_header(p, w_curs, &curpos, &size);\n> +\t\tunuse_pack(w_curs);\n\nThis made me wonder where this \"unuse\" comes from; w_curs's in-use\ncount is incremented by calling unpack_object_header() and we are\ndone with that access at this point, hence we have unuse here.\n\n> +\t\tif (type != OBJ_BLOB || size < big_file_threshold) {\n> +\t\t\tdata = unpack_entry(p, entries[i].offset, &type, &size);\n> +\t\t\tdata_valid = 1;\n\nThis codepath slurps the data in-core to hash and data is later\nfreed, i.e. non-blob objects and small ones are handled as before.\n\n> +\t\t}\n> +\n> +\t\tif (data_valid && !data)\n> \t\t\terr = error(\"cannot unpack %s from %s at offset %\"PRIuMAX\"\",\n> \t\t\t\t sha1_to_hex(entries[i].sha1), p->pack_name,\n> \t\t\t\t (uintmax_t)entries[i].offset);\n\nOtherwise, we'd go to check_sha1_signature() with map==NULL. And\nthat is exactly what we want---map==NULL is the way we tell the\nfunction to use the streaming interface to check.\n\nGood.\n\n> diff --git a/pack.h b/pack.h\n> index 3223f5a..0e77429 100644\n> --- a/pack.h\n> +++ b/pack.h\n> @@ -74,6 +74,7 @@ struct pack_idx_entry {\n> \n> \n> struct progress;\n> +/* Note, the data argument could be NULL if object type is blob */\n> typedef int (*verify_fn)(const unsigned char*, enum object_type, unsigned long, void*, int*);\n> \n> 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);\n"},{"id":"291293","messageId":"xmqq37ne66by.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"xmqqshve6azl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/5] Number truncation with 4+ GB files on 32-bit systems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-12T18:48:17Z","receivedAt":"2016-07-12T18:48:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Since I now could reproduce the problem that Christoph showed, I\n>> decided to send the good patches out. To sum up, we use \"unsigned\n>> long\" in some places related to file size. On 32-bit systems, it's\n>> limited to 32 bits even though the system can handle files larger than\n>> that (off_t is 64-bit). This fixes it.\n>>\n>> clang -Wshorten-64-to-32 is very helpful to spot these problems. I\n>> have a couple more patches to clean all these warnings, but some need\n>> more code study to see what is the right way to do.\n>>\n>> Most of the rest seems harmless, except for the local variable \"size\"\n>> in builtin/pack-objects.c:write_one(). I might send 6/5 for that one.\n>\n>\n> Thanks.\n\nAll looked nicely done. I'll queue with a few SQUASH??? (please ack\nor reject them) in between on 'pu' and push the result out later\ntoday.\n\nThanks.\n"},{"id":"291294","messageId":"xmqqvb0a4qx3.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"xmqq7fcq66q6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 7/5] fsck: use streaming interface for large blobs in pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-12T19:06:32Z","receivedAt":"2016-07-12T19:07:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\t\tif (type != OBJ_BLOB || size < big_file_threshold) {\n>> +\t\t\tdata = unpack_entry(p, entries[i].offset, &type, &size);\n>> +\t\t\tdata_valid = 1;\n>\n> This codepath slurps the data in-core to hash and data is later\n> freed, i.e. non-blob objects and small ones are handled as before.\n>\n>> +\t\t}\n>> +\n>> +\t\tif (data_valid && !data)\n>> \t\t\terr = error(\"cannot unpack %s from %s at offset %\"PRIuMAX\"\",\n>> \t\t\t\t sha1_to_hex(entries[i].sha1), p->pack_name,\n>> \t\t\t\t (uintmax_t)entries[i].offset);\n>\n> Otherwise, we'd go to check_sha1_signature() with map==NULL. And\n> that is exactly what we want---map==NULL is the way we tell the\n> function to use the streaming interface to check.\n>\n> Good.\n\nBut not quite correct yet ;-)\n\nHere is what I'd propose to squash in. Even though data_valid\nprotects the above if() condition from accessing an otherwise\nuninitialized \"data\", the call to check_sha1_signature() we have\nlater will get noticed by the compiler that \"data\" is passed\nuninitialized.\n\nMore importantly, uninitialized data passed may be non-NULL, in\nwhich case it would not trigger the streaming interface.\n\n pack-check.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex 0777766..ac263fd 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -123,7 +123,14 @@ static int verify_packfile(struct packed_git *p,\n \t\ttype = unpack_object_header(p, w_curs, &curpos, &size);\n \t\tunuse_pack(w_curs);\n \n-\t\tif (type != OBJ_BLOB || size < big_file_threshold) {\n+\t\tif (type == OBJ_BLOB && big_file_threshold <= size) {\n+\t\t\t/*\n+\t\t\t * Let check_sha1_signature() to check it with\n+\t\t\t * the streaming interface; no point slurping\n+\t\t\t * the data in-core only to discard.\n+\t\t\t */\n+\t\t\tdata = NULL;\n+\t\t} else {\n \t\t\tdata = unpack_entry(p, entries[i].offset, &type, &size);\n \t\t\tdata_valid = 1;\n \t\t}\n"},{"id":"291296","messageId":"xmqqr3ay4pzo.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"20160705170558.10906-3-pclouds@gmail.com","subject":"Re: [PATCH 2/5] sha1_file.c: use type off_t* for object_info->disk_sizep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-12T19:26:35Z","receivedAt":"2016-07-12T19:26:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> -\t\t\tstrbuf_addf(sb, \"%lu\", data->disk_size);\n> +\t\t\tstrbuf_addf(sb, \"%\"PRIuMAX, data->disk_size);\n\nSubject: [PATCH] SQUASH???\n\n---\n builtin/cat-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 58ad284..13ed944 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -184,7 +184,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\tif (data->mark_query)\n \t\t\tdata->info.disk_sizep = &data->disk_size;\n \t\telse\n-\t\t\tstrbuf_addf(sb, \"%\"PRIuMAX, data->disk_size);\n+\t\t\tstrbuf_addf(sb, \"%\"PRIuMAX, (uintmax_t)data->disk_size);\n \t} else if (is_atom(\"rest\", atom, len)) {\n \t\tif (data->mark_query)\n \t\t\tdata->split_on_whitespace = 1;\n-- \n2.9.1-500-g4c1e1e0\n\n"},{"id":"291297","messageId":"xmqqmvlm4pyk.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"20160705170558.10906-5-pclouds@gmail.com","subject":"Re: [PATCH 4/5] index-pack: report correct bad object offsets even if they are large","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-12T19:27:15Z","receivedAt":"2016-07-12T19:27:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> \tva_end(params);\n> -\tdie(_(\"pack has bad object at offset %lu: %s\"), offset, buf);\n> +\tdie(_(\"pack has bad object at offset %\"PRIiMAX\": %s\"),\n> +\t (intmax_t)offset, buf);\n\nSubject: [PATCH] SQUASH???\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 73f7cd2..e2d8ae4 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -349,8 +349,8 @@ static NORETURN void bad_object(off_t offset, const char *format, ...)\n \tva_start(params, format);\n \tvsnprintf(buf, sizeof(buf), format, params);\n \tva_end(params);\n-\tdie(_(\"pack has bad object at offset %\"PRIiMAX\": %s\"),\n-\t (intmax_t)offset, buf);\n+\tdie(_(\"pack has bad object at offset %\"PRIuMAX\": %s\"),\n+\t (uintmax_t)offset, buf);\n }\n \n static inline struct thread_local *get_thread_data(void)\n-- \n2.9.1-500-g4c1e1e0\n\n"},{"id":"291302","messageId":"xmqq60sa4mnv.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"Re: [PATCH 0/5] Number truncation with 4+ GB files on 32-bit systems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-12T20:38:28Z","receivedAt":"2016-07-12T20:38:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Since I now could reproduce the problem that Christoph showed, I\n> decided to send the good patches out. To sum up, we use \"unsigned\n> long\" in some places related to file size. On 32-bit systems, it's\n> limited to 32 bits even though the system can handle files larger than\n> that (off_t is 64-bit). This fixes it.\n>\n> clang -Wshorten-64-to-32 is very helpful to spot these problems. I\n> have a couple more patches to clean all these warnings, but some need\n> more code study to see what is the right way to do.\n>\n> Most of the rest seems harmless, except for the local variable \"size\"\n> in builtin/pack-objects.c:write_one(). I might send 6/5 for that one.\n\nWith 1-7/5 it appears t1050 is broken?\n"},{"id":"291327","messageId":"CACsJy8CXg_x_4GRe88yUBYFm-_1=REAox=wK5yzu4W7Mf2qwgQ@mail.gmail.com","threadId":"42706","inReplyTo":"xmqq60sa4mnv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/5] Number truncation with 4+ GB files on 32-bit systems","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-13T06:01:47Z","receivedAt":"2016-07-13T06:04:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 12, 2016 at 10:38 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Since I now could reproduce the problem that Christoph showed, I\n>> decided to send the good patches out. To sum up, we use \"unsigned\n>> long\" in some places related to file size. On 32-bit systems, it's\n>> limited to 32 bits even though the system can handle files larger than\n>> that (off_t is 64-bit). This fixes it.\n>>\n>> clang -Wshorten-64-to-32 is very helpful to spot these problems. I\n>> have a couple more patches to clean all these warnings, but some need\n>> more code study to see what is the right way to do.\n>>\n>> Most of the rest seems harmless, except for the local variable \"size\"\n>> in builtin/pack-objects.c:write_one(). I might send 6/5 for that one.\n>\n> With 1-7/5 it appears t1050 is broken?\n\nOops, sorry I focused on testing big repos and forgot the test suite.\nBut hehe.. it's a good thing. That test was added by 735efde, where\nfsck could keep on going after failing to inflate some blobs (instead\nof hitting malloc failure and abort). Now it's even better, fsck\npasses. I'll fix that up and resend with your squashes in.\n-- \nDuy\n"},{"id":"291347","messageId":"20160713154404.22909-5-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160713154404.22909-1-pclouds@gmail.com","subject":"[PATCH v2 4/7] index-pack: report correct bad object offsets even if they are large","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-13T15:44:01Z","receivedAt":"2016-07-13T15:45:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Use the right type for offsets in this case, off_t, which makes a\ndifference on 32-bit systems with large file support, and change\nformatting code accordingly.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n builtin/index-pack.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex cafaab7..e2d8ae4 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -338,10 +338,10 @@ static void parse_pack_header(void)\n \tuse(sizeof(struct pack_header));\n }\n \n-static NORETURN void bad_object(unsigned long offset, const char *format,\n+static NORETURN void bad_object(off_t offset, const char *format,\n \t\t ...) __attribute__((format (printf, 2, 3)));\n \n-static NORETURN void bad_object(unsigned long offset, const char *format, ...)\n+static NORETURN void bad_object(off_t offset, const char *format, ...)\n {\n \tva_list params;\n \tchar buf[1024];\n@@ -349,7 +349,8 @@ static NORETURN void bad_object(unsigned long offset, const char *format, ...)\n \tva_start(params, format);\n \tvsnprintf(buf, sizeof(buf), format, params);\n \tva_end(params);\n-\tdie(_(\"pack has bad object at offset %lu: %s\"), offset, buf);\n+\tdie(_(\"pack has bad object at offset %\"PRIuMAX\": %s\"),\n+\t (uintmax_t)offset, buf);\n }\n \n static inline struct thread_local *get_thread_data(void)\n-- \n2.9.1.564.gb2f7278\n\n"},{"id":"291348","messageId":"20160713154404.22909-4-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160713154404.22909-1-pclouds@gmail.com","subject":"[PATCH v2 3/7] index-pack: correct \"len\" type in unpack_data()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-13T15:44:00Z","receivedAt":"2016-07-13T15:45:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 32-bit systems with large file support, one entry could be larger\nthan 4GB and overflow \"len\". Correct it so we can unpack a full entry.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n builtin/index-pack.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex e8c71fc..cafaab7 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -549,13 +549,13 @@ static void *unpack_data(struct object_entry *obj,\n \t\t\t void *cb_data)\n {\n \toff_t from = obj[0].idx.offset + obj[0].hdr_size;\n-\tunsigned long len = obj[1].idx.offset - from;\n+\toff_t len = obj[1].idx.offset - from;\n \tunsigned char *data, *inbuf;\n \tgit_zstream stream;\n \tint status;\n \n \tdata = xmallocz(consume ? 64*1024 : obj->size);\n-\tinbuf = xmalloc((len < 64*1024) ? len : 64*1024);\n+\tinbuf = xmalloc((len < 64*1024) ? (int)len : 64*1024);\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n@@ -563,15 +563,15 @@ static void *unpack_data(struct object_entry *obj,\n \tstream.avail_out = consume ? 64*1024 : obj->size;\n \n \tdo {\n-\t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n+\t\tssize_t n = (len < 64*1024) ? (ssize_t)len : 64*1024;\n \t\tn = xpread(get_thread_data()->pack_fd, inbuf, n, from);\n \t\tif (n < 0)\n \t\t\tdie_errno(_(\"cannot pread pack file\"));\n \t\tif (!n)\n-\t\t\tdie(Q_(\"premature end of pack file, %lu byte missing\",\n-\t\t\t \"premature end of pack file, %lu bytes missing\",\n-\t\t\t len),\n-\t\t\t len);\n+\t\t\tdie(Q_(\"premature end of pack file, %\"PRIuMAX\" byte missing\",\n+\t\t\t \"premature end of pack file, %\"PRIuMAX\" bytes missing\",\n+\t\t\t (unsigned int)len),\n+\t\t\t (uintmax_t)len);\n \t\tfrom += n;\n \t\tlen -= n;\n \t\tstream.next_in = inbuf;\n-- \n2.9.1.564.gb2f7278\n\n"},{"id":"291349","messageId":"20160713154404.22909-3-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160713154404.22909-1-pclouds@gmail.com","subject":"[PATCH v2 2/7] sha1_file.c: use type off_t* for object_info->disk_sizep","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-13T15:43:59Z","receivedAt":"2016-07-13T15:45:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This field, filled by sha1_object_info() contains the on-disk size of\nan object, which could go over 4GB limit of unsigned long on 32-bit\nsystems. Use off_t for it instead and update all callers.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n builtin/cat-file.c | 4 ++--\n cache.h | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 54db118..13ed944 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -131,7 +131,7 @@ struct expand_data {\n \tunsigned char sha1[20];\n \tenum object_type type;\n \tunsigned long size;\n-\tunsigned long disk_size;\n+\toff_t disk_size;\n \tconst char *rest;\n \tunsigned char delta_base_sha1[20];\n \n@@ -184,7 +184,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\tif (data->mark_query)\n \t\t\tdata->info.disk_sizep = &data->disk_size;\n \t\telse\n-\t\t\tstrbuf_addf(sb, \"%lu\", data->disk_size);\n+\t\t\tstrbuf_addf(sb, \"%\"PRIuMAX, (uintmax_t)data->disk_size);\n \t} else if (is_atom(\"rest\", atom, len)) {\n \t\tif (data->mark_query)\n \t\t\tdata->split_on_whitespace = 1;\ndiff --git a/cache.h b/cache.h\nindex 4ff196c..ea64b51 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1502,7 +1502,7 @@ struct object_info {\n \t/* Request */\n \tenum object_type *typep;\n \tunsigned long *sizep;\n-\tunsigned long *disk_sizep;\n+\toff_t *disk_sizep;\n \tunsigned char *delta_base_sha1;\n \tstruct strbuf *typename;\n \n-- \n2.9.1.564.gb2f7278\n\n"},{"id":"291350","messageId":"20160713154404.22909-1-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160705170558.10906-1-pclouds@gmail.com","subject":"[PATCH v2 0/7] Number truncation with 4+ GB files on 32-bit systems","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-13T15:43:57Z","receivedAt":"2016-07-13T15:45:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A diff from nd/pack-ofs-4gb-limit can explain the changes better than\nme.\n\nI did not add PRIdMAX or similar because that carries a risk to exotic\nplatforms that people rarely test. Just casting to unsigned should be\nfine.\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 55eac75..b08bc8b 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -356,6 +356,10 @@ static int fsck_sha1(const unsigned char *sha1)\n static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,\n \t\t\t unsigned long size, void *buffer, int *eaten)\n {\n+\t/*\n+\t * Note, buffer may be NULL if type is OBJ_BLOB. See\n+\t * verify_packfile(), data_valid variable for details.\n+\t */\n \tstruct object *obj;\n \tobj = parse_object_buffer(sha1, type, size, buffer, eaten);\n \tif (!obj) {\ndiff --git a/pack-check.c b/pack-check.c\nindex 14e8cb0..d123846 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -106,7 +106,7 @@ static int verify_packfile(struct packed_git *p,\n \t\tenum object_type type;\n \t\tunsigned long size;\n \t\toff_t curpos;\n-\t\tint data_valid = 0;\n+\t\tint data_valid;\n \n \t\tif (p->index_version > 1) {\n \t\t\toff_t offset = entries[i].offset;\n@@ -130,6 +130,7 @@ static int verify_packfile(struct packed_git *p,\n \t\t\t * the data in-core only to discard.\n \t\t\t */\n \t\t\tdata = NULL;\n+\t\t\tdata_valid = 0;\n \t\t} else {\n \t\t\tdata = unpack_entry(p, entries[i].offset, &type, &size);\n \t\t\tdata_valid = 1;\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex f9f3d13..096dbff 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -177,10 +177,9 @@ test_expect_success 'zip achiving, deflate' '\n \tgit archive --format=zip HEAD >/dev/null\n '\n \n-test_expect_success 'fsck' '\n-\ttest_must_fail git fsck 2>err &&\n-\tn=$(grep \"error: attempting to allocate .* over limit\" err | wc -l) &&\n-\ttest \"$n\" -gt 1\n+test_expect_success 'fsck large blobs' '\n+\tgit fsck 2>err &&\n+\ttest_must_be_empty err\n '\n \n test_done\n\nNguyễn Thái Ngọc Duy (7):\n pack-objects: pass length to check_pack_crc() without truncation\n sha1_file.c: use type off_t* for object_info->disk_sizep\n index-pack: correct \"len\" type in unpack_data()\n index-pack: report correct bad object offsets even if they are large\n index-pack: correct \"offset\" type in unpack_entry_data()\n pack-objects: do not truncate result in-pack object size on 32-bit systems\n fsck: use streaming interface for large blobs in pack\n\n builtin/cat-file.c | 4 ++--\n builtin/fsck.c | 4 ++++\n builtin/index-pack.c | 23 ++++++++++++-----------\n builtin/pack-objects.c | 17 +++++++++--------\n cache.h | 2 +-\n pack-check.c | 23 +++++++++++++++++++++--\n pack.h | 1 +\n sha1_file.c | 2 +-\n t/t1050-large.sh | 7 +++----\n 9 files changed, 54 insertions(+), 29 deletions(-)\n\n-- \n2.9.1.564.gb2f7278\n\n"},{"id":"291351","messageId":"20160713154404.22909-6-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160713154404.22909-1-pclouds@gmail.com","subject":"[PATCH v2 5/7] index-pack: correct \"offset\" type in unpack_entry_data()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-13T15:44:02Z","receivedAt":"2016-07-13T15:45:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"unpack_entry_data() receives an off_t value from unpack_raw_entry(),\nwhich could be larger than unsigned long on 32-bit systems with large\nfile support. Correct the type so truncation does not happen. This\nonly affects bad object reporting though.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n builtin/index-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex e2d8ae4..1008d7f 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -430,7 +430,7 @@ static int is_delta_type(enum object_type type)\n \treturn (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);\n }\n \n-static void *unpack_entry_data(unsigned long offset, unsigned long size,\n+static void *unpack_entry_data(off_t offset, unsigned long size,\n \t\t\t enum object_type type, unsigned char *sha1)\n {\n \tstatic char fixed_buf[8192];\n-- \n2.9.1.564.gb2f7278\n\n"},{"id":"291352","messageId":"20160713154404.22909-8-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160713154404.22909-1-pclouds@gmail.com","subject":"[PATCH v2 7/7] fsck: use streaming interface for large blobs in pack","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-13T15:44:04Z","receivedAt":"2016-07-13T15:45:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"For blobs, we want to make sure the on-disk data is not corrupted\n(i.e. can be inflated and produce the expected SHA-1). Blob content is\nopaque, there's nothing else inside to check for.\n\nFor really large blobs, we may want to avoid unpacking the entire blob\nin memory, just to check whether it produces the same SHA-1. On 32-bit\nsystems, we may not have enough virtual address space for such memory\nallocation. And even on 64-bit where it's not a problem, allocating a\nlot more memory could result in kicking other parts of systems to swap\nfile, generating lots of I/O and slowing everything down.\n\nFor this particular operation, not unpacking the blob and letting\ncheck_sha1_signature, which supports streaming interface, do the job\nis sufficient. check_sha1_signature() is not shown in the diff,\nunfortunately. But if will be called when \"data_valid && !data\" is\nfalse.\n\nWe will call the callback function \"fn\" with NULL as \"data\". The only\ncallback of this function is fsck_obj_buffer(), which does not touch\n\"data\" at all if it's a blob.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n builtin/fsck.c | 4 ++++\n pack-check.c | 23 +++++++++++++++++++++--\n pack.h | 1 +\n t/t1050-large.sh | 7 +++----\n 4 files changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 55eac75..b08bc8b 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -356,6 +356,10 @@ static int fsck_sha1(const unsigned char *sha1)\n static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,\n \t\t\t unsigned long size, void *buffer, int *eaten)\n {\n+\t/*\n+\t * Note, buffer may be NULL if type is OBJ_BLOB. See\n+\t * verify_packfile(), data_valid variable for details.\n+\t */\n \tstruct object *obj;\n \tobj = parse_object_buffer(sha1, type, size, buffer, eaten);\n \tif (!obj) {\ndiff --git a/pack-check.c b/pack-check.c\nindex 1da89a4..d123846 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -105,6 +105,8 @@ static int verify_packfile(struct packed_git *p,\n \t\tvoid *data;\n \t\tenum object_type type;\n \t\tunsigned long size;\n+\t\toff_t curpos;\n+\t\tint data_valid;\n \n \t\tif (p->index_version > 1) {\n \t\t\toff_t offset = entries[i].offset;\n@@ -116,8 +118,25 @@ static int verify_packfile(struct packed_git *p,\n \t\t\t\t\t sha1_to_hex(entries[i].sha1),\n \t\t\t\t\t p->pack_name, (uintmax_t)offset);\n \t\t}\n-\t\tdata = unpack_entry(p, entries[i].offset, &type, &size);\n-\t\tif (!data)\n+\n+\t\tcurpos = entries[i].offset;\n+\t\ttype = unpack_object_header(p, w_curs, &curpos, &size);\n+\t\tunuse_pack(w_curs);\n+\n+\t\tif (type == OBJ_BLOB && big_file_threshold <= size) {\n+\t\t\t/*\n+\t\t\t * Let check_sha1_signature() check it with\n+\t\t\t * the streaming interface; no point slurping\n+\t\t\t * the data in-core only to discard.\n+\t\t\t */\n+\t\t\tdata = NULL;\n+\t\t\tdata_valid = 0;\n+\t\t} else {\n+\t\t\tdata = unpack_entry(p, entries[i].offset, &type, &size);\n+\t\t\tdata_valid = 1;\n+\t\t}\n+\n+\t\tif (data_valid && !data)\n \t\t\terr = error(\"cannot unpack %s from %s at offset %\"PRIuMAX\"\",\n \t\t\t\t sha1_to_hex(entries[i].sha1), p->pack_name,\n \t\t\t\t (uintmax_t)entries[i].offset);\ndiff --git a/pack.h b/pack.h\nindex 3223f5a..0e77429 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -74,6 +74,7 @@ struct pack_idx_entry {\n \n \n struct progress;\n+/* Note, the data argument could be NULL if object type is blob */\n typedef int (*verify_fn)(const unsigned char*, enum object_type, unsigned long, void*, int*);\n \n 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);\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex f9f3d13..096dbff 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -177,10 +177,9 @@ test_expect_success 'zip achiving, deflate' '\n \tgit archive --format=zip HEAD >/dev/null\n '\n \n-test_expect_success 'fsck' '\n-\ttest_must_fail git fsck 2>err &&\n-\tn=$(grep \"error: attempting to allocate .* over limit\" err | wc -l) &&\n-\ttest \"$n\" -gt 1\n+test_expect_success 'fsck large blobs' '\n+\tgit fsck 2>err &&\n+\ttest_must_be_empty err\n '\n \n test_done\n-- \n2.9.1.564.gb2f7278\n\n"},{"id":"291353","messageId":"20160713154404.22909-7-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160713154404.22909-1-pclouds@gmail.com","subject":"[PATCH v2 6/7] pack-objects: do not truncate result in-pack object size on 32-bit systems","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-13T15:44:03Z","receivedAt":"2016-07-13T15:45:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A typical diff will not show what's going on and you need to see full\nfunctions. The core code is like this, at the end of of write_one()\n\n\te->idx.offset = *offset;\n\tsize = write_object(f, e, *offset);\n\tif (!size) {\n\t\te->idx.offset = recursing;\n\t\treturn WRITE_ONE_BREAK;\n\t}\n\twritten_list[nr_written++] = &e->idx;\n\n\t/* make sure off_t is sufficiently large not to wrap */\n\tif (signed_add_overflows(*offset, size))\n\t\tdie(\"pack too large for current definition of off_t\");\n\t*offset += size;\n\nHere we can see that the in-pack object size is returned by\nwrite_object (or indirectly by write_reuse_object). And it's used to\ncalculate object offsets, which end up in the pack index file,\ngenerated at the end.\n\nIf \"size\" overflows (on 32-bit sytems, unsigned long is 32-bit while\noff_t can be 64-bit), we got wrong offsets and produce incorrect .idx\nfile, which may make it look like the .pack file is corrupted.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n builtin/pack-objects.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex a3a98c5..ac7a3a5 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -341,8 +341,8 @@ static unsigned long write_no_reuse_object(struct sha1file *f, struct object_ent\n }\n \n /* Return 0 if we will bust the pack-size limit */\n-static unsigned long write_reuse_object(struct sha1file *f, struct object_entry *entry,\n-\t\t\t\t\tunsigned long limit, int usable_delta)\n+static off_t write_reuse_object(struct sha1file *f, struct object_entry *entry,\n+\t\t\t\tunsigned long limit, int usable_delta)\n {\n \tstruct packed_git *p = entry->in_pack;\n \tstruct pack_window *w_curs = NULL;\n@@ -415,11 +415,12 @@ static unsigned long write_reuse_object(struct sha1file *f, struct object_entry\n }\n \n /* Return 0 if we will bust the pack-size limit */\n-static unsigned long write_object(struct sha1file *f,\n-\t\t\t\t struct object_entry *entry,\n-\t\t\t\t off_t write_offset)\n+static off_t write_object(struct sha1file *f,\n+\t\t\t struct object_entry *entry,\n+\t\t\t off_t write_offset)\n {\n-\tunsigned long limit, len;\n+\tunsigned long limit;\n+\toff_t len;\n \tint usable_delta, to_reuse;\n \n \tif (!pack_to_stdout)\n@@ -491,7 +492,7 @@ static enum write_one_status write_one(struct sha1file *f,\n \t\t\t\t struct object_entry *e,\n \t\t\t\t off_t *offset)\n {\n-\tunsigned long size;\n+\toff_t size;\n \tint recursing;\n \n \t/*\n-- \n2.9.1.564.gb2f7278\n\n"},{"id":"291354","messageId":"20160713154404.22909-2-pclouds@gmail.com","threadId":"42706","inReplyTo":"20160713154404.22909-1-pclouds@gmail.com","subject":"[PATCH v2 1/7] pack-objects: pass length to check_pack_crc() without truncation","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-13T15:43:58Z","receivedAt":"2016-07-13T15:45:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 32 bit systems with large file support, unsigned long is 32-bit\nwhile the two offsets in the subtraction expression (pack-objects has\nthe exact same expression as in sha1_file.c but not shown in diff) are\nin 64-bit. If an in-pack object is larger than 2^32 len/datalen is\ntruncated and we get a misleading \"error: bad packed object CRC for\n...\" as a result.\n\nUse off_t for len and datalen. check_pack_crc() already accepts this\nargument as off_t and can deal with 4+ GB.\n\nNoticed-by: Christoph Michelbach \nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n builtin/pack-objects.c | 2 +-\n sha1_file.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex b6664ce..a3a98c5 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -349,7 +349,7 @@ static unsigned long write_reuse_object(struct sha1file *f, struct object_entry\n \tstruct revindex_entry *revidx;\n \toff_t offset;\n \tenum object_type type = entry->type;\n-\tunsigned long datalen;\n+\toff_t datalen;\n \tunsigned char header[10], dheader[10];\n \tunsigned hdrlen;\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex d0f2aa0..cd9b560 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2282,7 +2282,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \n \t\tif (do_check_packed_object_crc && p->index_version > 1) {\n \t\t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n-\t\t\tunsigned long len = revidx[1].offset - obj_offset;\n+\t\t\toff_t len = revidx[1].offset - obj_offset;\n \t\t\tif (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {\n \t\t\t\tconst unsigned char *sha1 =\n \t\t\t\t\tnth_packed_object_sha1(p, revidx->nr);\n-- \n2.9.1.564.gb2f7278\n\n"},{"id":"291361","messageId":"xmqq8tx53448.fsf@gitster.mtv.corp.google.com","threadId":"42706","inReplyTo":"20160713154404.22909-1-pclouds@gmail.com","subject":"Re: [PATCH v2 0/7] Number truncation with 4+ GB files on 32-bit systems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-13T16:16:39Z","receivedAt":"2016-07-13T16:19:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> A diff from nd/pack-ofs-4gb-limit can explain the changes better than\n> me.\n>\n> I did not add PRIdMAX or similar because that carries a risk to exotic\n> platforms that people rarely test. Just casting to unsigned should be\n> fine.\n\nLooks very sensible. Thanks.\n"}]}