{"thread":{"id":"5409","subject":"Re: Problem with pack","startedAt":"2006-08-27T17:45:09Z","lastAt":"2006-08-27T21:55:38Z","messageCount":4,"participants":["Sergio Callegari","Linus Torvalds","Nicolas Pitre","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"25993","messageId":"44F1DA25.3050403@arces.unibo.it","threadId":"5409","inReplyTo":null,"subject":"Re: Problem with pack","fromName":"Sergio Callegari","fromEmail":"scallegari@arces.unibo.it","sentAt":"2006-08-27T17:45:09Z","receivedAt":"2006-08-27T17:45:09Z","isPatch":false,"sender":{"key":"scallegari@arces.unibo.it","avatar":null},"body":">\n> I do think that your synchronization using unison is _somehow_ part of the \n> reason why bad things happened, but I really can't see why it would cause \n> problems, and perhaps more importantly, git should have noticed them \n> earlier (and, in particular, failed the repack). So a git bug and/or \n> misfeature is involved somehow.\n>   \nGlad if my broken pack can help finding out!\n> One thing that may have happened is that the use of unison somehow \n> corrupted an older pack (or you had a disk corruption), and that it was \n> missed because the corruption was in a delta of the old pack that was \n> silently re-used for the new one.\n>\n> That would explain how the SHA1 of the pack-file matches - the repack \n> would have re-computed the SHA1 properly, but since the source delta \n> itself was corrupt, the resulting new pack is corrupt.\n>   \nThere is something that I still do not understand... (sorry if I ask \nstupid questions)...\nSince packs have an sha signature too, if there was a data corruption \n(disk or transfer), shouldn't that have been detected at the repack? \nI.e. doesn't repack -d verify the available data before cancelling anything?\n> If you had used git itself to synchronize the two repositories, that \n> corruption of one repo would have been noticed when it transfers the data \n> over to the other side, which is one reason why the native git syncing \n> tools are so superior to doing a filesystem-level synchronization.\n>   \nI think I learnt the lesson!\n> With a filesystem-level sync (unison or anything else - rsync, cp -r, \n> etc), a problem introduced in one repository will be copied to another one \n> without any sanity checking.\n>   \nIdem!\n> but in the meantime, when you find a place to put the corrupt pack/index \n> file, please include me and Junio at a minimum into the group of people \n> who you tell where to find it (and/or passwords to access it). I'll \n> happily keep your data private (I've done it before for others).\n>\n>   \nSure... I have already sent an email to Junio to arrange this.\n\nThanks,\nSergio\n"},{"id":"25995","messageId":"Pine.LNX.4.64.0608271102450.27779@g5.osdl.org","threadId":"5409","inReplyTo":"44F1DA25.3050403@arces.unibo.it","subject":"Re: Problem with pack","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-08-27T18:27:04Z","receivedAt":"2006-08-27T18:27:04Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 27 Aug 2006, Sergio Callegari wrote:\n>\n> There is something that I still do not understand... (sorry if I ask stupid\n> questions)...\n> Since packs have an sha signature too, if there was a data corruption (disk or\n> transfer), shouldn't that have been detected at the repack? I.e. doesn't\n> repack -d verify the available data before cancelling anything?\n\nThe packs do have a SHA1 signature, but verifying it is too expensive for \nnormal operations. It's only verified when you explicitly ask for it, ie \nby git-fsck-objects and git-verify-pack.\n\nNow, for small projects we could easily verify the SHA1 csum when we load \nthe pack, but imagine doing the same thing when the pack is half a \ngigabyte in size, and I think you see the problem.. Especially as most \nnormal operations wouldn't even otherwise touch more than a small small \nfraction of the pack contents, so verifying the SHA1 would be relatively \nvery expensive indeed.\n\nNow, \"git repack -a\" is obviously special in that the \"-a\" will mean that \nwe will generally touch all of the old pack _anyway_, and thus verifying \nthe signature is no longer at all as unreasonable as it is under other \ncircumstances. And very arguably, if you _also_ do \"-d\", then since that \nis a fairly dangerous operation with the potential for real data loss, you \ncould well argue that we should do it.\n\nHowever, since the data was _already_ corrupt in that situation, and since \na \"git-fsck-objects --full\" _will_ pick up the corruption in that case \nboth before and after, equally arguably it's also true that there's really \nnot a huge advantage to checking it in \"git repack -a -d\".\n\nIn other words, in your case, the reason you ended up with the corruption \nspreading was _not_ that \"git repack -a -d\" might have silently not \nnoticed it, but really the fact that unison meant that the corruption \nwould spread from one archive to another in the first place.\n\nNOTE! This is all assuming my theory that a packed entry was broken in the \nfirst place was correct. We obviously still don't _know_ what the problem \nwas. So far it's just a theory.\n\nFinal note: a \"git repack -a -d\" normally actually _does_ do almost as \nmuch checking as a \"git-fsck-objects\". It's literally just the \"copy the \nalready packed object from an old pack to a new one\" that it an \noptimization that short-circuits all the normal git sanity checks. All the \nother cases will effectively do a lot of integrity checking just by virtue \nof unpacking the data in the object that is packed, before re-packing it.\n\nSo it might well be the case that we should simply add an extra integrity \ncheck to the raw data copy in builtin-pack-objects.c: write_object().\n\nNow, these days there is actually two cases of that: the pack-to-pack copy \n(which has existed for a long while) and the new \"!legacy_loose_object()\" \ncase. They should perhaps both verify the integrity of what they copy.\n\nJunio, comments?\n\n\t\tLinus\n"},{"id":"25996","messageId":"Pine.LNX.4.64.0608271513260.3683@localhost.localdomain","threadId":"5409","inReplyTo":"Pine.LNX.4.64.0608271102450.27779@g5.osdl.org","subject":"Re: Problem with pack","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-08-27T19:26:55Z","receivedAt":"2006-08-27T19:26:55Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 27 Aug 2006, Linus Torvalds wrote:\n\n> Now, \"git repack -a\" is obviously special in that the \"-a\" will mean that \n> we will generally touch all of the old pack _anyway_, and thus verifying \n> the signature is no longer at all as unreasonable as it is under other \n> circumstances. And very arguably, if you _also_ do \"-d\", then since that \n> is a fairly dangerous operation with the potential for real data loss, you \n> could well argue that we should do it.\n\nWe definitely should do it with -d.\n\n> However, since the data was _already_ corrupt in that situation, and since \n> a \"git-fsck-objects --full\" _will_ pick up the corruption in that case \n> both before and after, equally arguably it's also true that there's really \n> not a huge advantage to checking it in \"git repack -a -d\".\n\nThere really is an advantage.  Given that the absence of -d leaves old \nobjects/packs around, there is a greater chance for still finding an \nearly copy of the bad object.\n\n> In other words, in your case, the reason you ended up with the corruption \n> spreading was _not_ that \"git repack -a -d\" might have silently not \n> noticed it, but really the fact that unison meant that the corruption \n> would spread from one archive to another in the first place.\n\nBut -d would definitely delete old packs that could have had a non \ncorrupted copy of the desired object.\n\n> Final note: a \"git repack -a -d\" normally actually _does_ do almost as \n> much checking as a \"git-fsck-objects\". It's literally just the \"copy the \n> already packed object from an old pack to a new one\" that it an \n> optimization that short-circuits all the normal git sanity checks. All the \n> other cases will effectively do a lot of integrity checking just by virtue \n> of unpacking the data in the object that is packed, before re-packing it.\n> \n> So it might well be the case that we should simply add an extra integrity \n> check to the raw data copy in builtin-pack-objects.c: write_object().\n> \n> Now, these days there is actually two cases of that: the pack-to-pack copy \n> (which has existed for a long while) and the new \"!legacy_loose_object()\" \n> case. They should perhaps both verify the integrity of what they copy.\n\nI think that git-pack-object should grow another flag: --verify-src or \nsomething.  That flag would force the verification of the pack checksum \nfor any pack used for the repack operation.  Then it should be used \nanytime -d is provided to git-repack.  When -d is not provided then we \ncan skip that --verify-src security measure since nothing gets deleted \nand therefore no risk of loosing a good object over a corrupted one \nwould happen.\n\n\nNicolas\n"},{"id":"26008","messageId":"7vpselx2qt.fsf@assigned-by-dhcp.cox.net","threadId":"5409","inReplyTo":"Pine.LNX.4.64.0608271102450.27779@g5.osdl.org","subject":"Re: Problem with pack","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-27T21:55:38Z","receivedAt":"2006-08-27T21:55:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> NOTE! This is all assuming my theory that a packed entry was broken in the \n> first place was correct. We obviously still don't _know_ what the problem \n> was. So far it's just a theory.\n\nBut it is a good theory.  More plausible than alpha particle\nhitting the output buffer of zlib at the right moment, although\nthe effects are the same ;-).\n\n> So it might well be the case that we should simply add an extra integrity \n> check to the raw data copy in builtin-pack-objects.c: write_object().\n\nI would agree that it is a sensible thing to do to insert check\nat places shown in the attached.  The revalidate_pack_piece()\nwould:\n\n - decode object header to make sure it decodes to sensible\n   enum object_type value from the start of the buffer given as\n   its first argument;\n\n - if it is of type OBJ_DELTA, skip 20-byte base object name;\n\n - the second argument is the length of the piece -- make sure\n   the above steps did not require more than the length;\n\n - make sure the remainder is sane by running inflate() into\n   void.  When fed the remainder in full, inflate() should\n   return Z_OK.\n\nFor the first step we need to refactor unpack_object_header() in\nsha1_file.c a tiny bit to reuse it.\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 46f524d..0521cad 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -276,6 +282,7 @@ static unsigned long write_object(struct\n \t\tmap = map_sha1_file(entry->sha1, &mapsize);\n \t\tif (map && !legacy_loose_object(map)) {\n \t\t\t/* We can copy straight into the pack file */\n+\t\t\trevalidate_pack_piece(map, mapsize);\n \t\t\tsha1write(f, map, mapsize);\n \t\t\tmunmap(map, mapsize);\n \t\t\twritten++;\n@@ -319,6 +326,7 @@ static unsigned long write_object(struct\n \n \t\tdatalen = find_packed_object_size(p, entry->in_pack_offset);\n \t\tbuf = (char *) p->pack_base + entry->in_pack_offset;\n+\t\trevalidate_pack_piece(buf, datalen);\n \t\tsha1write(f, buf, datalen);\n \t\tunuse_packed_git(p);\n \t\thdrlen = 0; /* not really */\n"}]}