{"thread":{"id":"21235","subject":"git hang with corrupted .pack","startedAt":"2009-10-14T04:22:49Z","lastAt":"2009-11-03T22:34:37Z","messageCount":23,"participants":["Andy Isaacson","Shawn O. Pearce","Nicolas Pitre","Junio C Hamano","Alex Riesen","Sverre Rabbelier","Pascal Obry"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"124922","messageId":"20091014042249.GA5250@hexapodia.org","threadId":"21235","inReplyTo":null,"subject":"git hang with corrupted .pack","fromName":"Andy Isaacson","fromEmail":"adi@hexapodia.org","sentAt":"2009-10-14T04:22:49Z","receivedAt":"2009-10-14T04:22:49Z","isPatch":false,"sender":{"key":"adi@hexapodia.org","avatar":null},"body":"I have a clone of torvalds/linux-2.6.git that got corrupted; many git\ncommands hang with a backtrace like\n\n% git cat-file -p 60fa3f769f7651a60125a0f44e3ffe3246d7cf39\n...\n\n#0 use_pack (p=0x1dcf310, w_cursor=0x7fffb4c0e430, offset=43544957, \n left=0x7fffb4c0e348) at sha1_file.c:792\n#1 0x00000000004990ed in unpack_compressed_entry (p=0x1dcf310, \n w_curs=0x7fffb4c0e430, curpos=43544957, size=728) at sha1_file.c:1594\n#2 0x000000000049b089 in unpack_entry (p=0x1dcf310, obj_offset=43544534, \n type=0x7fffb4c0e7b4, sizep=0x7fffb4c0e7a8) at sha1_file.c:1821\n#3 0x000000000049b5f2 in read_packed_sha1 (\n sha1=0x7fffb4c0e780 \"`xxxxxxxxxxxxxxxxxxxxxxxxxxx\", \n type=0x7fffb4c0e7b4, size=0x7fffb4c0e7a8) at sha1_file.c:2054\n#4 0x000000000049b6d6 in read_object (\n sha1=0x7fffb4c0e780 \"`xxxxxxxxxxxxxxxxxxxxxxxxxxx\", \n type=0x7fffb4c0e7b4, size=0x7fffb4c0e7a8) at sha1_file.c:2142\n#5 0x000000000049b765 in read_sha1_file (sha1=0x1dcf310 \"\", \n type=0x7fffb4c0e430, size=0x0) at sha1_file.c:2158\n#6 0x00000000004128da in cmd_cat_file (argc=, \n argv=, prefix=)\n at builtin-cat-file.c:125\n#7 0x0000000000404293 in handle_internal_command (argc=3, argv=0x7fffb4c0ea30)\n at git.c:246\n#8 0x0000000000404454 in main (argc=3, argv=0x7fffb4c0ea30) at git.c:438\n\n(I overwrote the UTF8 that gdb helpfully spewed with 'x'.)\n\nWe're looping in unpack_compressed_entry, adding a fprintf immediately\nafter the call to git_inflate() shows:\n\ngit_inflate returned -5 next_in=0x7f5e602bc17d curpos=43544536 avail_in=293462652 avail_out=0\ngit_inflate returned -5 next_in=0x7f5e602bc17d curpos=43544957 avail_in=293462652 avail_out=0\ngit_inflate returned -5 next_in=0x7f5e602bc17d curpos=43544957 avail_in=293462652 avail_out=0\ngit_inflate returned -5 next_in=0x7f5e602bc17d curpos=43544957 avail_in=293462652 avail_out=0\ngit_inflate returned -5 next_in=0x7f5e602bc17d curpos=43544957 avail_in=293462652 avail_out=0\n\nThe pack is corrupt:\n\n% hd -s $((0x2986fd0)) .git/objects/pack/pack-9836f9e49a483ad95e7de546d775a4f247e6ac74.pack\n02986fd0 f6 17 44 74 4e d5 98 2d 78 9c 95 51 5d 8b db 30 |..DtN..-x..Q]..0|\n02986fe0 10 7c d7 af d8 1f d0 18 cb df 0e a1 04 72 77 10 |.|...........rw.|\n02986ff0 b8 f4 a0 97 f7 20 4b ab 58 77 8a 64 24 f9 92 fc |..... K.Xw.d$...|\n02987000 fb ca 4e 08 a5 b4 0f 7d d3 ce 8e 66 76 77 82 43 |..N....}...fvw.C|\n02987010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|\n02987020 00 00 00 00 00 00 00 00 0f 00 00 00 00 00 00 00 |................|\n02987030 0f 00 00 00 33 a4 6d db 94 35 4d 5b 51 74 59 d9 |....3.m..5M[QtY.|\n02987040 b5 b4 ca 51 ca a2 2a aa 28 83 88 b9 20 6c 0c bd |...Q..*.(... l..|\n02987050 75 b0 77 d6 08 d8 5d 3f 35 76 a3 0f b0 9a 81 e4 |u.w...]?5v......|\n02987060 01 ac 0d 06 36 0c 09 b7 a7 ef 40 69 5d 95 6d 96 |....6.....@i].m.|\n02987070 d3 0c 16 69 91 a6 24 a2 27 15 02 3a 78 55 66 f4 |...i..$.'..:xUf.|\n02987080 b0 b7 ee 8b 69 e1 61 15 ee af f5 d9 5a 71 4d 74 |....i.a.....ZqMt|\n02987090 6c 5f 16 d2 8e 46 b0 a0 ac 49 ac 3b de f4 2a 9a |l_...F...I.;..*.|\n029870a0 15 69 13 f5 ea a8 47 7e bc bc 2f e1 45 5d 20 9c |.i....G~../.E] .|\n029870b0 2d 74 e3 d1 83 32 10 7a 84 b7 c3 d3 f6 e7 f3 66 |-t...2.z.......f|\n\nand that corruption is almost certainly a kernel bug or hardware\nfailure, but git shouldn't lock up.\n\nIlari on #git suggested the following, which does resolve the hangs in\nmy case.\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4ea0b18..838df82 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1594,6 +1594,8 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n+\t\tif (stream.next_in == in)\n+\t\t\tbreak;\n \t\tcurpos += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n \tgit_inflate_end(&stream);\n\nIf anyone has a suggestion for how 36 bytes of my .pack got overwritten,\nI'm interested in that too; it's a Core 2 Duo (Thinkpad x200) running\nUbuntu Karmic beta with 2.6.31-13 (upgraded from -10 it looks like),\nIntel GMA graphics, CONFIG_DMAR=n, ext4 on the internal HDD, which now\nthat I look at it actually does have a fair number of non-zero SMART\ncounters:\n\nDevice Model: FUJITSU MHZ2160BH G1\nSerial Number: K60WT8C2HHRS\nFirmware Version: 0084000A\nUser Capacity: 160,041,885,696 bytes\n...\nID# ATTRIBUTE_NAME FLAG VALUE WORST THRESH TYPE UPDATED WHEN_FAILED RAW_VALUE\n 1 Raw_Read_Error_Rate 0x000f 100 100 046 Pre-fail Always - 219593\n 2 Throughput_Performance 0x0005 100 100 030 Pre-fail Offline - 27721728\n 3 Spin_Up_Time 0x0003 100 100 025 Pre-fail Always - 0\n 4 Start_Stop_Count 0x0032 099 099 000 Old_age Always - 406\n 5 Reallocated_Sector_Ct 0x0033 100 100 024 Pre-fail Always - 8589934592000\n 7 Seek_Error_Rate 0x000f 100 100 047 Pre-fail Always - 112\n 8 Seek_Time_Performance 0x0005 100 100 019 Pre-fail Offline - 0\n 9 Power_On_Hours 0x0032 097 097 000 Old_age Always - 1598\n 10 Spin_Retry_Count 0x0013 100 100 020 Pre-fail Always - 0\n 12 Power_Cycle_Count 0x0032 100 100 000 Old_age Always - 284\n192 Power-Off_Retract_Count 0x0032 100 100 000 Old_age Always - 78\n193 Load_Cycle_Count 0x0032 100 100 000 Old_age Always - 1216\n194 Temperature_Celsius 0x0022 100 100 000 Old_age Always - 38 (Lifetime Min/Max 21/46)\n195 Hardware_ECC_Recovered 0x001a 100 100 000 Old_age Always - 247\n196 Reallocated_Event_Count 0x0032 100 100 000 Old_age Always - 457965568\n197 Current_Pending_Sector 0x0012 100 100 000 Old_age Always - 0\n198 Offline_Uncorrectable 0x0010 100 100 000 Old_age Offline - 0\n199 UDMA_CRC_Error_Count 0x003e 200 253 000 Old_age Always - 0\n200 Multi_Zone_Error_Rate 0x000f 100 100 060 Pre-fail Always - 10448\n203 Run_Out_Cancel 0x0002 100 100 000 Old_age Always - 1529011503750\n240 Head_Flying_Hours 0x003e 200 200 000 Old_age Always - 0\n\n-andy\n"},{"id":"124977","messageId":"20091014142351.GI9261@spearce.org","threadId":"21235","inReplyTo":"20091014042249.GA5250@hexapodia.org","subject":"Re: git hang with corrupted .pack","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-10-14T14:23:51Z","receivedAt":"2009-10-14T14:23:51Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Andy Isaacson wrote:\n> We're looping in unpack_compressed_entry, adding a fprintf immediately\n> after the call to git_inflate() shows:\n\nThanks, that was really quite helpful. Junio/Nico, I think we can\njust apply this patch to maint and include it in the next release:\n\n--8<--\n[PATCH] sha1_file: Fix infinite loop when pack is corrupted\n\nSome types of corruption to a pack may confuse the deflate stream\nwhich stores an object. In Andy's reported case a 36 byte region\nof the pack was overwritten, leading to what appeared to be a valid\ndeflate stream that was trying to produce a result larger than our\nallocated output buffer could accept.\n\nZ_BUF_ERROR is returned from inflate() if either the input buffer\nneeds more input bytes, or the output buffer has run out of space.\nPreviously we only considered the former case, as it meant we needed\nto move the stream's input buffer to the next window in the pack.\n\nWe now abort the loop if inflate() returns Z_BUF_ERROR without\nconsuming the entire input buffer it was given, or has filled\nthe entire output buffer but has not yet returned Z_STREAM_END.\nEither state is a clear indicator that this loop is not working\nas expected, and should not continue.\n\nThis problem cannot occur with loose objects as we open the entire\nloose object as a single buffer and treat Z_BUF_ERROR as an error.\n\nReported-by: Andy Isaacson \nSigned-off-by: Shawn O. Pearce \n---\n sha1_file.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4ea0b18..4cc8939 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1357,6 +1357,8 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n+\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n+\t\t\tbreak;\n \t\tcurpos += stream.next_in - in;\n \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n \t\t stream.total_out < sizeof(delta_head));\n@@ -1594,6 +1596,8 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n+\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n+\t\t\tbreak;\n \t\tcurpos += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n \tgit_inflate_end(&stream);\n-- \n1.6.5.52.g0ff2e\n\n-- \nShawn.\n"},{"id":"124986","messageId":"alpine.LFD.2.00.0910141208170.20122@xanadu.home","threadId":"21235","inReplyTo":"20091014142351.GI9261@spearce.org","subject":"Re: git hang with corrupted .pack","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2009-10-14T16:09:59Z","receivedAt":"2009-10-14T16:09:59Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 14 Oct 2009, Shawn O. Pearce wrote:\n\n> Andy Isaacson wrote:\n> > We're looping in unpack_compressed_entry, adding a fprintf immediately\n> > after the call to git_inflate() shows:\n> \n> Thanks, that was really quite helpful. Junio/Nico, I think we can\n> just apply this patch to maint and include it in the next release:\n> \n> --8<--\n> [PATCH] sha1_file: Fix infinite loop when pack is corrupted\n> \n> Some types of corruption to a pack may confuse the deflate stream\n> which stores an object. In Andy's reported case a 36 byte region\n> of the pack was overwritten, leading to what appeared to be a valid\n> deflate stream that was trying to produce a result larger than our\n> allocated output buffer could accept.\n> \n> Z_BUF_ERROR is returned from inflate() if either the input buffer\n> needs more input bytes, or the output buffer has run out of space.\n> Previously we only considered the former case, as it meant we needed\n> to move the stream's input buffer to the next window in the pack.\n> \n> We now abort the loop if inflate() returns Z_BUF_ERROR without\n> consuming the entire input buffer it was given, or has filled\n> the entire output buffer but has not yet returned Z_STREAM_END.\n> Either state is a clear indicator that this loop is not working\n> as expected, and should not continue.\n> \n> This problem cannot occur with loose objects as we open the entire\n> loose object as a single buffer and treat Z_BUF_ERROR as an error.\n> \n> Reported-by: Andy Isaacson \n> Signed-off-by: Shawn O. Pearce \n\nAcked-by: Nicolas Pitre \n\nThis is unfortunate that making a test case for this isn't exactly \ntrivial.\n\n\n> ---\n> sha1_file.c | 4 ++++\n> 1 files changed, 4 insertions(+), 0 deletions(-)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index 4ea0b18..4cc8939 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1357,6 +1357,8 @@ unsigned long get_size_from_delta(struct packed_git *p,\n> \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n> \t\tstream.next_in = in;\n> \t\tst = git_inflate(&stream, Z_FINISH);\n> +\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n> +\t\t\tbreak;\n> \t\tcurpos += stream.next_in - in;\n> \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n> \t\t stream.total_out < sizeof(delta_head));\n> @@ -1594,6 +1596,8 @@ static void *unpack_compressed_entry(struct packed_git *p,\n> \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n> \t\tstream.next_in = in;\n> \t\tst = git_inflate(&stream, Z_FINISH);\n> +\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n> +\t\t\tbreak;\n> \t\tcurpos += stream.next_in - in;\n> \t} while (st == Z_OK || st == Z_BUF_ERROR);\n> \tgit_inflate_end(&stream);\n> -- \n> 1.6.5.52.g0ff2e\n> \n> -- \n> Shawn.\n> \n"},{"id":"124987","messageId":"20091014161259.GK9261@spearce.org","threadId":"21235","inReplyTo":"alpine.LFD.2.00.0910141208170.20122@xanadu.home","subject":"Re: git hang with corrupted .pack","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-10-14T16:12:59Z","receivedAt":"2009-10-14T16:12:59Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> > Some types of corruption to a pack may confuse the deflate stream\n> > which stores an object. In Andy's reported case a 36 byte region\n> > of the pack was overwritten, leading to what appeared to be a valid\n> > deflate stream that was trying to produce a result larger than our\n> > allocated output buffer could accept.\n...\n> This is unfortunate that making a test case for this isn't exactly \n> trivial.\n\nHmmm. We could do something like manually create a pack file of\none non-delta blob whose pack header length is 16, but use a zlib\nstream whose result body is 64. Prior to this fix, we'd be stuck\nin the infinite loop. :-)\n\nIts a PITA to create though, you have to hand-craft the test vector\nand save it in the repository, we can't produce such a pack with\nany real code we ship.\n\n-- \nShawn.\n"},{"id":"124992","messageId":"alpine.LFD.2.00.0910141234540.20122@xanadu.home","threadId":"21235","inReplyTo":"20091014161259.GK9261@spearce.org","subject":"Re: git hang with corrupted .pack","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2009-10-14T16:42:39Z","receivedAt":"2009-10-14T16:42:39Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 14 Oct 2009, Shawn O. Pearce wrote:\n\n> Nicolas Pitre wrote:\n> > > Some types of corruption to a pack may confuse the deflate stream\n> > > which stores an object. In Andy's reported case a 36 byte region\n> > > of the pack was overwritten, leading to what appeared to be a valid\n> > > deflate stream that was trying to produce a result larger than our\n> > > allocated output buffer could accept.\n> ...\n> > This is unfortunate that making a test case for this isn't exactly \n> > trivial.\n> \n> Hmmm. We could do something like manually create a pack file of\n> one non-delta blob whose pack header length is 16, but use a zlib\n> stream whose result body is 64. Prior to this fix, we'd be stuck\n> in the infinite loop. :-)\n\nAh, of course.\n\n> Its a PITA to create though, you have to hand-craft the test vector\n> and save it in the repository, we can't produce such a pack with\n> any real code we ship.\n\nCan be done easily with dd though, see do_corrupt_object() in t5303 for \nexample.\n\n\nNicolas\n"},{"id":"124994","messageId":"20091014180302.GL9261@spearce.org","threadId":"21235","inReplyTo":"alpine.LFD.2.00.0910141234540.20122@xanadu.home","subject":"Re: git hang with corrupted .pack","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-10-14T18:03:02Z","receivedAt":"2009-10-14T18:03:02Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"> Nicolas Pitre wrote:\n> ...\n> > This is unfortunate that making a test case for this isn't exactly \n> > trivial.\n\nJunio, can you please squash this in?\n\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 5132d41..5f6cd4f 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -275,4 +275,13 @@ test_expect_success \\\n git cat-file blob $blob_2 > /dev/null &&\n git cat-file blob $blob_3 > /dev/null'\n \n+test_expect_success \\\n+ 'corrupting header to have too small output buffer fails unpack' \\\n+ 'create_new_pack &&\n+ git prune-packed &&\n+ printf \"\\262\\001\" | do_corrupt_object $blob_1 0 &&\n+ test_must_fail git cat-file blob $blob_1 > /dev/null &&\n+ test_must_fail git cat-file blob $blob_2 > /dev/null &&\n+ test_must_fail git cat-file blob $blob_3 > /dev/null'\n+\n test_done\n\n-- \nShawn.\n"},{"id":"125011","messageId":"alpine.LFD.2.00.0910141435040.20122@xanadu.home","threadId":"21235","inReplyTo":"20091014180302.GL9261@spearce.org","subject":"Re: git hang with corrupted .pack","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2009-10-14T18:39:23Z","receivedAt":"2009-10-14T18:39:23Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 14 Oct 2009, Shawn O. Pearce wrote:\n\n> Junio, can you please squash this in?\n> \n> diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\n> index 5132d41..5f6cd4f 100755\n> --- a/t/t5303-pack-corruption-resilience.sh\n> +++ b/t/t5303-pack-corruption-resilience.sh\n> @@ -275,4 +275,13 @@ test_expect_success \\\n> git cat-file blob $blob_2 > /dev/null &&\n> git cat-file blob $blob_3 > /dev/null'\n> \n> +test_expect_success \\\n> + 'corrupting header to have too small output buffer fails unpack' \\\n> + 'create_new_pack &&\n> + git prune-packed &&\n> + printf \"\\262\\001\" | do_corrupt_object $blob_1 0 &&\n> + test_must_fail git cat-file blob $blob_1 > /dev/null &&\n> + test_must_fail git cat-file blob $blob_2 > /dev/null &&\n> + test_must_fail git cat-file blob $blob_3 > /dev/null'\n> +\n> test_done\n> \n> -- \n\nI confirm this test without the fix reproduces the infinite loop (and \ndoes stall the test suite).\n\n\nNicolas\n"},{"id":"125063","messageId":"7vbpk985t1.fsf@alter.siamese.dyndns.org","threadId":"21235","inReplyTo":"alpine.LFD.2.00.0910141435040.20122@xanadu.home","subject":"Re: git hang with corrupted .pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-15T07:39:06Z","receivedAt":"2009-10-15T07:39:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> I confirm this test without the fix reproduces the infinite loop (and \n> does stall the test suite).\n\nThanks, both of you.\n"},{"id":"125482","messageId":"81b0412b0910200814v269e91fbkd7841308685e1c54@mail.gmail.com","threadId":"21235","inReplyTo":"7vbpk985t1.fsf@alter.siamese.dyndns.org","subject":"Re: git hang with corrupted .pack","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-10-20T15:14:26Z","receivedAt":"2009-10-20T15:14:26Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Thu, Oct 15, 2009 at 09:39, Junio C Hamano wrote:\n> Nicolas Pitre writes:\n>\n>> I confirm this test without the fix reproduces the infinite loop (and\n>> does stall the test suite).\n>\n> Thanks, both of you.\n\nI seem to have problems with this change (on Cygwin). Sometimes\naccessing an object in a pack fails in unpack_compressed_entry.\nWhen it happens, both avail_in and avail_out of the stream are 0,\nand the reported status is Z_BUF_ERROR.\nOutput with the second attached patch:\n\nerror: *** inflate error: 0x862380 size=1256, avail_in=0 (was 697),\navail_out=0 (was 1256)\nerror: *** unpack_compressed_entry failed\nerror: failed to read object 3296766eb5531ef051ae392114de5d75556f5613\nat offset 2620741 from\n.git/objects/pack/pack-996206790aaefbf4d34c86b3ff546bb924546b7c.pack\nfatal: object 3296766eb5531ef051ae392114de5d75556f5613 is corrupted\n\nI cannot reproduce the problem on a normal system (a 64bit, reasonably modern\nLinux in my case). An attempt to use an upgraded zlib on this cygwin system was\nnot successful, there was an updated library, but a clean recompile didn't\nchange anything.\n\nI worked the case around by allocating a bit more than uncompressed data need.\nIn case someone else also sees the problem, below is how. The size of the\noverallocation is arbitrary.\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4cc8939..66c2519 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1585,11 +1585,11 @@ static void *unpack_compressed_entry(struct\npacked_git *p,\n \tz_stream stream;\n \tunsigned char *buffer, *in;\n\n-\tbuffer = xmalloc(size + 1);\n-\tbuffer[size] = 0;\n+\tbuffer = xmalloc(size + 8);\n+\tmemset(buffer + size, 0, 8);\n \tmemset(&stream, 0, sizeof(stream));\n \tstream.next_out = buffer;\n-\tstream.avail_out = size;\n+\tstream.avail_out = size + 8;\n\n \tgit_inflate_init(&stream);\n \tdo {\n-- \n1.6.5.59.g000dd\n\nThe problematic repo is a little big to post (it's my cygwin-git repo),\nI'll have to find hosting for it first.\n\n\nFrom 643fdf50e4343c3ce3a4b99183dba8d35a10c877 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Tue, 20 Oct 2009 16:42:26 +0200\nSubject: [PATCH 1/2] Workaround inflate sometimes fails to unpack data\n\nWhen it happens, zlib stream's avail_in and avail_out are 0, the returned\nstatus is Z_BUF_ERROR. The values weren't 0 before calling inflate.\n\nerror: *** inflate error: 0x862380 size=1256, avail_in=0 (was 697), avail_out=0 (was 1256)\nerror: *** unpack_compressed_entry failed\nerror: failed to read object 3296766eb5531ef051ae392114de5d75556f5613 at offset 2620741 from .git/objects/pack/pack-996206790aaefbf4d34c86b3ff546bb924546b7c.pack\nfatal: object 3296766eb5531ef051ae392114de5d75556f5613 is corrupted\n---\n sha1_file.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4cc8939..66c2519 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1585,11 +1585,11 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tz_stream stream;\n \tunsigned char *buffer, *in;\n \n-\tbuffer = xmalloc(size + 1);\n-\tbuffer[size] = 0;\n+\tbuffer = xmalloc(size + 8);\n+\tmemset(buffer + size, 0, 8);\n \tmemset(&stream, 0, sizeof(stream));\n \tstream.next_out = buffer;\n-\tstream.avail_out = size;\n+\tstream.avail_out = size + 8;\n \n \tgit_inflate_init(&stream);\n \tdo {\n-- \n1.6.5.59.g000dd\n\n\n\nFrom fc5b47d953f171d7591349acad9a23d3ec683ce9 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Tue, 20 Oct 2009 15:53:33 +0200\nSubject: [PATCH 2/2] Debugging strange failure in zlibs inflate\n\n---\n sha1_file.c | 18 ++++++++++++++++++\n 1 files changed, 18 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 66c2519..1bf240e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1594,10 +1594,24 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tgit_inflate_init(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n+\t\tunsigned prev_ain = stream.avail_in;\n+\t\tunsigned prev_aout = stream.avail_out;\n+\t\tif (!stream.avail_in)\n+\t\t\terror(\"*** avail_in=0, inflate will fail!\");\n+\t\tif (!stream.avail_out)\n+\t\t\terror(\"*** avail_out=0, inflate will fail!\");\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n \t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n+ {\n+\t\t\terror(\"*** inflate error: %p size=%lu, \"\n+\t\t\t \"avail_in=%d (was %u), \"\n+\t\t\t \"avail_out=%d (was %u)\",\n+\t\t\t buffer, size,\n+\t\t\t stream.avail_in, prev_ain,\n+\t\t\t stream.avail_out, prev_aout);\n \t\t\tbreak;\n+ }\n \t\tcurpos += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n \tgit_inflate_end(&stream);\n@@ -1654,6 +1668,8 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,\n \t\tdelta_base_cached -= ent->size;\n \t} else {\n \t\tret = xmemdupz(ent->data, ent->size);\n+\t\tif (!ret)\n+\t\t\tfprintf(stderr, \"*** no memory\\n\");\n \t}\n \t*type = ent->type;\n \t*base_size = ent->size;\n@@ -1815,6 +1831,8 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \tcase OBJ_BLOB:\n \tcase OBJ_TAG:\n \t\tdata = unpack_compressed_entry(p, &w_curs, curpos, *sizep);\n+\t\tif (!data)\n+\t\t\terror(\"*** unpack_compressed_entry failed\");\n \t\tbreak;\n \tdefault:\n \t\tdata = NULL;\n-- \n1.6.5.59.g000dd\n\n"},{"id":"125483","messageId":"fabb9a1e0910200823h280a875p98c61eb5e5e6018a@mail.gmail.com","threadId":"21235","inReplyTo":"81b0412b0910200814v269e91fbkd7841308685e1c54@mail.gmail.com","subject":"Re: git hang with corrupted .pack","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-10-20T15:23:39Z","receivedAt":"2009-10-20T15:23:39Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Oct 20, 2009 at 10:14, Alex Riesen wrote:\n> -       buffer = xmalloc(size + 1);\n> +       buffer = xmalloc(size + 8);\n\n> -       stream.avail_out = size;\n> +       stream.avail_out = size + 8;\n\nThat seems wrong at first glance, you go from '+1' to '+8' on the\nfirst part, and then from '+0' to '+8' in the second part, am I\nmissing something obvious?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"125484","messageId":"81b0412b0910200836q3f519ac3l912cc1c4299047f4@mail.gmail.com","threadId":"21235","inReplyTo":"fabb9a1e0910200823h280a875p98c61eb5e5e6018a@mail.gmail.com","subject":"Re: git hang with corrupted .pack","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-10-20T15:36:28Z","receivedAt":"2009-10-20T15:36:28Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Tue, Oct 20, 2009 at 17:23, Sverre Rabbelier wrote:\n> Heya,\n>\n> On Tue, Oct 20, 2009 at 10:14, Alex Riesen wrote:\n>> -       buffer = xmalloc(size + 1);\n>> +       buffer = xmalloc(size + 8);\n>\n>> -       stream.avail_out = size;\n>> +       stream.avail_out = size + 8;\n>\n> That seems wrong at first glance, you go from '+1' to '+8' on the\n> first part, and then from '+0' to '+8' in the second part, am I\n> missing something obvious?\n\nYes. The \"size\" (the variable) is never changed. It is just the output\nbuffer size, not how many data expected or something like that.\nIOW, I just wanted the buffer to be obviously (to zlib) more than enough.\n"},{"id":"125493","messageId":"7viqeaovmp.fsf@alter.siamese.dyndns.org","threadId":"21235","inReplyTo":"20091014142351.GI9261@spearce.org","subject":"Re: git hang with corrupted .pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-20T16:52:46Z","receivedAt":"2009-10-20T16:52:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Z_BUF_ERROR is returned from inflate() if either the input buffer\n> needs more input bytes, or the output buffer has run out of space.\n> Previously we only considered the former case, as it meant we needed\n> to move the stream's input buffer to the next window in the pack.\n>\n> We now abort the loop if inflate() returns Z_BUF_ERROR without\n> consuming the entire input buffer it was given, or has filled\n> the entire output buffer but has not yet returned Z_STREAM_END.\n> Either state is a clear indicator that this loop is not working\n> as expected, and should not continue.\n\nWhen the inflated contents is of size 0, avail_out would be 0 and avail_in\nwould still have something because the input stream needs to have the end\nof stream marker that is more than zero byte long.\n\nIf that is more than one-byte long, and your avail_in originally fed only\nthe first byte from that sequence, what happens? Wouldn't inflate eat all\nwhat was given (now avail_in is zero), updated its internal state but it\nstill hasn't produced anything (avail_out is zero)?\n\nI am not saying the end-of-stream is more than one-byte long (I didn't\ncheck), but we had a similar bug arising from confusing \"no more output\ndata\" and \"fully consumed input stream\" (e.g. 456cdf6 (Fix loose object\nuncompression check., 2007-03-19).\n\nSomething like that may be what is happening to cause problem Alex is\nseeing.\n\nI think the corrupt packdata detection needs an output buffer at least\none-byte larger than what is needed to store the correct result. Then\nwhen we get BUF_ERROR:\n\n - We never look at avail to see if it is zero; !avail_out is not the same\n as \"it stopped because it ran out of output space\". It might mean\n \"there is nothing more to come but the input stream ended before\n signalling that fact to the inflate engine fully\".\n\n - We do look at avail_out to find how much data we ended up getting. If\n inflate has consumed more buffer than we planned to give it, the stream\n is corrupt (at least it is not what we expected to see);\n\n> \t\tst = git_inflate(&stream, Z_FINISH);\n> +\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n> +\t\t\tbreak;\n> \t\tcurpos += stream.next_in - in;\n> \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n> \t\t stream.total_out < sizeof(delta_head));\n> @@ -1594,6 +1596,8 @@ static void *unpack_compressed_entry(struct packed_git *p,\n> \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n> \t\tstream.next_in = in;\n> \t\tst = git_inflate(&stream, Z_FINISH);\n> +\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n> +\t\t\tbreak;\n> \t\tcurpos += stream.next_in - in;\n> \t} while (st == Z_OK || st == Z_BUF_ERROR);\n> \tgit_inflate_end(&stream);\n> -- \n> 1.6.5.52.g0ff2e\n>\n> -- \n> Shawn.\n"},{"id":"125498","messageId":"7vzl7mng35.fsf@alter.siamese.dyndns.org","threadId":"21235","inReplyTo":"7viqeaovmp.fsf@alter.siamese.dyndns.org","subject":"Re: git hang with corrupted .pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-20T17:13:50Z","receivedAt":"2009-10-20T17:13:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> We now abort the loop if inflate() returns Z_BUF_ERROR without\n>> consuming the entire input buffer it was given, or has filled\n>> the entire output buffer but has not yet returned Z_STREAM_END.\n>> Either state is a clear indicator that this loop is not working\n>> as expected, and should not continue.\n>\n> When the inflated contents is of size 0, avail_out would be 0 and avail_in\n> would still have something because the input stream needs to have the end\n> of stream marker that is more than zero byte long.\n\nAfter thinking about this a bit more, I am reasonably sure that this is\nit. The contents does not have to be a 0-length string, but you would hit\nthis if the pure-data portion of the deflated stream aligns at the end of\nyour (un)pack window and it happens to require another use_pack() to move\nthe window to read the end-of-stream signal. In that situation, the\noutput buffer has already been filled, but you haven't read the input\nstream fully. Would't the new check incorrectly trigger in such a case?\n\n>> \t\tst = git_inflate(&stream, Z_FINISH);\n>> +\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n>> +\t\t\tbreak;\n\nWe won't see this on 64-bit platforms because we use larger (un)pack\nwindow and the condition is much less likely to be met.\n"},{"id":"125511","messageId":"7vpr8hn9ly.fsf@alter.siamese.dyndns.org","threadId":"21235","inReplyTo":"7vzl7mng35.fsf@alter.siamese.dyndns.org","subject":"Re: git hang with corrupted .pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-20T19:33:45Z","receivedAt":"2009-10-20T19:33:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>>> We now abort the loop if inflate() returns Z_BUF_ERROR without\n>>> consuming the entire input buffer it was given, or has filled\n>>> the entire output buffer but has not yet returned Z_STREAM_END.\n>>> Either state is a clear indicator that this loop is not working\n>>> as expected, and should not continue.\n>>\n>> When the inflated contents is of size 0, avail_out would be 0 and avail_in\n>> would still have something because the input stream needs to have the end\n>> of stream marker that is more than zero byte long.\n>\n> After thinking about this a bit more, I am reasonably sure that this is\n> it. The contents does not have to be a 0-length string, but you would hit\n> this if the pure-data portion of the deflated stream aligns at the end of\n> your (un)pack window and it happens to require another use_pack() to move\n> the window to read the end-of-stream signal. In that situation, the\n> output buffer has already been filled, but you haven't read the input\n> stream fully. Would't the new check incorrectly trigger in such a case?\n>\n>>> \t\tst = git_inflate(&stream, Z_FINISH);\n>>> +\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n>>> +\t\t\tbreak;\n>\n> We won't see this on 64-bit platforms because we use larger (un)pack\n> window and the condition is much less likely to be met.\n\nPerhaps it would be as simple as this?\n\nWe deliberately give one byte more than what we expect to see and error\nout when we do get that extra byte filled.\n\n sha1_file.c | 17 +++++++++--------\n 1 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4cc8939..8c9f392 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1344,7 +1344,7 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \t\t\t off_t curpos)\n {\n \tconst unsigned char *data;\n-\tunsigned char delta_head[20], *in;\n+\tunsigned char delta_head[21], *in;\n \tz_stream stream;\n \tint st;\n \n@@ -1357,13 +1357,14 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n-\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n-\t\t\tbreak;\n+\t\tif (!stream.avail_out)\n+\t\t\tbreak; /* the payload is larger than it should be! */\n \t\tcurpos += stream.next_in - in;\n \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n-\t\t stream.total_out < sizeof(delta_head));\n+\t\t stream.total_out < (sizeof(delta_head) - 1));\n \tgit_inflate_end(&stream);\n-\tif ((st != Z_STREAM_END) && stream.total_out != sizeof(delta_head)) {\n+\tif ((st != Z_STREAM_END) &&\n+\t stream.total_out != sizeof(delta_head) - 1) {\n \t\terror(\"delta data unpack-initial failed\");\n \t\treturn 0;\n \t}\n@@ -1589,15 +1590,15 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tbuffer[size] = 0;\n \tmemset(&stream, 0, sizeof(stream));\n \tstream.next_out = buffer;\n-\tstream.avail_out = size;\n+\tstream.avail_out = size + 1;\n \n \tgit_inflate_init(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n-\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n-\t\t\tbreak;\n+\t\tif (!stream.avail_out)\n+\t\t\tbreak; /* the payload is larger than it should be! */\n \t\tcurpos += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n \tgit_inflate_end(&stream);\n"},{"id":"125523","messageId":"alpine.LFD.2.00.0910201538180.21460@xanadu.home","threadId":"21235","inReplyTo":"7vpr8hn9ly.fsf@alter.siamese.dyndns.org","subject":"Re: git hang with corrupted .pack","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2009-10-20T19:46:45Z","receivedAt":"2009-10-20T19:46:45Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 20 Oct 2009, Junio C Hamano wrote:\n\n> Perhaps it would be as simple as this?\n> \n> We deliberately give one byte more than what we expect to see and error\n> out when we do get that extra byte filled.\n> \n> sha1_file.c | 17 +++++++++--------\n> 1 files changed, 9 insertions(+), 8 deletions(-)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index 4cc8939..8c9f392 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1344,7 +1344,7 @@ unsigned long get_size_from_delta(struct packed_git *p,\n> \t\t\t off_t curpos)\n> {\n> \tconst unsigned char *data;\n> -\tunsigned char delta_head[20], *in;\n> +\tunsigned char delta_head[21], *in;\n\nI didn't spend the time needed to think about this issue and your \nproposed fix yet. However I think that using sizeof(delta_head)-1 \nmakes the code a bit confusing. At this point i'd use:\n\n\tint size = sizeof(delta_head) - 1;\n\nand use that variable instead just like it is done in \nunpack_compressed_entry() to have the same code pattern.\n\n\nNicolas\n"},{"id":"125528","messageId":"7vaazln61u.fsf@alter.siamese.dyndns.org","threadId":"21235","inReplyTo":"alpine.LFD.2.00.0910201538180.21460@xanadu.home","subject":"Re: git hang with corrupted .pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-20T20:50:37Z","receivedAt":"2009-10-20T20:50:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> I didn't spend the time needed to think about this issue and your \n> proposed fix yet. However I think that using sizeof(delta_head)-1 \n> makes the code a bit confusing. At this point i'd use:\n>\n> \tint size = sizeof(delta_head) - 1;\n>\n> and use that variable instead just like it is done in \n> unpack_compressed_entry() to have the same code pattern.\n\nSounds good. Here is a reroll with a bit more explanation.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Tue, 20 Oct 2009 12:40:02 -0700\nSubject: [PATCH] Fix \"corrupt input stream\" check while reading from packfiles\n\nAn ealier \"fix\" made us break out of the loop when we get Z_BUF_ERROR back\nfrom inflate(), and either the input stream still had some data to\nconsume, or we have already got the full output we expected.\n\nThis is the same kind of mistake as we corrected with 456cdf6 (Fix loose\nobject uncompression check., 2007-03-19); it is valid for inflate() to\nproduce full output before it consumes the input stream fully; e.g.\nimmediately before reading the end of stream marker.\n\nInstead, detect corrupt input stream by feeding the input as long as\ninflate() wants to without detecting a real error, and giving it an output\nbuffer that is one byte longer than necessary. If it touches the extra\nbyte, we know that the input stream is corrupt; otherwise inflate() will\nnotice the broken input stream by itself.\n\nSigned-off-by: Junio C Hamano \n---\n sha1_file.c | 18 ++++++++++--------\n 1 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4cc8939..f0907b8 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1344,7 +1344,8 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \t\t\t off_t curpos)\n {\n \tconst unsigned char *data;\n-\tunsigned char delta_head[20], *in;\n+\tunsigned char delta_head[21], *in;\n+\tunsigned long expected_size = sizeof(delta_head) - 1;\n \tz_stream stream;\n \tint st;\n \n@@ -1357,13 +1358,14 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n-\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n-\t\t\tbreak;\n+\t\tif (!stream.avail_out)\n+\t\t\tbreak; /* the payload is larger than it should be! */\n \t\tcurpos += stream.next_in - in;\n \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n-\t\t stream.total_out < sizeof(delta_head));\n+\t\t stream.total_out < expected_size);\n \tgit_inflate_end(&stream);\n-\tif ((st != Z_STREAM_END) && stream.total_out != sizeof(delta_head)) {\n+\tif ((st != Z_STREAM_END) &&\n+\t stream.total_out != expected_size) {\n \t\terror(\"delta data unpack-initial failed\");\n \t\treturn 0;\n \t}\n@@ -1589,15 +1591,15 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tbuffer[size] = 0;\n \tmemset(&stream, 0, sizeof(stream));\n \tstream.next_out = buffer;\n-\tstream.avail_out = size;\n+\tstream.avail_out = size + 1;\n \n \tgit_inflate_init(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n-\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n-\t\t\tbreak;\n+\t\tif (!stream.avail_out)\n+\t\t\tbreak; /* the payload is larger than it should be! */\n \t\tcurpos += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n \tgit_inflate_end(&stream);\n-- \n1.6.5.1.107.gba912\n"},{"id":"125673","messageId":"7vd44gm089.fsf@alter.siamese.dyndns.org","threadId":"21235","inReplyTo":"7vaazln61u.fsf@alter.siamese.dyndns.org","subject":"Re: git hang with corrupted .pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-22T06:06:14Z","receivedAt":"2009-10-22T06:06:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nicolas Pitre writes:\n>\n>> I didn't spend the time needed to think about this issue and your \n>> proposed fix yet.\n\nI looked at the issue again, and came to the conclusion that we need quite\ndifferent fix for the two inflate callsites, as they do quite different\nthings.\n\n-- >8 --\nSubject: Fix incorrect error check while reading deflated pack data\n\nThe loop in get_size_from_delta() feeds a deflated delta data from the\npack stream _until_ we get inflated result of 20 bytes[*] or we reach the\nend of stream.\n\n Side note. This magic number 20 does not have anything to do with the\n size of the hash we use, but comes from 1a3b55c (reduce delta head\n inflated size, 2006-10-18).\n\nThe loop reads like this:\n\n do {\n in = use_pack();\n stream.next_in = in;\n st = git_inflate(&stream, Z_FINISH);\n curpos += stream.next_in - in;\n } while ((st == Z_OK || st == Z_BUF_ERROR) &&\n stream.total_out < sizeof(delta_head));\n\nThis git_inflate() can return:\n\n - Z_STREAM_END, if use_pack() fed it enough input and the delta itself\n was smaller than 20 bytes;\n\n - Z_OK, when some progress has been made;\n\n - Z_BUF_ERROR, if no progress is possible, because we either ran out of\n input (due to corrupt pack), or we ran out of output before we saw the\n end of the stream.\n\nThe fix b3118bd (sha1_file: Fix infinite loop when pack is corrupted,\n2009-10-14) attempted was against a corruption that appears to be a valid\nstream that produces a result larger than the output buffer, but we are\nnot even trying to read the stream to the end in this loop. If avail_out\nbecomes zero, total_out will be the same as sizeof(delta_head) so the loop\nwill terminate without the \"fix\". There is no fix from b3118bd needed for\nthis loop, in other words.\n\nThe loop in unpack_compressed_entry() is quite a different story. It\nfeeds a deflated stream (either delta or base) and allows the stream to\nproduce output up to what we expect but no more.\n\n do {\n in = use_pack();\n stream.next_in = in;\n st = git_inflate(&stream, Z_FINISH);\n curpos += stream.next_in - in;\n } while (st == Z_OK || st == Z_BUF_ERROR)\n\nThis _does_ risk falling into an endless interation, as we can exhaust\navail_out if the length we expect is smaller than what the stream wants to\nproduce (due to pack corruption). In such a case, avail_out will become\nzero and inflate() will return Z_BUF_ERROR, while avail_in may (or may\nnot) be zero.\n\nBut this is not a right fix:\n\n do {\n in = use_pack();\n stream.next_in = in;\n st = git_inflate(&stream, Z_FINISH);\n+ if (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out)\n+ break; /* wants more input??? */\n curpos += stream.next_in - in;\n } while (st == Z_OK || st == Z_BUF_ERROR)\n\nas Z_BUF_ERROR from inflate() may be telling us that avail_in has also run\nout before reading the end of stream marker. In such a case, both avail_in\nand avail_out would be zero, and the loop should iterate to allow the end\nof stream marker to be seen by inflate from the input stream.\n\nThe right fix for this loop is likely to be to increment the initial\navail_out by one (we allocate one extra byte to terminate it with NUL\nanyway, so there is no risk to overrun the buffer), and break out if we\nsee that avail_out has become zero, in order to detect that the stream\nwants to produce more than what we expect. After the loop, we have a\ncheck that exactly tests this condition:\n\n if ((st != Z_STREAM_END) || stream.total_out != size) {\n free(buffer);\n return NULL;\n }\n\nSo here is a patch (without my previous botched attempts) to fix this\nissue. The first hunk reverts the corresponding hunk from b3118bd, and\nthe second hunk is the same fix proposed earlier. \n\n---\n sha1_file.c | 8 +++-----\n 1 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4cc8939..63981fb 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1357,8 +1357,6 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n-\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n-\t\t\tbreak;\n \t\tcurpos += stream.next_in - in;\n \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n \t\t stream.total_out < sizeof(delta_head));\n@@ -1589,15 +1587,15 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tbuffer[size] = 0;\n \tmemset(&stream, 0, sizeof(stream));\n \tstream.next_out = buffer;\n-\tstream.avail_out = size;\n+\tstream.avail_out = size + 1;\n \n \tgit_inflate_init(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n-\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n-\t\t\tbreak;\n+\t\tif (!stream.avail_out)\n+\t\t\tbreak; /* the payload is larger than it should be */\n \t\tcurpos += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n \tgit_inflate_end(&stream);\n"},{"id":"125919","messageId":"7vr5sqq3vm.fsf@alter.siamese.dyndns.org","threadId":"21235","inReplyTo":"81b0412b0910200814v269e91fbkd7841308685e1c54@mail.gmail.com","subject":"Re: git hang with corrupted .pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-26T02:35:09Z","receivedAt":"2009-10-26T02:35:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> On Thu, Oct 15, 2009 at 09:39, Junio C Hamano wrote:\n>> Nicolas Pitre writes:\n>>\n>>> I confirm this test without the fix reproduces the infinite loop (and\n>>> does stall the test suite).\n>>\n>> Thanks, both of you.\n>\n> I seem to have problems with this change (on Cygwin). Sometimes\n> accessing an object in a pack fails in unpack_compressed_entry.\n> When it happens, both avail_in and avail_out of the stream are 0,\n> and the reported status is Z_BUF_ERROR.\n\nCould you test the patch in\n\n http://mid.gmane.org/7vd44gm089.fsf@alter.siamese.dyndns.org\n\nwithout your workaround?\n\n-- >8 --\nSubject: Fix incorrect error check while reading deflated pack data\n\nI looked at the issue again, and came to the conclusion that we need quite\ndifferent fix for the two inflate callsites, as they do quite different\nthings.\n\n-- >8 --\nSubject: Fix incorrect error check while reading deflated pack data\n\nThe loop in get_size_from_delta() feeds a deflated delta data from the\npack stream _until_ we get inflated result of 20 bytes[*] or we reach the\nend of stream.\n\n Side note. This magic number 20 does not have anything to do with the\n size of the hash we use, but comes from 1a3b55c (reduce delta head\n inflated size, 2006-10-18).\n\nThe loop reads like this:\n\n do {\n in = use_pack();\n stream.next_in = in;\n st = git_inflate(&stream, Z_FINISH);\n curpos += stream.next_in - in;\n } while ((st == Z_OK || st == Z_BUF_ERROR) &&\n stream.total_out < sizeof(delta_head));\n\nThis git_inflate() can return:\n\n - Z_STREAM_END, if use_pack() fed it enough input and the delta itself\n was smaller than 20 bytes;\n\n - Z_OK, when some progress has been made;\n\n - Z_BUF_ERROR, if no progress is possible, because we either ran out of\n input (due to corrupt pack), or we ran out of output before we saw the\n end of the stream.\n\nThe fix b3118bd (sha1_file: Fix infinite loop when pack is corrupted,\n2009-10-14) attempted was against a corruption that appears to be a valid\nstream that produces a result larger than the output buffer, but we are\nnot even trying to read the stream to the end in this loop. If avail_out\nbecomes zero, total_out will be the same as sizeof(delta_head) so the loop\nwill terminate without the \"fix\". There is no fix from b3118bd needed for\nthis loop, in other words.\n\nThe loop in unpack_compressed_entry() is quite a different story. It\nfeeds a deflated stream (either delta or base) and allows the stream to\nproduce output up to what we expect but no more.\n\n do {\n in = use_pack();\n stream.next_in = in;\n st = git_inflate(&stream, Z_FINISH);\n curpos += stream.next_in - in;\n } while (st == Z_OK || st == Z_BUF_ERROR)\n\nThis _does_ risk falling into an endless interation, as we can exhaust\navail_out if the length we expect is smaller than what the stream wants to\nproduce (due to pack corruption). In such a case, avail_out will become\nzero and inflate() will return Z_BUF_ERROR, while avail_in may (or may\nnot) be zero.\n\nBut this is not a right fix:\n\n do {\n in = use_pack();\n stream.next_in = in;\n st = git_inflate(&stream, Z_FINISH);\n+ if (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out)\n+ break; /* wants more input??? */\n curpos += stream.next_in - in;\n } while (st == Z_OK || st == Z_BUF_ERROR)\n\nas Z_BUF_ERROR from inflate() may be telling us that avail_in has also run\nout before reading the end of stream marker. In such a case, both avail_in\nand avail_out would be zero, and the loop should iterate to allow the end\nof stream marker to be seen by inflate from the input stream.\n\nThe right fix for this loop is likely to be to increment the initial\navail_out by one (we allocate one extra byte to terminate it with NUL\nanyway, so there is no risk to overrun the buffer), and break out if we\nsee that avail_out has become zero, in order to detect that the stream\nwants to produce more than what we expect. After the loop, we have a\ncheck that exactly tests this condition:\n\n if ((st != Z_STREAM_END) || stream.total_out != size) {\n free(buffer);\n return NULL;\n }\n\nSo here is a patch (without my previous botched attempts) to fix this\nissue. The first hunk reverts the corresponding hunk from b3118bd, and\nthe second hunk is the same fix proposed earlier. \n\n---\n sha1_file.c | 8 +++-----\n 1 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4cc8939..63981fb 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1357,8 +1357,6 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n-\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n-\t\t\tbreak;\n \t\tcurpos += stream.next_in - in;\n \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n \t\t stream.total_out < sizeof(delta_head));\n@@ -1589,15 +1587,15 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tbuffer[size] = 0;\n \tmemset(&stream, 0, sizeof(stream));\n \tstream.next_out = buffer;\n-\tstream.avail_out = size;\n+\tstream.avail_out = size + 1;\n \n \tgit_inflate_init(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n-\t\tif (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))\n-\t\t\tbreak;\n+\t\tif (!stream.avail_out)\n+\t\t\tbreak; /* the payload is larger than it should be */\n \t\tcurpos += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n \tgit_inflate_end(&stream);\n"},{"id":"125921","messageId":"81b0412b0910260007i1a9d2a9cr23a80397478fbbf1@mail.gmail.com","threadId":"21235","inReplyTo":"7vr5sqq3vm.fsf@alter.siamese.dyndns.org","subject":"Re: git hang with corrupted .pack","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-10-26T07:07:33Z","receivedAt":"2009-10-26T07:07:33Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Mon, Oct 26, 2009 at 03:35, Junio C Hamano wrote:\n>\n> Could you test the patch in\n>\n>    http://mid.gmane.org/7vd44gm089.fsf@alter.siamese.dyndns.org\n>\n> without your workaround?\n>\n> -- >8 --\n> Subject: Fix incorrect error check while reading deflated pack data\n>\n\nI did. It works as intended since about 3 days. It also helped my home\nserver (a 32bit Linux system), which also experienced the problem.\n\nThanks!\n"},{"id":"125931","messageId":"20091026142351.GZ10505@spearce.org","threadId":"21235","inReplyTo":"7vr5sqq3vm.fsf@alter.siamese.dyndns.org","subject":"Re: git hang with corrupted .pack","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-10-26T14:23:51Z","receivedAt":"2009-10-26T14:23:51Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> Alex Riesen writes:\n> > I seem to have problems with this change (on Cygwin). Sometimes\n> > accessing an object in a pack fails in unpack_compressed_entry.\n> > When it happens, both avail_in and avail_out of the stream are 0,\n> > and the reported status is Z_BUF_ERROR.\n...\n> Subject: Fix incorrect error check while reading deflated pack data\n\nWow. \n \n> The right fix for this loop is likely to be to increment the initial\n> avail_out by one (we allocate one extra byte to terminate it with NUL\n> anyway, so there is no risk to overrun the buffer), and break out if we\n> see that avail_out has become zero, in order to detect that the stream\n> wants to produce more than what we expect. After the loop, we have a\n> check that exactly tests this condition:\n> \n> if ((st != Z_STREAM_END) || stream.total_out != size) {\n> free(buffer);\n> return NULL;\n> }\n> \n> So here is a patch (without my previous botched attempts) to fix this\n> issue. The first hunk reverts the corresponding hunk from b3118bd, and\n> the second hunk is the same fix proposed earlier. \n\nACK. This looks right to me too. I forgot about that end-of-stream\nmarker on the input buffer, and my testing failed to have a stream\nwhere the end-of-stream marker was in the next back window, so this\n\"fix\" wasn't triggering.\n\n*sigh* Thanks for digging into this and fixing it while I was away.\n\n-- \nShawn.\n"},{"id":"126679","messageId":"4AF0A132.1060103@obry.net","threadId":"21235","inReplyTo":"81b0412b0910200814v269e91fbkd7841308685e1c54@mail.gmail.com","subject":"Re: git hang with corrupted .pack","fromName":"Pascal Obry","fromEmail":"pascal@obry.net","sentAt":"2009-11-03T21:31:30Z","receivedAt":"2009-11-03T21:31:30Z","isPatch":false,"sender":{"key":"pascal@obry.net","avatar":"https://avatars.githubusercontent.com/u/467069?v=4"},"body":"Le 20/10/2009 17:14, Alex Riesen a écrit :\n> I seem to have problems with this change (on Cygwin). Sometimes\n> accessing an object in a pack fails in unpack_compressed_entry.\n> When it happens, both avail_in and avail_out of the stream are 0,\n> and the reported status is Z_BUF_ERROR.\n> Output with the second attached patch:\n>\n> error: *** inflate error: 0x862380 size=1256, avail_in=0 (was 697),\n> avail_out=0 (was 1256)\n> error: *** unpack_compressed_entry failed\n> error: failed to read object 3296766eb5531ef051ae392114de5d75556f5613\n> at offset 2620741 from\n> .git/objects/pack/pack-996206790aaefbf4d34c86b3ff546bb924546b7c.pack\n> fatal: object 3296766eb5531ef051ae392114de5d75556f5613 is corrupted\n\nI have this problem on some repository on Cygwin too. For now I have \nreverted to Git 1.6.4 which seems to be working fine.\n\nPascal.\n\n-- \n\n--|------------------------------------------------------\n--| Pascal Obry Team-Ada Member\n--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE\n--|------------------------------------------------------\n--| http://www.obry.net - http://v2p.fr.eu.org\n--| \"The best way to travel is by means of imagination\"\n--|\n--| gpg --keyserver keys.gnupg.net --recv-key F949BD3B\n"},{"id":"126683","messageId":"20091103222834.GC10505@spearce.org","threadId":"21235","inReplyTo":"4AF0A132.1060103@obry.net","subject":"Re: git hang with corrupted .pack","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-11-03T22:28:35Z","receivedAt":"2009-11-03T22:28:35Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Pascal Obry wrote:\n> Le 20/10/2009 17:14, Alex Riesen a ??crit :\n>> I seem to have problems with this change (on Cygwin). Sometimes\n>> accessing an object in a pack fails in unpack_compressed_entry.\n>> When it happens, both avail_in and avail_out of the stream are 0,\n>> and the reported status is Z_BUF_ERROR.\n>> Output with the second attached patch:\n>>\n>> error: *** inflate error: 0x862380 size=1256, avail_in=0 (was 697),\n>> avail_out=0 (was 1256)\n>> error: *** unpack_compressed_entry failed\n>> error: failed to read object 3296766eb5531ef051ae392114de5d75556f5613\n>> at offset 2620741 from\n>> .git/objects/pack/pack-996206790aaefbf4d34c86b3ff546bb924546b7c.pack\n>> fatal: object 3296766eb5531ef051ae392114de5d75556f5613 is corrupted\n>\n> I have this problem on some repository on Cygwin too. For now I have \n> reverted to Git 1.6.4 which seems to be working fine.\n\nIt was fixed in 1.6.5.2 by Junio.\n\n-- \nShawn.\n"},{"id":"126684","messageId":"4AF0AFFD.1050706@obry.net","threadId":"21235","inReplyTo":"20091103222834.GC10505@spearce.org","subject":"Re: git hang with corrupted .pack","fromName":"Pascal Obry","fromEmail":"pascal@obry.net","sentAt":"2009-11-03T22:34:37Z","receivedAt":"2009-11-03T22:34:37Z","isPatch":false,"sender":{"key":"pascal@obry.net","avatar":"https://avatars.githubusercontent.com/u/467069?v=4"},"body":"Shawn,\n\n> It was fixed in 1.6.5.2 by Junio.\n\nHum... I was using today Git from master. Just after I send my message I \ntried 1.6.5 and it was working... I went back to master and it is now \nworking... Really strange... Maybe a file did not get properly \nrecompiled. I doubt it... but for sure it was failing and I build git \nevery day from master! Strange!\n\nAnyway, it was a false alarm.\n\nPascal.\n\n-- \n\n--|------------------------------------------------------\n--| Pascal Obry Team-Ada Member\n--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE\n--|------------------------------------------------------\n--| http://www.obry.net - http://v2p.fr.eu.org\n--| \"The best way to travel is by means of imagination\"\n--|\n--| gpg --keyserver keys.gnupg.net --recv-key F949BD3B\n"}]}