{"thread":{"id":"45567","subject":"SHA1 collision in production repo?! (probably not)","startedAt":"2017-03-31T16:05:27Z","lastAt":"2017-09-12T17:38:25Z","messageCount":15,"participants":["Lars Schneider","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"315922","messageId":"4D74C1D4-9EA7-4A17-AFC5-0B54B4A6DD0E@gmail.com","threadId":"45567","inReplyTo":null,"subject":"SHA1 collision in production repo?! (probably not)","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-03-31T16:05:17Z","receivedAt":"2017-03-31T16:05:27Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Hi,\n\nI just got a report with the following output after a \"git fetch\" operation\nusing Git 2.11.0.windows.3 [1]:\n\nremote: Counting objects: 5922, done.\nremote: Compressing objects: 100% (14/14), done.\nerror: inflate: data stream error (unknown compression method)\nerror: unable to unpack 6acd8f279a8b20311665f41134579b7380970446 header\nfatal: SHA1 COLLISION FOUND WITH 6acd8f279a8b20311665f41134579b7380970446 !\nfatal: index-pack failed\n\nI would be really surprised if we discovered a SHA1 collision in a production\nrepo. My guess is that this is somehow triggered by a network issue (see data\nstream error). Any tips how to debug this?\n\nThanks,\nLars\n\n[1] Git for Windows build based on Git v2.11.0\n"},{"id":"315931","messageId":"20170331172726.z6w34kticc74bgec@sigill.intra.peff.net","threadId":"45567","inReplyTo":"4D74C1D4-9EA7-4A17-AFC5-0B54B4A6DD0E@gmail.com","subject":"Re: SHA1 collision in production repo?! (probably not)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-31T17:27:27Z","receivedAt":"2017-03-31T17:27:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 31, 2017 at 06:05:17PM +0200, Lars Schneider wrote:\n\n> I just got a report with the following output after a \"git fetch\" operation\n> using Git 2.11.0.windows.3 [1]:\n> \n> remote: Counting objects: 5922, done.\n> remote: Compressing objects: 100% (14/14), done.\n> error: inflate: data stream error (unknown compression method)\n> error: unable to unpack 6acd8f279a8b20311665f41134579b7380970446 header\n> fatal: SHA1 COLLISION FOUND WITH 6acd8f279a8b20311665f41134579b7380970446 !\n> fatal: index-pack failed\n> \n> I would be really surprised if we discovered a SHA1 collision in a production\n> repo. My guess is that this is somehow triggered by a network issue (see data\n> stream error). Any tips how to debug this?\n\nI'd be surprised, too. :)\n\nI'm not sure that inflate error actually comes from the network pack.\nThe \"unable to unpack $sha1 header\" message actually comes from\nsha1_object_info_loose(). Which means we're failing to read our _local_\nversion of 6acd8f279a8b, which is an object we believe is coming in from\nthe network.\n\nAnd that would explain the false-positive collision. We computed the\nsha1 on something come from the network. We believe we have an object\nwith that sha1 already (but it's corrupted), and then when we compared\nthe real bytes to the corrupted bytes, they didn't match.\n\nWe should be able to confirm that by running \"git fsck\" on the local\nrepo, which I'd expect to complain about the loose object.\n\nBut what I find disturbing there is that we did not notice the failure\nwhen accessing the loose object. It seems to have been lost in the call\nchain. I think the problem is that sha1_loose_object_info() may report\nerrors in two ways: returning -1 if it did not find the object, or\nputting OBJ_BAD into the type field if it found a corrupt object. But\ncallers are not aware of the second one.\n\nI think it should probably return -1 for a corruption, too, and act as\nif we don't have the object (because we effectively don't).\n\n-Peff\n"},{"id":"315935","messageId":"xmqqh929z6wl.fsf@gitster.mtv.corp.google.com","threadId":"45567","inReplyTo":"4D74C1D4-9EA7-4A17-AFC5-0B54B4A6DD0E@gmail.com","subject":"Re: SHA1 collision in production repo?! (probably not)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-31T17:35:06Z","receivedAt":"2017-03-31T17:35:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> Hi,\n>\n> I just got a report with the following output after a \"git fetch\" operation\n> using Git 2.11.0.windows.3 [1]:\n>\n> remote: Counting objects: 5922, done.\n> remote: Compressing objects: 100% (14/14), done.\n> error: inflate: data stream error (unknown compression method)\n> error: unable to unpack 6acd8f279a8b20311665f41134579b7380970446 header\n> fatal: SHA1 COLLISION FOUND WITH 6acd8f279a8b20311665f41134579b7380970446 !\n> fatal: index-pack failed\n>\n> I would be really surprised if we discovered a SHA1 collision in a production\n> repo. My guess is that this is somehow triggered by a network issue (see data\n> stream error). Any tips how to debug this?\n\nPerhaps the first thing to do is to tweak the messages in builtin/index-pack.c\nto help you identify which one of identical 5 messages is firing.\n\nMy guess would be that the code saw an object that came over the\nwire, hashed it to learn that its object name is\n6acd8f279a8b20311665f41134579b7380970446, noticed that it locally\nalready has the object with the same name, and tried to make sure\nthey have identical contents (otherwise, what came over the wire is\na successful second preimage attack). But your loose object on disk\nyou already had was corrupt and didn't inflate correctly when\nbuiltin/index-pack.c::compare_objects() or check_collision() tried\nto. The code saw no data, or truncated data, or\nwhatever---something different from what the other data that hashed\ndown to 6acd8..., and reported a collision when there is no\ncollision.\n\n"},{"id":"315936","messageId":"20170331174515.j2ruifuigskyvucc@sigill.intra.peff.net","threadId":"45567","inReplyTo":"xmqqh929z6wl.fsf@gitster.mtv.corp.google.com","subject":"Re: SHA1 collision in production repo?! (probably not)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-31T17:45:15Z","receivedAt":"2017-03-31T17:45:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 31, 2017 at 10:35:06AM -0700, Junio C Hamano wrote:\n\n> Lars Schneider writes:\n> \n> > Hi,\n> >\n> > I just got a report with the following output after a \"git fetch\" operation\n> > using Git 2.11.0.windows.3 [1]:\n> >\n> > remote: Counting objects: 5922, done.\n> > remote: Compressing objects: 100% (14/14), done.\n> > error: inflate: data stream error (unknown compression method)\n> > error: unable to unpack 6acd8f279a8b20311665f41134579b7380970446 header\n> > fatal: SHA1 COLLISION FOUND WITH 6acd8f279a8b20311665f41134579b7380970446 !\n> > fatal: index-pack failed\n> >\n> > I would be really surprised if we discovered a SHA1 collision in a production\n> > repo. My guess is that this is somehow triggered by a network issue (see data\n> > stream error). Any tips how to debug this?\n> \n> Perhaps the first thing to do is to tweak the messages in builtin/index-pack.c\n> to help you identify which one of identical 5 messages is firing.\n> \n> My guess would be that the code saw an object that came over the\n> wire, hashed it to learn that its object name is\n> 6acd8f279a8b20311665f41134579b7380970446, noticed that it locally\n> already has the object with the same name, and tried to make sure\n> they have identical contents (otherwise, what came over the wire is\n> a successful second preimage attack). But your loose object on disk\n> you already had was corrupt and didn't inflate correctly when\n> builtin/index-pack.c::compare_objects() or check_collision() tried\n> to. The code saw no data, or truncated data, or\n> whatever---something different from what the other data that hashed\n> down to 6acd8..., and reported a collision when there is no\n> collision.\n\nMy guess is the one in compare_objects(). The \"unable to unpack\" error\ncomes from sha1_loose_object_info(). We'd normally then follow up with\nread_sha1_file(), which would generate its own set of errors.\n\nBut if open_istream() got a bogus value for the object size (but didn't\ncorrectly report an error), then it would probably quietly return 0\nbytes from read_istream() later.\n\nI suspect this may improve things, but I haven't dug deeper to see if\nthere are unwanted side effects, or if there are other spots that need\nsimilar treatment.\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 43990dec7..38411f90b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2952,7 +2952,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \tif (status && oi->typep)\n \t\t*oi->typep = status;\n \tstrbuf_release(&hdrbuf);\n-\treturn 0;\n+\treturn status;\n }\n \n int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)\n\n-Peff\n"},{"id":"315937","messageId":"20170331174827.zheqstwtlsqtxa6e@sigill.intra.peff.net","threadId":"45567","inReplyTo":"20170331174515.j2ruifuigskyvucc@sigill.intra.peff.net","subject":"Re: SHA1 collision in production repo?! (probably not)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-31T17:48:27Z","receivedAt":"2017-03-31T17:48:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 31, 2017 at 01:45:15PM -0400, Jeff King wrote:\n\n> I suspect this may improve things, but I haven't dug deeper to see if\n> there are unwanted side effects, or if there are other spots that need\n> similar treatment.\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index 43990dec7..38411f90b 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2952,7 +2952,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n> \tif (status && oi->typep)\n> \t\t*oi->typep = status;\n> \tstrbuf_release(&hdrbuf);\n> -\treturn 0;\n> +\treturn status;\n> }\n> \n> int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)\n\nEr, no, that's totally wrong. \"status' may be holding the type. It\nshould really be:\n\n return status < 0 ? status : 0;\n\n-Peff\n"},{"id":"315942","messageId":"xmqq4ly9z4tx.fsf@gitster.mtv.corp.google.com","threadId":"45567","inReplyTo":"20170331174827.zheqstwtlsqtxa6e@sigill.intra.peff.net","subject":"Re: SHA1 collision in production repo?! (probably not)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-31T18:19:54Z","receivedAt":"2017-03-31T18:20:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 31, 2017 at 01:45:15PM -0400, Jeff King wrote:\n>\n>> I suspect this may improve things, but I haven't dug deeper to see if\n>> there are unwanted side effects, or if there are other spots that need\n>> similar treatment.\n>> \n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 43990dec7..38411f90b 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -2952,7 +2952,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n>> \tif (status && oi->typep)\n>> \t\t*oi->typep = status;\n>> \tstrbuf_release(&hdrbuf);\n>> -\treturn 0;\n>> +\treturn status;\n>> }\n>> \n>> int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)\n>\n> Er, no, that's totally wrong. \"status' may be holding the type. It\n> should really be:\n>\n> return status < 0 ? status : 0;\n\nSounds more like it. The only caller will say \"ah, that object is\nnot available to us---let's try packs again\", which is exactly what\nwe want to happen.\n\nThere is another bug in the codepath: the assignment to *oi->typep\nin the pre-context must guard against negative status value. By\nreturning an error correctly like you do above, that bug becomes\nmore or less irrelevant, though.\n\n\n\n"},{"id":"315945","messageId":"20170331184223.4im6sollr3vfahxw@sigill.intra.peff.net","threadId":"45567","inReplyTo":"xmqq4ly9z4tx.fsf@gitster.mtv.corp.google.com","subject":"Re: SHA1 collision in production repo?! (probably not)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-31T18:42:23Z","receivedAt":"2017-03-31T18:42:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 31, 2017 at 11:19:54AM -0700, Junio C Hamano wrote:\n\n> > Er, no, that's totally wrong. \"status' may be holding the type. It\n> > should really be:\n> >\n> > return status < 0 ? status : 0;\n> \n> Sounds more like it. The only caller will say \"ah, that object is\n> not available to us---let's try packs again\", which is exactly what\n> we want to happen.\n\nRight. Callers cannot distinguish between \"did not have it\" and\n\"corrupted\", but that is no different than the rest of the sha1-file\ninterface.\n\n> There is another bug in the codepath: the assignment to *oi->typep\n> in the pre-context must guard against negative status value. By\n> returning an error correctly like you do above, that bug becomes\n> more or less irrelevant, though.\n\nI think that is intentional. OBJ_BAD is -1 (though the callers are\nassuming that error() == OBJ_BAD). And that type gets relayed through\nsha1_object_info() as the combined type/status return value.\n\n-Peff\n"},{"id":"315954","messageId":"xmqq37dtxi36.fsf@gitster.mtv.corp.google.com","threadId":"45567","inReplyTo":"20170331174515.j2ruifuigskyvucc@sigill.intra.peff.net","subject":"Re: SHA1 collision in production repo?! (probably not)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-31T21:16:29Z","receivedAt":"2017-03-31T21:16:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Before we forget...\n\n\n-- >8 --\nFrom: Jeff King \n\nWhen sha1_loose_object_info() finds that a loose object file cannot\nbe stat(2)ed or mmap(2)ed, it returns -1 to signal an error to the\ncaller. However, if it found that the loose object file is corrupt\nand the object data cannot be used from it, it forgets to return -1.\n\nThis can confuse the caller, who may be lead to mistakenly think\nthat there is a loose object and possibly gets an incorrect type and\nsize from the function. The SHA-1 collision detection codepath, for\nexample, when it gets an object over the wire and tries to see the\ndata is the same as what is available as a loose object locally, can\nget confused when the loose object is correupted due to this bug.\n\n---\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 71063890ff..368c89b5c3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2952,7 +2952,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \tif (status && oi->typep)\n \t\t*oi->typep = status;\n \tstrbuf_release(&hdrbuf);\n-\treturn 0;\n+\treturn (status < 0) ? status : 0;\n }\n \n int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)\n"},{"id":"315996","messageId":"20170401080349.lccextuc3l6fgs6j@sigill.intra.peff.net","threadId":"45567","inReplyTo":"xmqq37dtxi36.fsf@gitster.mtv.corp.google.com","subject":"Re: SHA1 collision in production repo?! (probably not)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-01T08:03:49Z","receivedAt":"2017-04-01T08:04:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 31, 2017 at 02:16:29PM -0700, Junio C Hamano wrote:\n\n> Before we forget...\n> \n> -- >8 --\n> From: Jeff King \n> \n> When sha1_loose_object_info() finds that a loose object file cannot\n> be stat(2)ed or mmap(2)ed, it returns -1 to signal an error to the\n> caller. However, if it found that the loose object file is corrupt\n> and the object data cannot be used from it, it forgets to return -1.\n> \n> This can confuse the caller, who may be lead to mistakenly think\n> that there is a loose object and possibly gets an incorrect type and\n> size from the function. The SHA-1 collision detection codepath, for\n> example, when it gets an object over the wire and tries to see the\n> data is the same as what is available as a loose object locally, can\n> get confused when the loose object is correupted due to this bug.\n\nUnfortunately this isn't quite right. I was able to reproduce the\nproblem that Lars saw, and this patch doesn't fix it.\n\nSo here's a two-patch series. The first fixes the problem described\nabove, along with a simpler test that demonstrates it. The second fixes\nLars's problem on top.\n\nI know you're heading out for a week, so I'll post it now for review,\nand then hold and repost when you get back.\n\n [1/2]: sha1_loose_object_info: return error for corrupted objects\n [2/2]: index-pack: detect local corruption in collision check\n\n builtin/index-pack.c | 2 ++\n sha1_file.c | 2 +-\n t/t1060-object-corruption.sh | 24 ++++++++++++++++++++++++\n 3 files changed, 27 insertions(+), 1 deletion(-)\n\n-Peff\n"},{"id":"315997","messageId":"20170401080521.ehjc6jomjd5py3lj@sigill.intra.peff.net","threadId":"45567","inReplyTo":"20170401080349.lccextuc3l6fgs6j@sigill.intra.peff.net","subject":"[PATCH 1/2] sha1_loose_object_info: return error for corrupted objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-01T08:05:21Z","receivedAt":"2017-04-01T08:05:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When sha1_loose_object_info() finds that a loose object file\ncannot be stat(2)ed or mmap(2)ed, it returns -1 to signal an\nerror to the caller. However, if it found that the loose\nobject file is corrupt and the object data cannot be used\nfrom it, it stuffs OBJ_BAD into \"type\" field of the\nobject_info, but returns zero (i.e., success), which can\nconfuse callers.\n\nThis is due to 052fe5eac (sha1_loose_object_info: make type\nlookup optional, 2013-07-12), which switched the return to a\nstrict success/error, rather than returning the type (but\nbotched the return).\n\nCallers of regular sha1_object_info() don't notice the\ndifference, as that function returns the type (which is\nOBJ_BAD in this case). However, direct callers of\nsha1_object_info_extended() see the function return success,\nbut without setting any meaningful values in the object_info\nstruct, leading them to access potentially uninitialized\nmemory.\n\nThe easiest way to see the bug is via \"cat-file -s\", which\nwill happily ignore the corruption and report whatever\nvalue happened to be in the \"size\" variable.\n\nSigned-off-by: Jeff King \n---\nSo not only does it not fail, but running with --valgrind complains.\n\n sha1_file.c | 2 +-\n t/t1060-object-corruption.sh | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 71063890f..368c89b5c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2952,7 +2952,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \tif (status && oi->typep)\n \t\t*oi->typep = status;\n \tstrbuf_release(&hdrbuf);\n-\treturn 0;\n+\treturn (status < 0) ? status : 0;\n }\n \n int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex 3f8705139..3a88d79c5 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -53,6 +53,13 @@ test_expect_success 'streaming a corrupt blob fails' '\n \t)\n '\n \n+test_expect_success 'getting type of a corrupt blob fails' '\n+\t(\n+\t\tcd bit-error &&\n+\t\ttest_must_fail git cat-file -s HEAD:content.t\n+\t)\n+'\n+\n test_expect_success 'read-tree -u detects bit-errors in blobs' '\n \t(\n \t\tcd bit-error &&\n-- \n2.12.2.943.g91cb50fd8\n\n"},{"id":"315998","messageId":"20170401080931.t67jbtta4w4enui6@sigill.intra.peff.net","threadId":"45567","inReplyTo":"20170401080349.lccextuc3l6fgs6j@sigill.intra.peff.net","subject":"[PATCH 2/2] index-pack: detect local corruption in collision check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-01T08:09:32Z","receivedAt":"2017-04-01T08:09:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we notice that we have a local copy of an incoming\nobject, we compare the two objects to make sure we haven't\nfound a collision. Before we get to the actual object\nbytes, though, we compare the type and size from\nsha1_object_info().\n\nIf our local object is corrupted, then the type will be\nOBJ_BAD, which obviously will not match the incoming type,\nand we'll report \"SHA1 COLLISION FOUND\" (with capital\nletters and everything). This is confusing, as the problem\nis not a collision but rather local corruption. We should\nreoprt that instead (just like we do if reading the rest of\nthe object content fails a few lines later).\n\nNote that we _could_ just ignore the error and mark it as a\nnon-collision. That would let you \"git fetch\" to replace a\ncorrupted object. But it's not a very reliable method for\nrepairing a repository. The earlier want/have negotiation\ntries to get the other side to omit objects we already have,\nand it would not realize that we are \"missing\" this\ncorrupted object. So we're better off complaining loudly\nwhen we see corruption, and letting the user take more\ndrastic measures to repair (like making a full clone\nelsewhere and copying the pack into place).\n\nNote that the test sets transfer.unpackLimit in the\nreceiving repository so that we use index-pack (which is\nwhat does the collision check). Normally for such a small\npush we'd use unpack-objects, which would simply try to\nwrite the loose object, and discard the new one when we see\nthat there's already an old one.\n\nSigned-off-by: Jeff King \n---\nI was surprised to see that unpack-objects doesn't do the same collision\ncheck. I think it relies on the loose-object precedence. But that seems\nlike it misses a case: if you have a packed version of an object and\nreceive a small fetch or push, we may unpack a duplicate object to its\nloose format without doing any kind of collision check.\n\nWe may want to tighten that up. In the long run, I'd love to see\nunpack-objects go away in favor of teaching index-pack how to write\nloose objects. The two had very similar code once upon a time, but\nindex-pack has grown a lot of feature and bugfixes over the years.\n\n builtin/index-pack.c | 2 ++\n t/t1060-object-corruption.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 88d205f85..9f17adb37 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -809,6 +809,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\tunsigned long has_size;\n \t\tread_lock();\n \t\thas_type = sha1_object_info(sha1, &has_size);\n+\t\tif (has_type < 0)\n+\t\t\tdie(_(\"cannot read existing object info %s\"), sha1_to_hex(sha1));\n \t\tif (has_type != type || has_size != size)\n \t\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"), sha1_to_hex(sha1));\n \t\thas_data = read_sha1_file(sha1, &has_type, &has_size);\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex 3a88d79c5..ac1f189fd 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -21,6 +21,14 @@ test_expect_success 'setup corrupt repo' '\n \t\tcd bit-error &&\n \t\ttest_commit content &&\n \t\tcorrupt_byte HEAD:content.t 10\n+\t) &&\n+\tgit init no-bit-error &&\n+\t(\n+\t\t# distinct commit from bit-error, but containing a\n+\t\t# non-corrupted version of the same blob\n+\t\tcd no-bit-error &&\n+\t\ttest_tick &&\n+\t\ttest_commit content\n \t)\n '\n \n@@ -108,4 +116,13 @@ test_expect_failure 'clone --local detects misnamed objects' '\n \ttest_must_fail git clone --local misnamed misnamed-checkout\n '\n \n+test_expect_success 'fetch into corrupted repo with index-pack' '\n+\t(\n+\t\tcd bit-error &&\n+\t\ttest_must_fail git -c transfer.unpackLimit=1 \\\n+\t\t\tfetch ../no-bit-error 2>stderr &&\n+\t\ttest_i18ngrep ! -i collision stderr\n+\t)\n+'\n+\n test_done\n-- \n2.12.2.943.g91cb50fd8\n"},{"id":"316013","messageId":"xmqqwpb4uij8.fsf@gitster.mtv.corp.google.com","threadId":"45567","inReplyTo":"20170401080521.ehjc6jomjd5py3lj@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] sha1_loose_object_info: return error for corrupted objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T17:47:23Z","receivedAt":"2017-04-01T17:47:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When sha1_loose_object_info() finds that a loose object file\n> cannot be stat(2)ed or mmap(2)ed, it returns -1 to signal an\n> error to the caller. However, if it found that the loose\n> object file is corrupt and the object data cannot be used\n> from it, it stuffs OBJ_BAD into \"type\" field of the\n> object_info, but returns zero (i.e., success), which can\n> confuse callers.\n\nThanks for a nice analysis and a fix.\n\n"},{"id":"316014","messageId":"xmqqshlsuhql.fsf@gitster.mtv.corp.google.com","threadId":"45567","inReplyTo":"20170401080931.t67jbtta4w4enui6@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] index-pack: detect local corruption in collision check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T18:04:34Z","receivedAt":"2017-04-01T18:04:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> is not a collision but rather local corruption. We should\n> reoprt that instead (just like we do if reading the rest of\n> the object content fails a few lines later).\n\ns/reoprt/report/ (locally amended while queuing).\n\n> We may want to tighten that up. In the long run, I'd love to see\n> unpack-objects go away in favor of teaching index-pack how to write\n> loose objects. The two had very similar code once upon a time, but\n> index-pack has grown a lot of feature and bugfixes over the years.\n\nThis sounds like a nice future to aspire to reach. \n\nBesides having to maintain two separate executables, another\ndownside of the current arrangement is that we have to make the\ndecision between streaming to a single pack and exploding into loose\nobjects too early and base our decision solely on the object count.\nIf we are moving to a single receiver, it is conceivable to switch\nto a scheme based on the size of the incoming pack (e.g. spool the\nfirst N MB and if we run out we write out loose objects, otherwise\ncreate a new pack, and dump the spooled part and stream the rest\ninto it while indexing).\n\nThanks.\n"},{"id":"327895","messageId":"512E7480-E923-4EBF-BA9D-1FEEB99B8BA6@gmail.com","threadId":"45567","inReplyTo":"20170331174515.j2ruifuigskyvucc@sigill.intra.peff.net","subject":"Re: SHA1 collision in production repo?! (probably not)","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-09-12T16:18:32Z","receivedAt":"2017-09-12T16:18:40Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 31 Mar 2017, at 19:45, Jeff King wrote:\n> \n> On Fri, Mar 31, 2017 at 10:35:06AM -0700, Junio C Hamano wrote:\n> \n>> Lars Schneider writes:\n>> \n>>> Hi,\n>>> \n>>> I just got a report with the following output after a \"git fetch\" operation\n>>> using Git 2.11.0.windows.3 [1]:\n>>> \n>>> remote: Counting objects: 5922, done.\n>>> remote: Compressing objects: 100% (14/14), done.\n>>> error: inflate: data stream error (unknown compression method)\n>>> error: unable to unpack 6acd8f279a8b20311665f41134579b7380970446 header\n>>> fatal: SHA1 COLLISION FOUND WITH 6acd8f279a8b20311665f41134579b7380970446 !\n>>> fatal: index-pack failed\n>>> \n>>> I would be really surprised if we discovered a SHA1 collision in a production\n>>> repo. My guess is that this is somehow triggered by a network issue (see data\n>>> stream error). Any tips how to debug this?\n>> \n>> Perhaps the first thing to do is to tweak the messages in builtin/index-pack.c\n>> to help you identify which one of identical 5 messages is firing.\n>> \n>> My guess would be that the code saw an object that came over the\n>> wire, hashed it to learn that its object name is\n>> 6acd8f279a8b20311665f41134579b7380970446, noticed that it locally\n>> already has the object with the same name, and tried to make sure\n>> they have identical contents (otherwise, what came over the wire is\n>> a successful second preimage attack). But your loose object on disk\n>> you already had was corrupt and didn't inflate correctly when\n>> builtin/index-pack.c::compare_objects() or check_collision() tried\n>> to. The code saw no data, or truncated data, or\n>> whatever---something different from what the other data that hashed\n>> down to 6acd8..., and reported a collision when there is no\n>> collision.\n> \n> My guess is the one in compare_objects(). The \"unable to unpack\" error\n> comes from sha1_loose_object_info(). We'd normally then follow up with\n> read_sha1_file(), which would generate its own set of errors.\n> \n> But if open_istream() got a bogus value for the object size (but didn't\n> correctly report an error), then it would probably quietly return 0\n> bytes from read_istream() later.\n> \n> I suspect this may improve things, but I haven't dug deeper to see if\n> there are unwanted side effects, or if there are other spots that need\n> similar treatment.\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index 43990dec7..38411f90b 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2952,7 +2952,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n> \tif (status && oi->typep)\n> \t\t*oi->typep = status;\n> \tstrbuf_release(&hdrbuf);\n> -\treturn 0;\n> +\treturn status;\n> }\n> \n> int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)\n\nHi Peff,\n\nwe are seeing this now in Git 2.14.1:\n\n...\nerror: inflate: data stream error (unknown compression method)\nerror: unable to unpack 7b513f98a66ef9488e516e7abbc246438597c6d5 header\nerror: inflate: data stream error (unknown compression method)\nerror: unable to unpack 7b513f98a66ef9488e516e7abbc246438597c6d5 header\nfatal: loose object 7b513f98a66ef9488e516e7abbc246438597c6d5 (stored in .git/objects/7b/513f98a66ef9488e516e7abbc246438597c6d5) is corrupt\nfatal: The remote end hung up unexpectedly\n\nI guess this means your fix [1] works properly :-)\n\nAt some point I will try to explore a retry mechanism for these cases.\n\nCheers,\nLars\n\n[1] https://github.com/git/git/commit/93cff9a978e1c177ac3e889867004a56773301b2"},{"id":"327902","messageId":"20170912173818.a22a54pvbv7yhjyt@sigill.intra.peff.net","threadId":"45567","inReplyTo":"512E7480-E923-4EBF-BA9D-1FEEB99B8BA6@gmail.com","subject":"Re: SHA1 collision in production repo?! (probably not)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-12T17:38:18Z","receivedAt":"2017-09-12T17:38:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 12, 2017 at 06:18:32PM +0200, Lars Schneider wrote:\n\n> we are seeing this now in Git 2.14.1:\n> \n> ...\n> error: inflate: data stream error (unknown compression method)\n> error: unable to unpack 7b513f98a66ef9488e516e7abbc246438597c6d5 header\n> error: inflate: data stream error (unknown compression method)\n> error: unable to unpack 7b513f98a66ef9488e516e7abbc246438597c6d5 header\n> fatal: loose object 7b513f98a66ef9488e516e7abbc246438597c6d5 (stored in .git/objects/7b/513f98a66ef9488e516e7abbc246438597c6d5) is corrupt\n> fatal: The remote end hung up unexpectedly\n> \n> I guess this means your fix [1] works properly :-)\n\nOh, good. :)\n\n> At some point I will try to explore a retry mechanism for these cases.\n\nI don't think we can generally retry loose-object failures. We use\ncopies from packs first, and then loose. So a corrupt loose can fallback\nto another pack or to loose, but not the other way around (because we\nwould never look at the loose if we had a good copy elsewhere).\n\nThough in your particular case, if I recall, you're receiving the object\nover the network and the corrupted copy is in the way. So right now the\nrecovery process is:\n\n 1. Notice the commit message.\n\n 2. Run git-fsck to notice that we don't really[1] need the object.\n\n 3. Run `rm .git/objects/7b/513f...`\n\n 4. Re-run the fetch.\n\nBut in theory we should be able to say \"oh, we don't _really_ have that,\nthe collision test isn't necessary\" and then overwrite it. I actually\nthought that's what would happen now (has_sha1_file() would return an\nerror), but I guess for what we need, it just does a stat() and calls it\na day, not realizing we ought to be overwriting.\n\n-Peff\n\n[1] git-fsck will actually complain if reflogs point to the object, and\n we can always expire those in a corrupted repo. So possibly what you\n want to know is whether it's reachable from actual refs. Of course\n this whole check is optional. If the object's corrupted, it's\n corrupted. But I get nervous calling `rm` on something that _could_\n be precious (say it's just a single-bit error that could be\n recovered). But if you have a known-good copy incoming, that's less\n of an issue.\n"}]}