{"thread":{"id":"28957","subject":"[PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice","startedAt":"2011-11-17T06:04:13Z","lastAt":"2011-11-18T23:50:51Z","messageCount":8,"participants":["Junio C Hamano","Shawn Pearce","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"179615","messageId":"7v7h2znv36.fsf@alter.siamese.dyndns.org","threadId":"28957","inReplyTo":null,"subject":"[PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-17T06:04:13Z","receivedAt":"2011-11-17T06:04:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When receive-pack & fetch-pack are run and store the pack obtained over\nthe wire to a local repository, they internally run the index-pack command\nwith the --strict option. Make sure that we reject incoming packfile that\nrecords objects twice to avoid spreading such a damage.\n\nSigned-off-by: Junio C Hamano \n---\n * Passing --strict from fetch-pack actually is a recent invention, so\n this will be only useful to 1.7.8 and later.\n\n builtin/index-pack.c | 4 +++-\n object.c | 2 ++\n pack-write.c | 4 ++++\n pack.h | 3 ++-\n 4 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 0945adb..98025da 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1122,8 +1122,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tif (!index_name)\n \t\t\tdie(\"--verify with no packfile name given\");\n \t\tread_idx_option(&opts, index_name);\n-\t\topts.flags |= WRITE_IDX_VERIFY;\n+\t\topts.flags |= WRITE_IDX_VERIFY | WRITE_IDX_STRICT;\n \t}\n+\tif (strict)\n+\t\topts.flags |= WRITE_IDX_STRICT;\n \n \tcurr_pack = open_pack_file(pack_name);\n \tparse_pack_header();\ndiff --git a/object.c b/object.c\nindex 31976b5..d8d09f9 100644\n--- a/object.c\n+++ b/object.c\n@@ -149,6 +149,8 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\tstruct tree *tree = lookup_tree(sha1);\n \t\tif (tree) {\n \t\t\tobj = &tree->object;\n+\t\t\tif (!tree->buffer)\n+\t\t\t\ttree->object.parsed = 0;\n \t\t\tif (!tree->object.parsed) {\n \t\t\t\tif (parse_tree_buffer(tree, buffer, size))\n \t\t\t\t\treturn NULL;\ndiff --git a/pack-write.c b/pack-write.c\nindex 9cd3bfb..f84adde 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -129,6 +129,10 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t\t}\n \t\tsha1write(f, obj->sha1, 20);\n \t\tgit_SHA1_Update(&ctx, obj->sha1, 20);\n+\t\tif ((opts->flags & WRITE_IDX_STRICT) &&\n+\t\t (i && !hashcmp(list[-2]->sha1, obj->sha1)))\n+\t\t\tdie(\"The same object %s appears twice in the pack\",\n+\t\t\t sha1_to_hex(obj->sha1));\n \t}\n \n \tif (index_version >= 2) {\ndiff --git a/pack.h b/pack.h\nindex 722a54e..aca4739 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -37,7 +37,8 @@ struct pack_header {\n struct pack_idx_option {\n \tunsigned flags;\n \t/* flag bits */\n-#define WRITE_IDX_VERIFY 01\n+#define WRITE_IDX_VERIFY 01 /* verify only, do not write the idx file */\n+#define WRITE_IDX_STRICT 02\n \n \tuint32_t version;\n \tuint32_t off32_limit;\n-- \n1.7.8.rc2.109.g72037\n"},{"id":"179649","messageId":"CAJo=hJtYYZqSowCLrBH02MrQyiuQFmTTxY2dHP9mOQQRqA5Z-Q@mail.gmail.com","threadId":"28957","inReplyTo":"7v7h2znv36.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-11-17T20:51:43Z","receivedAt":"2011-11-17T20:51:43Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Nov 16, 2011 at 22:04, Junio C Hamano wrote:\n> When receive-pack & fetch-pack are run and store the pack obtained over\n> the wire to a local repository, they internally run the index-pack command\n> with the --strict option. Make sure that we reject incoming packfile that\n> records objects twice to avoid spreading such a damage.\n>\n> Signed-off-by: Junio C Hamano \n\nLooks sane to me. I thought this would be more difficult to catch and\nreport to the user. :-)\n"},{"id":"179687","messageId":"20111118103355.GA4854@sigill.intra.peff.net","threadId":"28957","inReplyTo":"7v7h2znv36.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-18T10:33:55Z","receivedAt":"2011-11-18T10:33:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 16, 2011 at 10:04:13PM -0800, Junio C Hamano wrote:\n\n> When receive-pack & fetch-pack are run and store the pack obtained over\n> the wire to a local repository, they internally run the index-pack command\n> with the --strict option. Make sure that we reject incoming packfile that\n> records objects twice to avoid spreading such a damage.\n\nIf we are fixing a thin pack (which should be the case most of the\ntime), we are rewriting the packfile anyway. Shouldn't we just omit\nthe duplicate?\n\nI guess I'm a little confused about what is generating these duplicates.\nA buggy git? A malicious server? Bad luck?\n\n-Peff\n"},{"id":"179712","messageId":"7vd3cpl1cw.fsf@alter.siamese.dyndns.org","threadId":"28957","inReplyTo":"20111118103355.GA4854@sigill.intra.peff.net","subject":"Re: [PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-18T18:41:35Z","receivedAt":"2011-11-18T18:41:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If we are fixing a thin pack (which should be the case most of the\n> time), we are rewriting the packfile anyway. Shouldn't we just omit\n> the duplicate?\n\nExcising unwanted objects from the middle of an existing packfile would\nmean you would need an equivalent of memmove() in the file, which amounts\nto really rewriting the packfile, but the thing is, we are _not_ rewriting\nin that sense in \"index-pack --fix-thin\"; it only appends and adjust the\nfixed-size header.\n"},{"id":"179713","messageId":"20111118184455.GA13782@sigill.intra.peff.net","threadId":"28957","inReplyTo":"7vd3cpl1cw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-18T18:44:55Z","receivedAt":"2011-11-18T18:44:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 18, 2011 at 10:41:35AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > If we are fixing a thin pack (which should be the case most of the\n> > time), we are rewriting the packfile anyway. Shouldn't we just omit\n> > the duplicate?\n> \n> Excising unwanted objects from the middle of an existing packfile would\n> mean you would need an equivalent of memmove() in the file, which amounts\n> to really rewriting the packfile, but the thing is, we are _not_ rewriting\n> in that sense in \"index-pack --fix-thin\"; it only appends and adjust the\n> fixed-size header.\n\nI thought we took the packfile over --stdin, and we really were\nwriting the entire thing to disk as we processed it. So we could just\nsuppress writing the second entry. But I guess there is some complexity\nwith deltified entries? As in, if the first entry is deltified but the\nsecond is not, you would want to keep the second one?\n\nI'm not complaining if it's really too hard to do in practice, or not\nworth the trouble. I just still don't understand what is causing these\nand when it would come up.\n\n-Peff\n"},{"id":"179714","messageId":"7v62ihkzhb.fsf@alter.siamese.dyndns.org","threadId":"28957","inReplyTo":"20111118184455.GA13782@sigill.intra.peff.net","subject":"Re: [PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-18T19:22:08Z","receivedAt":"2011-11-18T19:22:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Nov 18, 2011 at 10:41:35AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > If we are fixing a thin pack (which should be the case most of the\n>> > time), we are rewriting the packfile anyway. Shouldn't we just omit\n>> > the duplicate?\n>> ...\n> ... But I guess there is some complexity\n> with deltified entries? As in, if the first entry is deltified but the\n> second is not, you would want to keep the second one?\n\nI think you answered your own question here; it is not \"some complexity\"\nbut is exactly the \"you need to memmove() in the output file\" situation in\nthe message you are responding to.\n\nUpon seeing a delta, you would not know if the same object as this delta\nrepresents appears later in the pack stream, which means until you read to\nthe end you wouldn't know. You obviously would not want to hold onto all\ndeltas in-core to \"just omit the duplicate\".\n"},{"id":"179719","messageId":"20111118210825.GA14710@sigill.intra.peff.net","threadId":"28957","inReplyTo":"7v62ihkzhb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-18T21:08:25Z","receivedAt":"2011-11-18T21:08:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 18, 2011 at 11:22:08AM -0800, Junio C Hamano wrote:\n\n> >> > If we are fixing a thin pack (which should be the case most of the\n> >> > time), we are rewriting the packfile anyway. Shouldn't we just omit\n> >> > the duplicate?\n> >> ...\n> > ... But I guess there is some complexity\n> > with deltified entries? As in, if the first entry is deltified but the\n> > second is not, you would want to keep the second one?\n> \n> I think you answered your own question here; it is not \"some complexity\"\n> but is exactly the \"you need to memmove() in the output file\" situation in\n> the message you are responding to.\n> \n> Upon seeing a delta, you would not know if the same object as this delta\n> represents appears later in the pack stream, which means until you read to\n> the end you wouldn't know. You obviously would not want to hold onto all\n> deltas in-core to \"just omit the duplicate\".\n\nOK, that makes sense to me. It's nice to hear it confirmed from somebody\nwho obviously thought more about it.\n\n-Peff\n"},{"id":"179726","messageId":"CAJo=hJukf1C56-rSurJm5voKw+8T9up=9_Umhn032n4rNnLviw@mail.gmail.com","threadId":"28957","inReplyTo":"20111118103355.GA4854@sigill.intra.peff.net","subject":"Re: [PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-11-18T23:50:51Z","receivedAt":"2011-11-18T23:50:51Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Nov 18, 2011 at 02:33, Jeff King wrote:\n> On Wed, Nov 16, 2011 at 10:04:13PM -0800, Junio C Hamano wrote:\n>\n>> When receive-pack & fetch-pack are run and store the pack obtained over\n>> the wire to a local repository, they internally run the index-pack command\n>> with the --strict option. Make sure that we reject incoming packfile that\n>> records objects twice to avoid spreading such a damage.\n>\n> If we are fixing a thin pack (which should be the case most of the\n> time), we are rewriting the packfile anyway. Shouldn't we just omit\n> the duplicate?\n>\n> I guess I'm a little confused about what is generating these duplicates.\n> A buggy git? A malicious server? Bad luck?\n\nA buggy Git. We found a case where JGit could generate duplicate\nobjects in the pack stream during a clone. The resulting client\nworked... until it tried to do `git gc` or really any sort of `git\npack-objects`.\n\nIn my opinion, a pack should never contain duplicate objects. Its a\nbuggy remote that sends them. What I like about this patch is it stops\nand tells the user the remote is broken, which it is.\n"}]}