{"thread":{"id":"39522","subject":"[PATCH] index-pack: fix truncation of off_t in comparison","startedAt":"2015-06-04T12:35:42Z","lastAt":"2015-06-04T17:28:12Z","messageCount":3,"participants":["Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"262950","messageId":"20150604123541.GA8888@peff.net","threadId":"39522","inReplyTo":null,"subject":"[PATCH] index-pack: fix truncation of off_t in comparison","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-04T12:35:42Z","receivedAt":"2015-06-04T12:35:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit c6458e6 (index-pack: kill union delta_base to save\nmemory, 2015-04-18) refactored the comparison functions used\nin sorting and binary searching our delta list. The\nresulting code does something like:\n\n int cmp_offsets(off_t a, off_t b)\n {\n\t return a - b;\n }\n\nThis works most of the time, but produces nonsensical\nresults when the difference between the two offsets is\nlarger than what can be stored in an \"int\". This can lead to\nunresolved deltas if the packsize is larger than 2G (even on\n64-bit systems, an int is still typically 32 bits):\n\n $ git clone git://github.com/mozilla/gecko-dev\n Cloning into 'gecko-dev'...\n remote: Counting objects: 4800161, done.\n remote: Compressing objects: 100% (178/178), done.\n remote: Total 4800161 (delta 88), reused 0 (delta 0), pack-reused 4799978\n Receiving objects: 100% (4800161/4800161), 2.21 GiB | 3.26 MiB/s, done.\n Resolving deltas: 99% (3808820/3811944), completed with 0 local objects.\n fatal: pack has 3124 unresolved deltas\n fatal: index-pack failed\n\nWe can fix it by doing direct comparisons between the\noffsets and returning constants; the callers only care about\nthe sign of the comparison, not the magnitude.\n\nSigned-off-by: Jeff King \n---\nOn top of nd/slim-index-pack-memory-usage, which introduced the bug (but\nit is already in master).\n\n builtin/index-pack.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 3ed53e3..06dd973 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -616,7 +616,9 @@ static int compare_ofs_delta_bases(off_t offset1, off_t offset2,\n \tint cmp = type1 - type2;\n \tif (cmp)\n \t\treturn cmp;\n-\treturn offset1 - offset2;\n+\treturn offset1 < offset2 ? -1 :\n+\t offset1 > offset2 ? 1 :\n+\t 0;\n }\n \n static int find_ofs_delta(const off_t offset, enum object_type type)\n@@ -1051,7 +1053,9 @@ static int compare_ofs_delta_entry(const void *a, const void *b)\n \tconst struct ofs_delta_entry *delta_a = a;\n \tconst struct ofs_delta_entry *delta_b = b;\n \n-\treturn delta_a->offset - delta_b->offset;\n+\treturn delta_a->offset < delta_b->offset ? -1 :\n+\t delta_a->offset > delta_b->offset ? 1 :\n+\t 0;\n }\n \n static int compare_ref_delta_entry(const void *a, const void *b)\n-- \n2.4.2.745.g0aa058d\n"},{"id":"262953","messageId":"CACsJy8BUEcduXf3LCbwGnJRYNb4s1KEKj_hwdo0qCtJoS4SgFw@mail.gmail.com","threadId":"39522","inReplyTo":"20150604123541.GA8888@peff.net","subject":"Re: [PATCH] index-pack: fix truncation of off_t in comparison","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-04T13:10:03Z","receivedAt":"2015-06-04T13:10:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 4, 2015 at 7:35 PM, Jeff King wrote:\n> Commit c6458e6 (index-pack: kill union delta_base to save\n> memory, 2015-04-18) refactored the comparison functions used\n> in sorting and binary searching our delta list. The\n> resulting code does something like:\n>\n> int cmp_offsets(off_t a, off_t b)\n> {\n> return a - b;\n> }\n>\n> This works most of the time, but produces nonsensical\n> results when the difference between the two offsets is\n> larger than what can be stored in an \"int\".\n\nUgh.. thanks for fixing this. Too bad neither gcc, clang or coverity\nspotted this.\n-- \nDuy\n"},{"id":"262968","messageId":"xmqqlhfzv0wj.fsf@gitster.dls.corp.google.com","threadId":"39522","inReplyTo":"20150604123541.GA8888@peff.net","subject":"Re: [PATCH] index-pack: fix truncation of off_t in comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-04T17:28:12Z","receivedAt":"2015-06-04T17:28:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On top of nd/slim-index-pack-memory-usage, which introduced the bug (but\n> it is already in master).\n\nThanks.\n\nIn this round, I decided to deliberately merge more iffy and larger\ntopics to 'master' in early part of the cycle, and it seems to be\npaying off nicely ;-).\n\nWill queue.\n\n> builtin/index-pack.c | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 3ed53e3..06dd973 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -616,7 +616,9 @@ static int compare_ofs_delta_bases(off_t offset1, off_t offset2,\n> \tint cmp = type1 - type2;\n> \tif (cmp)\n> \t\treturn cmp;\n> -\treturn offset1 - offset2;\n> +\treturn offset1 < offset2 ? -1 :\n> +\t offset1 > offset2 ? 1 :\n> +\t 0;\n> }\n> \n> static int find_ofs_delta(const off_t offset, enum object_type type)\n> @@ -1051,7 +1053,9 @@ static int compare_ofs_delta_entry(const void *a, const void *b)\n> \tconst struct ofs_delta_entry *delta_a = a;\n> \tconst struct ofs_delta_entry *delta_b = b;\n> \n> -\treturn delta_a->offset - delta_b->offset;\n> +\treturn delta_a->offset < delta_b->offset ? -1 :\n> +\t delta_a->offset > delta_b->offset ? 1 :\n> +\t 0;\n> }\n> \n> static int compare_ref_delta_entry(const void *a, const void *b)\n"}]}