{"thread":{"id":"29723","subject":"[PATCH 1/2] Skip SHA-1 collision test on \"index-pack --verify\"","startedAt":"2012-02-24T12:23:20Z","lastAt":"2012-02-26T13:28:34Z","messageCount":11,"participants":["Nguyễn Thái Ngọc Duy","Ian Kumlien","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"185330","messageId":"1330086201-13916-1-git-send-email-pclouds@gmail.com","threadId":"29723","inReplyTo":null,"subject":"[PATCH 1/2] Skip SHA-1 collision test on \"index-pack --verify\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-24T12:23:20Z","receivedAt":"2012-02-24T12:23:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"index-pack --verify (or verify-pack) is about verifying the pack\nitself. SHA-1 collision test is about outside (probably malicious)\nobjects with the same SHA-1 entering current repo.\n\nSHA-1 collision test is currently done unconditionally. Which means if\nyou verify an in-repo pack, all objects from the pack will be checked\nagainst objects in repo, which are themselves.\n\nSkip this test for --verify, unless --strict is also specified.\n\nlinux-2.6 $ ls -sh .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack\n401M .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack\n\nWithout the patch (and with another patch to cut out second pass in\nindex-pack):\n\nlinux-2.6 $ time ~/w/git/old index-pack -v --verify .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack\nIndexing objects: 100% (1944656/1944656), done.\nfatal: pack has 1617280 unresolved deltas\n\nreal 1m1.223s\nuser 0m55.028s\nsys 0m0.828s\n\nWith the patch:\n\nlinux-2.6 $ time ~/w/git/git index-pack -v --verify .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack\nIndexing objects: 100% (1944656/1944656), done.\nfatal: pack has 1617280 unresolved deltas\n\nreal 0m41.714s\nuser 0m40.994s\nsys 0m0.550s\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/index-pack.c | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex dd1c5c9..cee83b9 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -62,6 +62,7 @@ static int nr_resolved_deltas;\n \n static int from_stdin;\n static int strict;\n+static int verify;\n static int verbose;\n \n static struct progress *progress;\n@@ -461,7 +462,7 @@ static void sha1_object(const void *data, unsigned long size,\n \t\t\tenum object_type type, unsigned char *sha1)\n {\n \thash_sha1_file(data, size, typename(type), sha1);\n-\tif (has_sha1_file(sha1)) {\n+\tif ((strict || !verify) && has_sha1_file(sha1)) {\n \t\tvoid *has_data;\n \t\tenum object_type has_type;\n \t\tunsigned long has_size;\n@@ -1078,7 +1079,7 @@ static void show_pack_info(int stat_only)\n \n int cmd_index_pack(int argc, const char **argv, const char *prefix)\n {\n-\tint i, fix_thin_pack = 0, verify = 0, stat_only = 0, stat = 0;\n+\tint i, fix_thin_pack = 0, stat_only = 0, stat = 0;\n \tconst char *curr_pack, *curr_index;\n \tconst char *index_name = NULL, *pack_name = NULL;\n \tconst char *keep_name = NULL, *keep_msg = NULL;\n-- \n1.7.8.36.g69ee2\n"},{"id":"185331","messageId":"1330086201-13916-2-git-send-email-pclouds@gmail.com","threadId":"29723","inReplyTo":"1330086201-13916-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-24T12:23:21Z","receivedAt":"2012-02-24T12:23:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This command unpacks every non-delta objects in order to:\n\n1. calculate sha-1\n2. do byte-to-byte sha-1 collision test if we happen to have objects\n with the same sha-1\n3. validate object content in strict mode\n\nAll this requires the entire object to stay in memory, a bad news for\ngiant blobs. This patch lowers memory consumption by not saving the\nobject in memory whenever possible, calculating SHA-1 while unpacking\nthe object.\n\nThis patch assumes that the collision test is rarely needed. The\ncollision test will be done later in second pass if necessary, which\nputs the entire object back to memory again (We could even do the\ncollision test without putting the entire object back in memory, by\ncomparing as we unpack it).\n\nIn strict mode, it always keeps non-blob objects in memory for\nvalidation (blobs do not need data validation). \"--strict --verify\"\nalso keeps blobs in memory.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Does anybody do \"git index-pack --stdin < .git/objects/pack/something\"?\n\n builtin/index-pack.c | 74 +++++++++++++++++++++++++++++++++++++++++---------\n 1 files changed, 61 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex cee83b9..0c1f915 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -277,30 +277,60 @@ static void unlink_base_data(struct base_data *c)\n \tfree_base_data(c);\n }\n \n-static void *unpack_entry_data(unsigned long offset, unsigned long size)\n+static void *unpack_entry_data(unsigned long offset, unsigned long size,\n+\t\t\t enum object_type type, unsigned char *sha1)\n {\n+\tstatic char fixed_buf[8192];\n \tint status;\n \tgit_zstream stream;\n-\tvoid *buf = xmalloc(size);\n+\tvoid *buf;\n+\tgit_SHA_CTX c;\n+\n+\tif (sha1) {\t\t/* do hash_sha1_file internally */\n+\t\tchar hdr[32];\n+\t\tint hdrlen = sprintf(hdr, \"%s %lu\", typename(type), size)+1;\n+\t\tgit_SHA1_Init(&c);\n+\t\tgit_SHA1_Update(&c, hdr, hdrlen);\n+\n+\t\tbuf = fixed_buf;\n+\t} else {\n+\t\tbuf = xmalloc(size);\n+\t}\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n \tstream.next_out = buf;\n-\tstream.avail_out = size;\n+\tstream.avail_out = buf == fixed_buf ? sizeof(fixed_buf) : size;\n \n \tdo {\n \t\tstream.next_in = fill(1);\n \t\tstream.avail_in = input_len;\n \t\tstatus = git_inflate(&stream, 0);\n \t\tuse(input_len - stream.avail_in);\n+\t\tif (sha1) {\n+\t\t\tgit_SHA1_Update(&c, buf, stream.next_out - (unsigned char *)buf);\n+\t\t\tstream.next_out = buf;\n+\t\t\tstream.avail_out = sizeof(fixed_buf);\n+\t\t}\n \t} while (status == Z_OK);\n \tif (stream.total_out != size || status != Z_STREAM_END)\n \t\tbad_object(offset, \"inflate returned %d\", status);\n \tgit_inflate_end(&stream);\n+\tif (sha1) {\n+\t\tgit_SHA1_Final(sha1, &c);\n+\t\tbuf = NULL;\n+\t}\n \treturn buf;\n }\n \n-static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_base)\n+static int is_delta_type(enum object_type type)\n+{\n+\treturn (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);\n+}\n+\n+static void *unpack_raw_entry(struct object_entry *obj,\n+\t\t\t union delta_base *delta_base,\n+\t\t\t unsigned char *sha1)\n {\n \tunsigned char *p;\n \tunsigned long size, c;\n@@ -360,7 +390,17 @@ static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_\n \t}\n \tobj->hdr_size = consumed_bytes - obj->idx.offset;\n \n-\tdata = unpack_entry_data(obj->idx.offset, obj->size);\n+\t/*\n+\t * --verify --strict: sha1_object() does all collision test\n+\t * --strict: sha1_object() does all except blobs,\n+\t * blobs tested in second pass\n+\t * --verify : no collision test\n+\t * : all in second pass\n+\t */\n+\tif (is_delta_type(obj->type) ||\n+\t (strict && (verify || obj->type != OBJ_BLOB)))\n+\t\tsha1 = NULL;\t/* save unpacked object */\n+\tdata = unpack_entry_data(obj->idx.offset, obj->size, obj->type, sha1);\n \tobj->idx.crc32 = input_crc32;\n \treturn data;\n }\n@@ -461,8 +501,9 @@ static void find_delta_children(const union delta_base *base,\n static void sha1_object(const void *data, unsigned long size,\n \t\t\tenum object_type type, unsigned char *sha1)\n {\n-\thash_sha1_file(data, size, typename(type), sha1);\n-\tif ((strict || !verify) && has_sha1_file(sha1)) {\n+\tif (data)\n+\t\thash_sha1_file(data, size, typename(type), sha1);\n+\tif (data && has_sha1_file(sha1)) {\n \t\tvoid *has_data;\n \t\tenum object_type has_type;\n \t\tunsigned long has_size;\n@@ -511,11 +552,6 @@ static void sha1_object(const void *data, unsigned long size,\n \t}\n }\n \n-static int is_delta_type(enum object_type type)\n-{\n-\treturn (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);\n-}\n-\n /*\n * This function is part of find_unresolved_deltas(). There are two\n * walkers going in the opposite ways.\n@@ -702,7 +738,7 @@ static void parse_pack_objects(unsigned char *sha1)\n \t\t\t\tnr_objects);\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tstruct object_entry *obj = &objects[i];\n-\t\tvoid *data = unpack_raw_entry(obj, &delta->base);\n+\t\tvoid *data = unpack_raw_entry(obj, &delta->base, obj->idx.sha1);\n \t\tobj->real_type = obj->type;\n \t\tif (is_delta_type(obj->type)) {\n \t\t\tnr_deltas++;\n@@ -744,6 +780,9 @@ static void parse_pack_objects(unsigned char *sha1)\n \t * - if used as a base, uncompress the object and apply all deltas,\n \t * recursively checking if the resulting object is used as a base\n \t * for some more deltas.\n+\t * - if the same object exists in repository and we're not in strict\n+\t * mode, we skipped the sha-1 collision test in the first pass.\n+\t * Do it now.\n \t */\n \tif (verbose)\n \t\tprogress = start_progress(\"Resolving deltas\", nr_deltas);\n@@ -753,6 +792,15 @@ static void parse_pack_objects(unsigned char *sha1)\n \n \t\tif (is_delta_type(obj->type))\n \t\t\tcontinue;\n+\n+\t\tif (((!strict && !verify) ||\n+\t\t (strict && !verify && obj->type == OBJ_BLOB)) &&\n+\t\t has_sha1_file(obj->idx.sha1)) {\n+\t\t\tvoid *data = get_data_from_pack(obj);\n+\t\t\tsha1_object(data, obj->size, obj->type, obj->idx.sha1);\n+\t\t\tfree(data);\n+\t\t}\n+\n \t\tbase_obj->obj = obj;\n \t\tbase_obj->data = NULL;\n \t\tfind_unresolved_deltas(base_obj);\n-- \n1.7.8.36.g69ee2\n"},{"id":"185335","messageId":"20120224143042.GE9526@pomac.netswarm.net","threadId":"29723","inReplyTo":"1330086201-13916-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs","fromName":"Ian Kumlien","fromEmail":"pomac@vapor.com","sentAt":"2012-02-24T14:30:42Z","receivedAt":"2012-02-24T14:30:42Z","isPatch":true,"sender":{"key":"pomac@vapor.com","avatar":null},"body":"On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> This command unpacks every non-delta objects in order to:\n> \n> 1. calculate sha-1\n> 2. do byte-to-byte sha-1 collision test if we happen to have objects\n> with the same sha-1\n> 3. validate object content in strict mode\n> \n> All this requires the entire object to stay in memory, a bad news for\n> giant blobs. This patch lowers memory consumption by not saving the\n> object in memory whenever possible, calculating SHA-1 while unpacking\n> the object.\n> \n> This patch assumes that the collision test is rarely needed. The\n> collision test will be done later in second pass if necessary, which\n> puts the entire object back to memory again (We could even do the\n> collision test without putting the entire object back in memory, by\n> comparing as we unpack it).\n> \n> In strict mode, it always keeps non-blob objects in memory for\n> validation (blobs do not need data validation). \"--strict --verify\"\n> also keeps blobs in memory.\n\nI applied both patches to git master, with some manual tinkering so i\nmight have missed some change that caused this to break.\n\nBut i get a segmentation fault and i just thought that i'd send you a\nsmall trace before i even start trying to look in to this:\n0xb7eb5b43 in SHA1_Update () from /lib/i686/cmov/libcrypto.so.0.9.8\n(gdb) bt\n#0 0xb7eb5b43 in SHA1_Update () from /lib/i686/cmov/libcrypto.so.0.9.8\n#1 0x08116a2d in write_sha1_file_prepare\n#2 0x08116a83 in hash_sha1_file\n#3 0x0807c2a6 in sha1_object \n#4 0x0807d74a in parse_pack_objects\n#5 0x0807de6f in cmd_index_pack \n#6 0x0804be97 in run_builtin \n#7 handle_internal_command \n#8 0x0804c0ad in run_argv \n#9 main\n\nSorry about the censorship but i don't know how sensetive this data\nis...\n\n\nsha1_file.c:2343\n---\nstatic void write_sha1_file_prepare(const void *buf, unsigned long len,\n const char *type, unsigned char *sha1,\n char *hdr, int *hdrlen)\n{\n git_SHA_CTX c;\n\n /* Generate the header */\n *hdrlen = sprintf(hdr, \"%s %lu\", type, len)+1;\n\n /* Sha1.. */\n git_SHA1_Init(&c);\n git_SHA1_Update(&c, hdr, *hdrlen);\n git_SHA1_Update(&c, buf, len); <== this line fails.\n git_HA1_Final(sha1, &c);\n}\n---\n\nJust keep sending patches, i have atleast one git to test it on. ;)\n"},{"id":"185337","messageId":"20120224144052.GF9526@pomac.netswarm.net","threadId":"29723","inReplyTo":"1330086201-13916-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs","fromName":"Ian Kumlien","fromEmail":"pomac@vapor.com","sentAt":"2012-02-24T14:40:52Z","receivedAt":"2012-02-24T14:40:52Z","isPatch":true,"sender":{"key":"pomac@vapor.com","avatar":null},"body":"On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> This command unpacks every non-delta objects in order to:\n> \n> 1. calculate sha-1\n> 2. do byte-to-byte sha-1 collision test if we happen to have objects\n> with the same sha-1\n> 3. validate object content in strict mode\n> \n> All this requires the entire object to stay in memory, a bad news for\n> giant blobs. This patch lowers memory consumption by not saving the\n> object in memory whenever possible, calculating SHA-1 while unpacking\n> the object.\n> \n> This patch assumes that the collision test is rarely needed. The\n> collision test will be done later in second pass if necessary, which\n> puts the entire object back to memory again (We could even do the\n> collision test without putting the entire object back in memory, by\n> comparing as we unpack it).\n> \n> In strict mode, it always keeps non-blob objects in memory for\n> validation (blobs do not need data validation). \"--strict --verify\"\n> also keeps blobs in memory.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nActually, nevermind my last report - i had missed a merge :(\n\nAnd now that i merged that part it seems like it doesn't do much..\n(No real output for 2+ minutes)\n\nI think i should reapply the patches again and verify that everything is\ncorrect before reporting any additional progress.\n\nBut, this might not be before monday, unfortunately... But *thanks* for\nposting the patches!\n"},{"id":"185338","messageId":"20120224153753.GG9526@pomac.netswarm.net","threadId":"29723","inReplyTo":"1330086201-13916-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs","fromName":"Ian Kumlien","fromEmail":"pomac@vapor.com","sentAt":"2012-02-24T15:37:53Z","receivedAt":"2012-02-24T15:37:53Z","isPatch":true,"sender":{"key":"pomac@vapor.com","avatar":null},"body":"On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> This command unpacks every non-delta objects in order to:\n> \n> 1. calculate sha-1\n> 2. do byte-to-byte sha-1 collision test if we happen to have objects\n> with the same sha-1\n> 3. validate object content in strict mode\n> \n> All this requires the entire object to stay in memory, a bad news for\n> giant blobs. This patch lowers memory consumption by not saving the\n> object in memory whenever possible, calculating SHA-1 while unpacking\n> the object.\n> \n> This patch assumes that the collision test is rarely needed. The\n> collision test will be done later in second pass if necessary, which\n> puts the entire object back to memory again (We could even do the\n> collision test without putting the entire object back in memory, by\n> comparing as we unpack it).\n> \n> In strict mode, it always keeps non-blob objects in memory for\n> validation (blobs do not need data validation). \"--strict --verify\"\n> also keeps blobs in memory.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nFinally, reapplied the patches and so on:\nremote: Counting objects: 1425, done.\nremote: Compressing objects: 100% (617/617), done.\nremote: Total 1425 (delta 790), reused 1425 (delta 790)\nReceiving objects: 100% (1425/1425), 56.06 MiB | 3.97 MiB/s, done.\nResolving deltas: 100% (790/790), done.\n\nreal\t1m57.742s\nuser\t0m29.950s\nsys\t0m6.308s\n\n*YAY*\n\nI wonder how the hell i could have missed several parts of the patch =(\n\nBut there seems to be some issue in gerrit 2.1.8, will have to check\nagainst a newer gerrit to verify if it's still a problem.\n\nFYI - it seems to hang doing nothing.\n\nAs for your patches:\nTested-by: Ian Kumlien \n\n;)\n"},{"id":"185339","messageId":"20120224161613.GH9526@pomac.netswarm.net","threadId":"29723","inReplyTo":"1330086201-13916-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs","fromName":"Ian Kumlien","fromEmail":"pomac@vapor.com","sentAt":"2012-02-24T16:16:13Z","receivedAt":"2012-02-24T16:16:13Z","isPatch":true,"sender":{"key":"pomac@vapor.com","avatar":null},"body":"On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> This command unpacks every non-delta objects in order to:\n> \n> 1. calculate sha-1\n> 2. do byte-to-byte sha-1 collision test if we happen to have objects\n> with the same sha-1\n> 3. validate object content in strict mode\n> \n> All this requires the entire object to stay in memory, a bad news for\n> giant blobs. This patch lowers memory consumption by not saving the\n> object in memory whenever possible, calculating SHA-1 while unpacking\n> the object.\n> \n> This patch assumes that the collision test is rarely needed. The\n> collision test will be done later in second pass if necessary, which\n> puts the entire object back to memory again (We could even do the\n> collision test without putting the entire object back in memory, by\n> comparing as we unpack it).\n> \n> In strict mode, it always keeps non-blob objects in memory for\n> validation (blobs do not need data validation). \"--strict --verify\"\n> also keeps blobs in memory.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nWriting objects: 100% (1425/1425), 56.06 MiB | 4.62 MiB/s, done.\nTotal 1425 (delta 790), reused 1425 (delta 790)\nfatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\nfatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\nfatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\nfatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\nTo ../test_data/\n ! [remote rejected] master -> master (missing necessary objects)\n ! [remote rejected] origin/HEAD -> origin/HEAD (missing necessary objects)\n ! [remote rejected] origin/master -> origin/master (missing necessary objects)\nerror: failed to push some refs to '../test_data/'\n\nSo there are additional code paths to look at... =( \n"},{"id":"185401","messageId":"CACsJy8C-8dvXpNTU=JpdupSpS8OuqqTpGvDs6s1ASeKdk9d5Dg@mail.gmail.com","threadId":"29723","inReplyTo":"20120224161613.GH9526@pomac.netswarm.net","subject":"Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-25T01:49:55Z","receivedAt":"2012-02-25T01:49:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2012/2/24 Ian Kumlien :\n> Writing objects: 100% (1425/1425), 56.06 MiB | 4.62 MiB/s, done.\n> Total 1425 (delta 790), reused 1425 (delta 790)\n> fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> To ../test_data/\n>  ! [remote rejected] master -> master (missing necessary objects)\n>  ! [remote rejected] origin/HEAD -> origin/HEAD (missing necessary objects)\n>  ! [remote rejected] origin/master -> origin/master (missing necessary objects)\n> error: failed to push some refs to '../test_data/'\n>\n> So there are additional code paths to look at... =(\n\nI can't say where that came from. Does this help? (Space damaged, may\nneed manual application)\n\n-- 8< --\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 264e3ae..6dc46eb 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -183,7 +183,8 @@ static void show_object(struct object *obj,\n struct rev_list_info *info = cb_data;\n\n finish_object(obj, path, component, cb_data);\n- if (info->revs->verify_objects && !obj->parsed && obj->type !=\nOBJ_COMMIT)\n+ if (info->revs->verify_objects && !obj->parsed &&\n+ obj->type != OBJ_COMMIT && obj->type != OBJ_BLOB)\n parse_object(obj->sha1);\n show_object_with_name(stdout, obj, path, component);\n }\n-- 8< --\n\nIf not, you might need to apply this to generate coredump, then look\nand see where that failed malloc comes from\n\n-- 8< --\ndiff --git a/wrapper.c b/wrapper.c\nindex 85f09df..03f423e 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -40,9 +40,11 @@ void *xmalloc(size_t size)\n ret = malloc(size);\n if (!ret && !size)\n ret = malloc(1);\n- if (!ret)\n+ if (!ret) {\n+ *(char*)0 = 1;\n die(\"Out of memory, malloc failed (tried to\nallocate %lu bytes)\",\n (unsigned long)size);\n+ }\n }\n #ifdef XMALLOC_POISON\n memset(ret, 0xA5, size);\n-- 8< --\n\n-- \nDuy\n"},{"id":"185429","messageId":"20120225131708.GI9526@pomac.netswarm.net","threadId":"29723","inReplyTo":"CACsJy8C-8dvXpNTU=JpdupSpS8OuqqTpGvDs6s1ASeKdk9d5Dg@mail.gmail.com","subject":"Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs","fromName":"Ian Kumlien","fromEmail":"pomac@vapor.com","sentAt":"2012-02-25T13:17:08Z","receivedAt":"2012-02-25T13:17:08Z","isPatch":true,"sender":{"key":"pomac@vapor.com","avatar":null},"body":"On Sat, Feb 25, 2012 at 08:49:55AM +0700, Nguyen Thai Ngoc Duy wrote:\n> 2012/2/24 Ian Kumlien :\n> > Writing objects: 100% (1425/1425), 56.06 MiB | 4.62 MiB/s, done.\n> > Total 1425 (delta 790), reused 1425 (delta 790)\n> > fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> > fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> > fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> > fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> > To ../test_data/\n> >  ! [remote rejected] master -> master (missing necessary objects)\n> >  ! [remote rejected] origin/HEAD -> origin/HEAD (missing necessary objects)\n> >  ! [remote rejected] origin/master -> origin/master (missing necessary objects)\n> > error: failed to push some refs to '../test_data/'\n> >\n> > So there are additional code paths to look at... =(\n> \n> I can't say where that came from. Does this help? (Space damaged, may\n> need manual application)\n\nEverything has so far, since i'm using mainline to get the gzip fixes in\n;)\n\nAnyway, with:\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 264e3ae..533081d 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -183,7 +183,8 @@ static void show_object(struct object *obj,\n struct rev_list_info *info = cb_data;\n\n finish_object(obj, path, component, cb_data);\n- if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n+ if (info->revs->verify_objects && !obj->parsed\n+ && obj->type != OBJ_COMMIT && obj->type != OBJ_BLOB)\n parse_object(obj->sha1);\n show_object_with_name(stdout, obj, path, component);\n }\n---\n\nI get:\n../git/git push --mirror ../test_data/\nCounting objects: 1425, done.\nDelta compression using up to 2 threads.\nCompressing objects: 100% (617/617), done.\nWriting objects: 100% (1425/1425), 56.06 MiB | 4.22 MiB/s, done.\nTotal 1425 (delta 790), reused 1425 (delta 790)\nerror: index-pack died of signal 11\nerror: unpack failed: index-pack abnormal exit\nTo ../test_data/\n ! [remote rejected] master -> master (n/a (unpacker error))\n ! [remote rejected] origin/HEAD -> origin/HEAD (n/a (unpacker error))\n ! [remote rejected] origin/master -> origin/master (n/a (unpacker error))\nerror: failed to push some refs to '../test_data/'\n\nWhich, to me, means that the installed git is now the problem - it can't verify \nthe pack and say that it's all ok ;)\n\nI'll have to look some more at this on monday, or during the weekend if i get too curious =)\n\nFor now, thank $deity that $company i work for allows VPN from Linux machines! It looks\nreally good, i wonder if there is further tests i should do - any clues?\n\n\n> -- \n> Duy\n"},{"id":"185442","messageId":"20120225224533.GJ9526@pomac.netswarm.net","threadId":"29723","inReplyTo":"CACsJy8C-8dvXpNTU=JpdupSpS8OuqqTpGvDs6s1ASeKdk9d5Dg@mail.gmail.com","subject":"Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs","fromName":"Ian Kumlien","fromEmail":"pomac@vapor.com","sentAt":"2012-02-25T22:45:33Z","receivedAt":"2012-02-25T22:45:33Z","isPatch":true,"sender":{"key":"pomac@vapor.com","avatar":null},"body":"On Sat, Feb 25, 2012 at 08:49:55AM +0700, Nguyen Thai Ngoc Duy wrote:\n> 2012/2/24 Ian Kumlien :\n> > Writing objects: 100% (1425/1425), 56.06 MiB | 4.62 MiB/s, done.\n> > Total 1425 (delta 790), reused 1425 (delta 790)\n> > fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> > fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> > fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> > fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)\n> > To ../test_data/\n> >  ! [remote rejected] master -> master (missing necessary objects)\n> >  ! [remote rejected] origin/HEAD -> origin/HEAD (missing necessary objects)\n> >  ! [remote rejected] origin/master -> origin/master (missing necessary objects)\n> > error: failed to push some refs to '../test_data/'\n> >\n> > So there are additional code paths to look at... =(\n> \n> I can't say where that came from. Does this help? (Space damaged, may\n> need manual application)\n\n> If not, you might need to apply this to generate coredump, then look\n> and see where that failed malloc comes from\n\nActually, i added a backtrace and used addr2line to confirm my\nsuspicion... which is:\nbuiltin/index-pack.c:414\n\nie get_data_from_pack... \n\nIt looks to me like, if we are to support this kind of things, we need a\nslightly different approach - instead of passing the data around, it\nfeels like passing a function pointer around would be beneficial.\n\nLooking at the code i see alot of places where this would be a issue,\njust the fact that get_data_from_pack is used in several functions that\nmight do some small operation and then just free it.\n\nI understand and recognize that my \"problem\" is not what git was\ndesigned for; it was designed for small files, which is very evident in\nhow it approaches the data... And I'd most definetly have to look alot\ncloser to this code... =)\n\n> -- \n> Duy\n"},{"id":"185448","messageId":"CACsJy8Cncs8RYiSB0N20vy9zu2NRTTHpfw3rSfmW64i-4_wxSw@mail.gmail.com","threadId":"29723","inReplyTo":"20120225224533.GJ9526@pomac.netswarm.net","subject":"Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-26T04:10:14Z","receivedAt":"2012-02-26T04:10:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 26, 2012 at 5:45 AM, Ian Kumlien wrote:\n> Actually, i added a backtrace and used addr2line to confirm my\n> suspicion... which is:\n> builtin/index-pack.c:414\n>\n> ie get_data_from_pack...\n\nThat function should only be called when objects are deltified, which\nshould _not_ happen for large blobs. What is its caller?\n\n>\n> It looks to me like, if we are to support this kind of things, we need a\n> slightly different approach - instead of passing the data around, it\n> feels like passing a function pointer around would be beneficial.\n>\n> Looking at the code i see alot of places where this would be a issue,\n> just the fact that get_data_from_pack is used in several functions that\n> might do some small operation and then just free it.\n>\n> I understand and recognize that my \"problem\" is not what git was\n> designed for; it was designed for small files, which is very evident in\n> how it approaches the data... And I'd most definetly have to look alot\n> closer to this code... =)\n>\n>> --\n>> Duy\n\n\n\n-- \nDuy\n"},{"id":"185454","messageId":"20120226132834.GK9526@pomac.netswarm.net","threadId":"29723","inReplyTo":"CACsJy8Cncs8RYiSB0N20vy9zu2NRTTHpfw3rSfmW64i-4_wxSw@mail.gmail.com","subject":"Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs","fromName":"Ian Kumlien","fromEmail":"pomac@vapor.com","sentAt":"2012-02-26T13:28:34Z","receivedAt":"2012-02-26T13:28:34Z","isPatch":true,"sender":{"key":"pomac@vapor.com","avatar":null},"body":"On Sun, Feb 26, 2012 at 11:10:14AM +0700, Nguyen Thai Ngoc Duy wrote:\n> On Sun, Feb 26, 2012 at 5:45 AM, Ian Kumlien wrote:\n> > Actually, i added a backtrace and used addr2line to confirm my\n> > suspicion... which is:\n> > builtin/index-pack.c:414\n> >\n> > ie get_data_from_pack...\n> \n> That function should only be called when objects are deltified, which\n> should _not_ happen for large blobs. What is its caller?\n\nFull backtrace:\n\nfor x in 0x536031 0x451b0e 0x452212 0x4523f5 0x452711 0x452799 0x452bbb\n0x454344 0x4170d1 0x41726c ; do addr2line $x -e ../git/git ; done\ngit/wrapper.c:41\ngit/builtin/index-pack.c:414\ngit/builtin/index-pack.c:588\ngit/builtin/index-pack.c:625\ngit/builtin/index-pack.c:679\ngit/builtin/index-pack.c:694\ngit/builtin/index-pack.c:805\ngit/builtin/index-pack.c:1246\ngit/git.c:308\ngit/git.c:467\n\nWhich means:\nxmalloc\nget_data_from_pack\nget_base_data -- line just after: if (!delta_nr) {\nresolve_delta\nfind_unresolved_deltas_1\nfind_unresolved_deltas\nparse_pack_objects\ncmd_index_pack\n[skipping the git.c part]\n\nBtw, i'm running these tests on a 64 bit laptop - since i'm not at work\n;) (had to manually limit xmalloc but it triggers at the same point)\n"}]}