{"thread":{"id":"30946","subject":"[BUG] serious inflate inconsistency on master","startedAt":"2012-07-03T22:19:01Z","lastAt":"2012-07-04T07:24:12Z","messageCount":7,"participants":["Jeff King","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"194585","messageId":"20120703221900.GA28897@sigill.intra.peff.net","threadId":"30946","inReplyTo":null,"subject":"[BUG] serious inflate inconsistency on master","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-03T22:19:01Z","receivedAt":"2012-07-03T22:19:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I'm getting a 'serious inflate consistency' error while running \"git\nverify-pack\" (actually, \"git index-pack --verify\" under the hood). It\nbisects to 4614043 (index-pack: use streaming interface for collision\ntest on large blobs, 2012-05-24).\n\nThe interesting thing about this repository is that it has a 2.8G text\nfile in it which compresses down to only about 420M. I'm not sure that\n4614043 actually introduces the bug, but rather just triggers the code\npath.\n\nI'm able to reproduce it with the following script:\n\n  # empty repo...\n  git init repo &&\n  cd repo &&\n\n  # set this low to make sure we follow the unpack_data code-path\n  git config core.bigfilethreshold 100k &&\n\n  # now make a file bigger than our threshold, but that will compress\n  # well\n  perl -le 'print for (1..100000)' >file &&\n\n  # and then make a commit\n  git add file &&\n  git commit -m file &&\n\n  # and a pack with it\n  git repack -ad &&\n\n  # and then verify that pack\n  git verify-pack .git/objects/pack/*.pack\n\nThe problem seems to be in index-pack.c:unpack_data, which does this:\n\n>\tgit_inflate_init(&stream);\n>\tstream.next_out = data;\n>\tstream.avail_out = consume ? 64*1024 : obj->size;\n>\n>\tdo {\n>\t\tunsigned char *last_out = stream.next_out;\n>\t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n>\t\tn = pread(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\tfrom += n;\n>\t\tlen -= n;\n>\t\tstream.next_in = inbuf;\n>\t\tstream.avail_in = n;\n>\t\tstatus = git_inflate(&stream, 0);\n>\t\tif (consume) {\n>\t\t\tif (consume(last_out, stream.next_out - last_out, cb_data)) {\n>\t\t\t\tfree(inbuf);\n>\t\t\t\tfree(data);\n>\t\t\t\treturn NULL;\n>\t\t\t}\n>\t\t\tstream.next_out = data;\n>\t\t\tstream.avail_out = 64*1024;\n>\t\t}\n>\t} while (len && status == Z_OK && !stream.avail_in);\n>\n>\t/* This has been inflated OK when first encountered, so... */\n>\tif (status != Z_STREAM_END || stream.total_out != obj->size)\n>\t\tdie(_(\"serious inflate inconsistency\"));\n\nWe limit ourselves to handling just 64K at a time. So we read in 64K and\nstuff it in the next_in/avail_in buffer. And then we make 64K of buffer\navailable for zlib to write into via the next_out/avail_out buffer. So\nzlib reads the first chunk, and after reading 28K or so fills up the 64K\noutput buffer and returns. We call consume on the chunk, but when we hit\nthe outer loop condition, stream.avail_in still mentions the 36K we\nhaven't processed yet, and the loop ends with status == Z_OK, which\ntriggers the assertion below it.\n\nSo I don't really understand what this !stream.avail_in is doing there\nin the do-while loop.  Don't we instead need to have an inner loop that\nkeeps feeding the result of pread into git_inflate until we don't have\nany available data left?\n\nSomething like the patch below, which seems to work for me, but I still\ndon't understand the function of the !stream.avail_in check in the outer\nloop.\n\n-Peff\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8b5c1eb..0db1923 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -538,15 +538,19 @@ static void *unpack_data(struct object_entry *obj,\n \t\tlen -= n;\n \t\tstream.next_in = inbuf;\n \t\tstream.avail_in = n;\n-\t\tstatus = git_inflate(&stream, 0);\n-\t\tif (consume) {\n-\t\t\tif (consume(last_out, stream.next_out - last_out, cb_data)) {\n-\t\t\t\tfree(inbuf);\n-\t\t\t\tfree(data);\n-\t\t\t\treturn NULL;\n-\t\t\t}\n-\t\t\tstream.next_out = data;\n-\t\t\tstream.avail_out = 64*1024;\n+\t\tif (!consume)\n+\t\t\tstatus = git_inflate(&stream, 0);\n+\t\telse {\n+\t\t\tdo {\n+\t\t\t\tstatus = git_inflate(&stream, 0);\n+\t\t\t\tif (consume(last_out, stream.next_out - last_out, cb_data)) {\n+\t\t\t\t\tfree(inbuf);\n+\t\t\t\t\tfree(data);\n+\t\t\t\t\treturn NULL;\n+\t\t\t\t}\n+\t\t\t\tstream.next_out = data;\n+\t\t\t\tstream.avail_out = 64*1024;\n+\t\t\t} while (status == Z_OK && stream.avail_in);\n \t\t}\n \t} while (len && status == Z_OK && !stream.avail_in);\n \n"},{"id":"194586","messageId":"7vipe4tqns.fsf@alter.siamese.dyndns.org","threadId":"30946","inReplyTo":"20120703221900.GA28897@sigill.intra.peff.net","subject":"Re: [BUG] serious inflate inconsistency on master","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-03T22:40:07Z","receivedAt":"2012-07-03T22:40:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The problem seems to be in index-pack.c:unpack_data, which does this:\n>\n>>\tgit_inflate_init(&stream);\n>>\tstream.next_out = data;\n>>\tstream.avail_out = consume ? 64*1024 : obj->size;\n>>\n>>\tdo {\n>>\t\tunsigned char *last_out = stream.next_out;\n>>\t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n>>\t\tn = pread(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\tfrom += n;\n>>\t\tlen -= n;\n>>\t\tstream.next_in = inbuf;\n>>\t\tstream.avail_in = n;\n>>\t\tstatus = git_inflate(&stream, 0);\n>>\t\tif (consume) {\n>>\t\t\tif (consume(last_out, stream.next_out - last_out, cb_data)) {\n>>\t\t\t\tfree(inbuf);\n>>\t\t\t\tfree(data);\n>>\t\t\t\treturn NULL;\n>>\t\t\t}\n>>\t\t\tstream.next_out = data;\n>>\t\t\tstream.avail_out = 64*1024;\n>>\t\t}\n>>\t} while (len && status == Z_OK && !stream.avail_in);\n>>\n>>\t/* This has been inflated OK when first encountered, so... */\n>>\tif (status != Z_STREAM_END || stream.total_out != obj->size)\n>>\t\tdie(_(\"serious inflate inconsistency\"));\n\nYeah, that \"if (consume)\" part is clearly bogus.  It should feed\nwhat it read in \"inbuf\" fully to git_inflate() in a (missing) inner\nloop and keep feeding consume() with the inflated data, but instead\nhappily goes on to read more from the packfile once the initial part\nof the inflated data is fed to consume, ignoring the remainder.\n\n> So I don't really understand what this !stream.avail_in is doing there\n> in the do-while loop.  Don't we instead need to have an inner loop that\n> keeps feeding the result of pread into git_inflate until we don't have\n> any available data left?\n\nExactly.  I do not think the avain_in check should be done at the\nend of the outer loop at all.  When we are buffering the entire\ninflated data in core without using consume, we allocate enough\nmemory to \"data\" to hold the whole thing, so in that case, it may be\nOK to expect that git_inflate() would never return without consuming\nthe input bytes (i.e. stream.avail_in would always be zero at the\nsite of the check), but with consume(), we give small piece of\nmemory as a temporary output area and call git_inflate(), and it is\nentirely possible and normal for it to run out of output before\ninflating all the input bytes.\n"},{"id":"194595","messageId":"CACsJy8AFs6PV8XCAUar9KadE8g-WSaZofKQAGPB6VjbhVpSRig@mail.gmail.com","threadId":"30946","inReplyTo":"7vipe4tqns.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] serious inflate inconsistency on master","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-07-04T05:35:34Z","receivedAt":"2012-07-04T05:35:34Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 4, 2012 at 5:40 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> The problem seems to be in index-pack.c:unpack_data, which does this:\n>>\n>>>      git_inflate_init(&stream);\n>>>      stream.next_out = data;\n>>>      stream.avail_out = consume ? 64*1024 : obj->size;\n>>>\n>>>      do {\n>>>              unsigned char *last_out = stream.next_out;\n>>>              ssize_t n = (len < 64*1024) ? len : 64*1024;\n>>>              n = pread(pack_fd, inbuf, n, from);\n>>>              if (n < 0)\n>>>                      die_errno(_(\"cannot pread pack file\"));\n>>>              if (!n)\n>>>                      die(Q_(\"premature end of pack file, %lu byte missing\",\n>>>                             \"premature end of pack file, %lu bytes missing\",\n>>>                             len),\n>>>                          len);\n>>>              from += n;\n>>>              len -= n;\n>>>              stream.next_in = inbuf;\n>>>              stream.avail_in = n;\n>>>              status = git_inflate(&stream, 0);\n>>>              if (consume) {\n>>>                      if (consume(last_out, stream.next_out - last_out, cb_data)) {\n>>>                              free(inbuf);\n>>>                              free(data);\n>>>                              return NULL;\n>>>                      }\n>>>                      stream.next_out = data;\n>>>                      stream.avail_out = 64*1024;\n>>>              }\n>>>      } while (len && status == Z_OK && !stream.avail_in);\n>>>\n>>>      /* This has been inflated OK when first encountered, so... */\n>>>      if (status != Z_STREAM_END || stream.total_out != obj->size)\n>>>              die(_(\"serious inflate inconsistency\"));\n>\n> Yeah, that \"if (consume)\" part is clearly bogus.  It should feed\n> what it read in \"inbuf\" fully to git_inflate() in a (missing) inner\n> loop and keep feeding consume() with the inflated data, but instead\n> happily goes on to read more from the packfile once the initial part\n> of the inflated data is fed to consume, ignoring the remainder.\n\nAgreed.\n\n>> So I don't really understand what this !stream.avail_in is doing there\n>> in the do-while loop.  Don't we instead need to have an inner loop that\n>> keeps feeding the result of pread into git_inflate until we don't have\n>> any available data left?\n>\n> Exactly.  I do not think the avain_in check should be done at the\n> end of the outer loop at all.  When we are buffering the entire\n> inflated data in core without using consume, we allocate enough\n> memory to \"data\" to hold the whole thing, so in that case, it may be\n> OK to expect that git_inflate() would never return without consuming\n> the input bytes (i.e. stream.avail_in would always be zero at the\n> site of the check), but with consume(), we give small piece of\n> memory as a temporary output area and call git_inflate(), and it is\n> entirely possible and normal for it to run out of output before\n> inflating all the input bytes.\n\nMaybe the !stream.avail_in check is for safety? obj->size could be\nwrong due to an index-pack bug, leading to insufficient avail_out..\n\nBy the way I searched the commit that introduces that check with \"git\nlog --follow -p builtin/index-pack.c\" but I could not find it. What\ndid I do wrong?\n-- \nDuy\n"},{"id":"194596","messageId":"7vy5n0rq9c.fsf@alter.siamese.dyndns.org","threadId":"30946","inReplyTo":"CACsJy8AFs6PV8XCAUar9KadE8g-WSaZofKQAGPB6VjbhVpSRig@mail.gmail.com","subject":"Re: [BUG] serious inflate inconsistency on master","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-04T06:31:43Z","receivedAt":"2012-07-04T06:31:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> By the way I searched the commit that introduces that check with \"git\n> log --follow -p builtin/index-pack.c\" but I could not find it. What\n> did I do wrong?\n\nYour commit 8a2e163cc shows changes to the file at ll.535-540; these\ncome from 776ea370 builtin-index-pack.c ll.383-388.\n\n  $ git show 776ea370:builtin-index-pack.c\n\nThe get_data_from_pack() function in that commit gives sufficient\nbuffer to output side (avail_out starts out as obj->size), and feeds\nthe data from the packfile in chunks.  With the arrangement this\ncommit makes to call git_inflate(), it should never get stuck\nbecause it ran out of output buffer.  In each iteration of the loop,\nwhen the function returns, status should read Z_OK and the function\nshould have consumed all input.\n\nBut the version that uses consume() function does not give\nsufficient output buffer to ensure that the input will always be\ninflated fully (avoiding to use large output buffer is the whole\npoint of your patch after all), so with your patch, that no longer\nholds true.\n"},{"id":"194598","messageId":"CACsJy8AH6HHxE-P4j4O_VtVzz6-pe2VWk5ZayOfbZomfX2z7sg@mail.gmail.com","threadId":"30946","inReplyTo":"7vy5n0rq9c.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] serious inflate inconsistency on master","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-07-04T07:01:06Z","receivedAt":"2012-07-04T07:01:06Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 4, 2012 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n>\n>> By the way I searched the commit that introduces that check with \"git\n>> log --follow -p builtin/index-pack.c\" but I could not find it. What\n>> did I do wrong?\n>\n> Your commit 8a2e163cc shows changes to the file at ll.535-540; these\n> come from 776ea370 builtin-index-pack.c ll.383-388.\n>\n>   $ git show 776ea370:builtin-index-pack.c\n\ngit newbie's hat's on. How do you find 776ea370, git-blame? Another\nquestion is why doesn't git-log show that commit?\n-- \nDuy\n"},{"id":"194599","messageId":"20120704071214.GB24807@sigill.intra.peff.net","threadId":"30946","inReplyTo":"7vy5n0rq9c.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] serious inflate inconsistency on master","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-04T07:12:14Z","receivedAt":"2012-07-04T07:12:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 03, 2012 at 11:31:43PM -0700, Junio C Hamano wrote:\n\n> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n> \n> > By the way I searched the commit that introduces that check with \"git\n> > log --follow -p builtin/index-pack.c\" but I could not find it. What\n> > did I do wrong?\n> \n> Your commit 8a2e163cc shows changes to the file at ll.535-540; these\n> come from 776ea370 builtin-index-pack.c ll.383-388.\n> \n>   $ git show 776ea370:builtin-index-pack.c\n> \n> The get_data_from_pack() function in that commit gives sufficient\n> buffer to output side (avail_out starts out as obj->size), and feeds\n> the data from the packfile in chunks.  With the arrangement this\n> commit makes to call git_inflate(), it should never get stuck\n> because it ran out of output buffer.  In each iteration of the loop,\n> when the function returns, status should read Z_OK and the function\n> should have consumed all input.\n> \n> But the version that uses consume() function does not give\n> sufficient output buffer to ensure that the input will always be\n> inflated fully (avoiding to use large output buffer is the whole\n> point of your patch after all), so with your patch, that no longer\n> holds true.\n\nYeah, that makes sense. I was wondering if we could get rid of the\navail_in check (and instead just _always_ loop to drain avail_in,\nwhether we are using consume() or not). The intent would be that the\nloop would be a harmless no-op in the non-consume case, because we would\nalways drain the input completely on the first call.\n\nBut that's not right; if we _didn't_ drain it, it's probably because the\ninput is malformed (i.e., the deflated data is larger than the object\nsize claims), and we would loop infinitely (because we are not extending\nthe output buffer during each run of the loop).\n\nSo the patch I posted earlier is the right direction. I didn't properly\ndeal with moving last_out into the inner loop in that patch, but after\nchecking to see where it goes, I'm pretty sure it's superfluous. Here it\nis with last_out removed and a proper commit message:\n\n-- >8 --\nSubject: index-pack: loop while inflating objects in unpack_data\n\nWhen the unpack_data function is given a consume() callback,\nit unpacks only 64K of the input at a time, feeding it to\ngit_inflate along with a 64K output buffer.  However,\nbecause we are inflating, there is a good chance that the\noutput buffer will fill before consuming all of the input.\nIn this case, we need to loop on git_inflate until we have\nfed the whole input buffer, feeding each chunk of output to\nthe consume buffer.\n\nThe current code does not do this, and as a result, will\nfail the loop condition and trigger a fatal \"serious inflate\ninconsistency\" error in this case.\n\nWhile we're rearranging the loop, let's get rid of the\nextra last_out pointer. It is meant to point to the\nbeginning of the buffer that we feed to git_inflate, but in\npractice this is always the beginning of our same 64K\nbuffer, because:\n\n  1. At the beginning of the loop, we are feeding the\n     buffer.\n\n  2. At the end of the loop, if we are using a consume()\n     function, we reset git_inflate's pointer to the\n     beginning of the buffer.  If we are not using a\n     consume() function, then we do not care about the value\n     of last_out at all.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8b5c1eb..50d3876 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -524,7 +524,6 @@ static void *unpack_data(struct object_entry *obj,\n \tstream.avail_out = consume ? 64*1024 : obj->size;\n \n \tdo {\n-\t\tunsigned char *last_out = stream.next_out;\n \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n \t\tn = pread(pack_fd, inbuf, n, from);\n \t\tif (n < 0)\n@@ -538,15 +537,19 @@ static void *unpack_data(struct object_entry *obj,\n \t\tlen -= n;\n \t\tstream.next_in = inbuf;\n \t\tstream.avail_in = n;\n-\t\tstatus = git_inflate(&stream, 0);\n-\t\tif (consume) {\n-\t\t\tif (consume(last_out, stream.next_out - last_out, cb_data)) {\n-\t\t\t\tfree(inbuf);\n-\t\t\t\tfree(data);\n-\t\t\t\treturn NULL;\n-\t\t\t}\n-\t\t\tstream.next_out = data;\n-\t\t\tstream.avail_out = 64*1024;\n+\t\tif (!consume)\n+\t\t\tstatus = git_inflate(&stream, 0);\n+\t\telse {\n+\t\t\tdo {\n+\t\t\t\tstatus = git_inflate(&stream, 0);\n+\t\t\t\tif (consume(data, stream.next_out - data, cb_data)) {\n+\t\t\t\t\tfree(inbuf);\n+\t\t\t\t\tfree(data);\n+\t\t\t\t\treturn NULL;\n+\t\t\t\t}\n+\t\t\t\tstream.next_out = data;\n+\t\t\t\tstream.avail_out = 64*1024;\n+\t\t\t} while (status == Z_OK && stream.avail_in);\n \t\t}\n \t} while (len && status == Z_OK && !stream.avail_in);\n \n-- \n1.7.11.rc1.21.g3c8d91e\n"},{"id":"194603","messageId":"20120704072412.GC24807@sigill.intra.peff.net","threadId":"30946","inReplyTo":"CACsJy8AH6HHxE-P4j4O_VtVzz6-pe2VWk5ZayOfbZomfX2z7sg@mail.gmail.com","subject":"Re: [BUG] serious inflate inconsistency on master","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-04T07:24:12Z","receivedAt":"2012-07-04T07:24:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 04, 2012 at 02:01:06PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> On Wed, Jul 4, 2012 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> > Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n> >\n> >> By the way I searched the commit that introduces that check with \"git\n> >> log --follow -p builtin/index-pack.c\" but I could not find it. What\n> >> did I do wrong?\n> >\n> > Your commit 8a2e163cc shows changes to the file at ll.535-540; these\n> > come from 776ea370 builtin-index-pack.c ll.383-388.\n> >\n> >   $ git show 776ea370:builtin-index-pack.c\n> \n> git newbie's hat's on. How do you find 776ea370, git-blame? Another\n> question is why doesn't git-log show that commit?\n\nI used git-blame to find it. As to your second question, I believe it is\none of the side-effects of the way --follow is bolted onto the revision\ntraversal. Look at:\n\n  gitk -- builtin/index-pack.c builtin-index-pack.c\n\nand you will see that the commit in question happened on a simultaneous\nbranch with the big builtin rename commit. Since we process 776ea370\nbefore we hit the rename commit, we do not yet realize that\nbuiltin-index-pack.c is of interest to us.\n\n-Peff\n"}]}