{"thread":{"id":"51798","subject":"[RFC PATCH 0/1] commit-graph.c: handle corrupt commit trees","startedAt":"2019-09-04T02:22:42Z","lastAt":"2019-09-09T16:54:24Z","messageCount":20,"participants":["Taylor Blau","Jeff King","Garima Singh","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"381784","messageId":"cover.1567563244.git.me@ttaylorr.com","threadId":"51798","inReplyTo":null,"subject":"[RFC PATCH 0/1] commit-graph.c: handle corrupt commit trees","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-04T02:22:38Z","receivedAt":"2019-09-04T02:22:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nI was running some of the new 'git commit-graph' commands, and noticed\nthat I could consistently get 'git commit-graph write --reachable' to\nsegfault when a commit's root tree is corrupt.\n\nI have an extremely-unfinished fix attached as an RFC PATCH below, but I\nwanted to get a few thoughts on this before sending it out as a non-RFC.\n\nIn my patch, I simply 'die()' when a commit isn't able to be parsed\n(i.e., when 'parse_commit_no_graph' returns a non-zero code), but I\nwanted to see if others thought that this was an OK approach. Some\nthoughts:\n\n  * It seems like we could write a commit-graph by placing a \"filler\"\n    entry where the broken commit would have gone. I don't see any place\n    where this is implemented currently, but this seems like a viable\n    alternative to not writing _any_ commits into the commit-graph.\n\n  * Could we learn about the corruption earlier? Ideally (in the obscene\n    of these placeholder objects that indicate corruption), we wouldn't\n    start writing a commit-graph until all of the objects in it are\n    known to be good.\n\n    This seems like a costly operation when it comes to memory, but\n    maybe I'm thinking about it the wrong way.\n\nDepending on the approach here, I'll clean up the commit and message, as\nwell as add a test that demonstrates the breakage and subsequent fix.\n\nThanks in advance for your feedback :-).\n\nTaylor Blau (1):\n  commit-graph.c: die on un-parseable commits\n\n commit-graph.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\n--\n2.22.0\n"},{"id":"381785","messageId":"34e4ec793cb0d321d16b88777cd2db64ed7b772e.1567563244.git.me@ttaylorr.com","threadId":"51798","inReplyTo":"cover.1567563244.git.me@ttaylorr.com","subject":"[RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-04T02:22:57Z","receivedAt":"2019-09-04T02:23:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When we write a commit graph chunk, we process a given list of 'struct\ncommit *'s and parse out the parent(s) and tree OID in order to write\nout its information.\n\nWe do this by calling 'parse_commit_no_graph', and then checking the\nresult of 'get_commit_tree_oid' to write the tree OID. This process\nassumes that 'parse_commit_no_graph' parses the commit successfully.\nWhen this isn't the case, 'get_commit_tree_oid(*list)' may return NULL,\nin which case trying to '->hash' it causes a SIGSEGV.\n\nInstead, teach 'write_graph_chunk_data' to stop when a commit isn't able\nto be parsed, at the peril of failing to write a commit-graph.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f2888c203b..6aa6998ecd 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -843,7 +843,9 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \t\tuint32_t packedDate[2];\n \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n \n-\t\tparse_commit_no_graph(*list);\n+\t\tif (parse_commit_no_graph(*list))\n+\t\t\tdie(_(\"unable to parse commit %s\"),\n+\t\t\t\toid_to_hex(&(*list)->object.oid));\n \t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n \n \t\tparent = (*list)->parents;\n-- \n2.22.0\n"},{"id":"381786","messageId":"20190904030456.GA28836@sigill.intra.peff.net","threadId":"51798","inReplyTo":"34e4ec793cb0d321d16b88777cd2db64ed7b772e.1567563244.git.me@ttaylorr.com","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-04T03:04:56Z","receivedAt":"2019-09-04T03:04:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 03, 2019 at 10:22:57PM -0400, Taylor Blau wrote:\n\n> When we write a commit graph chunk, we process a given list of 'struct\n> commit *'s and parse out the parent(s) and tree OID in order to write\n> out its information.\n> \n> We do this by calling 'parse_commit_no_graph', and then checking the\n> result of 'get_commit_tree_oid' to write the tree OID. This process\n> assumes that 'parse_commit_no_graph' parses the commit successfully.\n> When this isn't the case, 'get_commit_tree_oid(*list)' may return NULL,\n> in which case trying to '->hash' it causes a SIGSEGV.\n> \n> Instead, teach 'write_graph_chunk_data' to stop when a commit isn't able\n> to be parsed, at the peril of failing to write a commit-graph.\n\nYeah, I think it makes sense for commit-graph to bail completely if it\ncan't parse here. In theory it could omit the entry from the\ncommit-graph file (and a reader would just fall back to trying to access\nthe object contents itself), but I think we're too late in the process\nfor that. And besides, this should generally only happen in a corrupt\nrepository.\n\nHowever...\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> index f2888c203b..6aa6998ecd 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -843,7 +843,9 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n>  \t\tuint32_t packedDate[2];\n>  \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n>  \n> -\t\tparse_commit_no_graph(*list);\n> +\t\tif (parse_commit_no_graph(*list))\n> +\t\t\tdie(_(\"unable to parse commit %s\"),\n> +\t\t\t\toid_to_hex(&(*list)->object.oid));\n>  \t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n\nI don't think parse_commit_no_graph() returning 0 assures us that\nget_commit_tree() and friends will return non-NULL.\n\nThis is similar to the case discussed recently where a failed parse of a\ntag may leave \"tag->tagged == NULL\" even though \"tag->obj.parsed\" is\nset.\n\nHere an earlier parsing error could cause (*list)->object.parsed to be\ntrue, but with (*list)->maybe_tree still NULL. Our call to\nparse_commit_no_graph() here would silently return \"yep, already tried\nto parse this\", and then we'd still segfault.\n\nWe _could_ check:\n\n  if (parse_commit_no_graph(*list))\n\tdie(\"unable to parse...\");\n  tree = get_commit_tree_oid(*list);\n  if (!tree)\n\tdie(\"unable to get tree for %s...\");\n\nbut trees are just one piece of data. In fact, the situation is much\nworse for parents: there a NULL parent pointer is valid data, so worse\nthan segfaulting, we'd write invalid data to the commit graph, skipping\none or more parents!\n\nAnd I think there's literally no way for this function to tell the\ndifference between \"no parent\" and \"there was an earlier error, but we\nset the parsed flag anyway and the parent flag is invalid\".\n\nI think that argues against Junio's response in:\n\n  https://public-inbox.org/git/xmqqo90bhmi3.fsf@gitster-ct.c.googlers.com/\n\nabout how we can use the parsed flag to look at \"slightly corrupt\"\nobjects. I think we'd need at least an extra flag for \"corrupt\", though\nI think it is simpler just _not_ setting \"parsed\" and letting the next\ncaller spend the extra cycles to rediscover the problem if they're\ninterested.\n\n(All of this presupposes that you can convince another piece of code in\nthe same process to parse the commit buffer and ignore the error. I have\nno idea if that's possible or not in this case, but it sure would be\nnice not to have to care).\n\n-Peff\n"},{"id":"381809","messageId":"c5ba4eaa-f995-5f2e-4f0f-a8f59ba65fd3@gmail.com","threadId":"51798","inReplyTo":"cover.1567563244.git.me@ttaylorr.com","subject":"Re: [RFC PATCH 0/1] commit-graph.c: handle corrupt commit trees","fromName":"Garima Singh","fromEmail":"garimasigit@gmail.com","sentAt":"2019-09-04T18:25:55Z","receivedAt":"2019-09-04T18:25:58Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"\nOn 9/3/2019 10:22 PM, Taylor Blau wrote:\n> Hi,\n> \n> I was running some of the new 'git commit-graph' commands, and noticed\n> that I could consistently get 'git commit-graph write --reachable' to\n> segfault when a commit's root tree is corrupt.\n> \n> I have an extremely-unfinished fix attached as an RFC PATCH below, but I\n> wanted to get a few thoughts on this before sending it out as a non-RFC.\n> \n> In my patch, I simply 'die()' when a commit isn't able to be parsed\n> (i.e., when 'parse_commit_no_graph' returns a non-zero code), but I\n> wanted to see if others thought that this was an OK approach. Some\n> thoughts:\n\nI like the idea of completely bailing if the commit can't be parsed too.\nOnly question: Is there a reason you chose to die() instead of BUG() \nlike the other two places in that function? What is the criteria of \nchoosing one over the other?\n\n> \n>    * It seems like we could write a commit-graph by placing a \"filler\"\n>      entry where the broken commit would have gone. I don't see any place\n>      where this is implemented currently, but this seems like a viable\n>      alternative to not writing _any_ commits into the commit-graph.\n\nI would rather we didn't do this cause it will probably kick open the \ncan of always watching for that filler when we are working with the \ncommit-graph. Or do we already do that today? Maybe @stolee can chime in \non what we do in cases of shallow clones and other potential gaps in the \nwalk\n\n-Garima\n"},{"id":"381831","messageId":"20190904211847.GA20904@syl.local","threadId":"51798","inReplyTo":"20190904030456.GA28836@sigill.intra.peff.net","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-04T21:18:47Z","receivedAt":"2019-09-04T21:18:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nOn Tue, Sep 03, 2019 at 11:04:56PM -0400, Jeff King wrote:\n> On Tue, Sep 03, 2019 at 10:22:57PM -0400, Taylor Blau wrote:\n>\n> > When we write a commit graph chunk, we process a given list of 'struct\n> > commit *'s and parse out the parent(s) and tree OID in order to write\n> > out its information.\n> >\n> > We do this by calling 'parse_commit_no_graph', and then checking the\n> > result of 'get_commit_tree_oid' to write the tree OID. This process\n> > assumes that 'parse_commit_no_graph' parses the commit successfully.\n> > When this isn't the case, 'get_commit_tree_oid(*list)' may return NULL,\n> > in which case trying to '->hash' it causes a SIGSEGV.\n> >\n> > Instead, teach 'write_graph_chunk_data' to stop when a commit isn't able\n> > to be parsed, at the peril of failing to write a commit-graph.\n>\n> Yeah, I think it makes sense for commit-graph to bail completely if it\n> can't parse here. In theory it could omit the entry from the\n> commit-graph file (and a reader would just fall back to trying to access\n> the object contents itself), but I think we're too late in the process\n> for that. And besides, this should generally only happen in a corrupt\n> repository.\n\nYep. I sent this as an RFC PATCH because I wasn't quite sure how folks\nwould feel about 'die()'-ing in the middle of 'write_graph_chunk_data',\nbut I think that your reasoning makes sense, and it matches my own\npreferences.\n\nSo, I am glad that we resolved that portion. I'll keep going below...\n\n> However...\n>\n> > diff --git a/commit-graph.c b/commit-graph.c\n> > index f2888c203b..6aa6998ecd 100644\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n> > @@ -843,7 +843,9 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n> >  \t\tuint32_t packedDate[2];\n> >  \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n> >\n> > -\t\tparse_commit_no_graph(*list);\n> > +\t\tif (parse_commit_no_graph(*list))\n> > +\t\t\tdie(_(\"unable to parse commit %s\"),\n> > +\t\t\t\toid_to_hex(&(*list)->object.oid));\n> >  \t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n>\n> I don't think parse_commit_no_graph() returning 0 assures us that\n> get_commit_tree() and friends will return non-NULL.\n>\n> This is similar to the case discussed recently where a failed parse of a\n> tag may leave \"tag->tagged == NULL\" even though \"tag->obj.parsed\" is\n> set.\n>\n> Here an earlier parsing error could cause (*list)->object.parsed to be\n> true, but with (*list)->maybe_tree still NULL. Our call to\n> parse_commit_no_graph() here would silently return \"yep, already tried\n> to parse this\", and then we'd still segfault.\n>\n> We _could_ check:\n>\n>   if (parse_commit_no_graph(*list))\n> \tdie(\"unable to parse...\");\n>   tree = get_commit_tree_oid(*list);\n>   if (!tree)\n> \tdie(\"unable to get tree for %s...\");\n>\n> but trees are just one piece of data. In fact, the situation is much\n> worse for parents: there a NULL parent pointer is valid data, so worse\n> than segfaulting, we'd write invalid data to the commit graph, skipping\n> one or more parents!\n>\n> And I think there's literally no way for this function to tell the\n> difference between \"no parent\" and \"there was an earlier error, but we\n> set the parsed flag anyway and the parent flag is invalid\".\n>\n> I think that argues against Junio's response in:\n>\n>   https://public-inbox.org/git/xmqqo90bhmi3.fsf@gitster-ct.c.googlers.com/\n>\n> about how we can use the parsed flag to look at \"slightly corrupt\"\n> objects. I think we'd need at least an extra flag for \"corrupt\", though\n> I think it is simpler just _not_ setting \"parsed\" and letting the next\n> caller spend the extra cycles to rediscover the problem if they're\n> interested.\n\nAll of this makes sense to me, so I'm wondering what part(s) of this you\nfeel are worth addressing in this first patch series. Presumably, there\nis a longer series that we _could_ write which would introduce a new\n'corrupt' field and then check for it here.\n\nBut, I'm hesitant to write those patches, since I only have this one\ncall-site in mind. If we introduce 'corrupt', I feel it would be best to\nuse it uniformly, instead of only checking it here, and relying on other\nbespoke mechanisms to detect corruption elsewhere.\n\nSo, I'm content to write the pseudo-code you provided above (which is to\nsay, call and check both 'parse_commit_no_graph', _and_\n'get_commit_tree_oid'), because I think that it's expedient, and fix the\nissue which I'm pointing out here.\n\nI don't know how to address the parents situation without going further,\nso perhaps it's worth it to think of an alternative, or even leave it\nout of these first patch(es).\n\nLet me know what you think, and thanks for your thoughts so far.\n\n> (All of this presupposes that you can convince another piece of code in\n> the same process to parse the commit buffer and ignore the error. I have\n> no idea if that's possible or not in this case, but it sure would be\n> nice not to have to care).\n>\n> -Peff\nThanks,\nTaylor\n"},{"id":"381832","messageId":"20190904212121.GB20904@syl.local","threadId":"51798","inReplyTo":"c5ba4eaa-f995-5f2e-4f0f-a8f59ba65fd3@gmail.com","subject":"Re: [RFC PATCH 0/1] commit-graph.c: handle corrupt commit trees","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-04T21:21:21Z","receivedAt":"2019-09-04T21:21:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Garima,\n\nOn Wed, Sep 04, 2019 at 02:25:55PM -0400, Garima Singh wrote:\n>\n> On 9/3/2019 10:22 PM, Taylor Blau wrote:\n> > Hi,\n> >\n> > I was running some of the new 'git commit-graph' commands, and noticed\n> > that I could consistently get 'git commit-graph write --reachable' to\n> > segfault when a commit's root tree is corrupt.\n> >\n> > I have an extremely-unfinished fix attached as an RFC PATCH below, but I\n> > wanted to get a few thoughts on this before sending it out as a non-RFC.\n> >\n> > In my patch, I simply 'die()' when a commit isn't able to be parsed\n> > (i.e., when 'parse_commit_no_graph' returns a non-zero code), but I\n> > wanted to see if others thought that this was an OK approach. Some\n> > thoughts:\n>\n> I like the idea of completely bailing if the commit can't be parsed too.\n> Only question: Is there a reason you chose to die() instead of BUG() like\n> the other two places in that function? What is the criteria of choosing one\n> over the other?\n\nI did not call 'BUG' here because 'BUG' is traditionally used to\nindicate an internal bug, e.g., an unexpected state or some such. On the\nother side of that coin, 'BUG' is _not_ used to indicate repository\ncorruption, since that is not an issue in the Git codebase, rather in\nthe user's repository.\n\nThough, to be honest, I've never seen that rule written out explicitly\n(maybe if it were to be written somewhere, it could be stored in\nDocumentation/CodingGuidelines?). I think that this is some good\n#leftoverbits material.\n\n> >\n> >    * It seems like we could write a commit-graph by placing a \"filler\"\n> >      entry where the broken commit would have gone. I don't see any place\n> >      where this is implemented currently, but this seems like a viable\n> >      alternative to not writing _any_ commits into the commit-graph.\n>\n> I would rather we didn't do this cause it will probably kick open the can of\n> always watching for that filler when we are working with the commit-graph.\n> Or do we already do that today? Maybe @stolee can chime in on what we do in\n> cases of shallow clones and other potential gaps in the walk\n\nYeah, I think that the consensus is that it makes sense to just die\nhere, which is fine by me.\n\n> -Garima\n\nThanks,\nTaylor\n"},{"id":"381855","messageId":"20190905060838.GB21450@sigill.intra.peff.net","threadId":"51798","inReplyTo":"20190904212121.GB20904@syl.local","subject":"Re: [RFC PATCH 0/1] commit-graph.c: handle corrupt commit trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T06:08:39Z","receivedAt":"2019-09-05T06:15:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 04, 2019 at 05:21:21PM -0400, Taylor Blau wrote:\n\n> > I like the idea of completely bailing if the commit can't be parsed too.\n> > Only question: Is there a reason you chose to die() instead of BUG() like\n> > the other two places in that function? What is the criteria of choosing one\n> > over the other?\n> \n> I did not call 'BUG' here because 'BUG' is traditionally used to\n> indicate an internal bug, e.g., an unexpected state or some such. On the\n> other side of that coin, 'BUG' is _not_ used to indicate repository\n> corruption, since that is not an issue in the Git codebase, rather in\n> the user's repository.\n> \n> Though, to be honest, I've never seen that rule written out explicitly\n> (maybe if it were to be written somewhere, it could be stored in\n> Documentation/CodingGuidelines?). I think that this is some good\n> #leftoverbits material.\n\nThat rule matches my understanding. A BUG() should be about asserting\ninvariants or catching should-not-happen cases, etc. Any time a BUG()\ntriggers, that is truly a bug in Git, no matter what input got thrown at\nit, what syscalls failed, etc, and is worth fixing (even if the only\nsensible thing is to die()).\n\nAs a side note, we've generally treated segfaults the same way. It\ndoesn't matter if the files on disk or the program input is garbage, we\nshould say so and abort the operation cleanly.\n\n-Peff\n"},{"id":"381857","messageId":"20190905064723.GC21450@sigill.intra.peff.net","threadId":"51798","inReplyTo":"20190904211847.GA20904@syl.local","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T06:47:24Z","receivedAt":"2019-09-05T06:47:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 04, 2019 at 05:18:47PM -0400, Taylor Blau wrote:\n\n> All of this makes sense to me, so I'm wondering what part(s) of this you\n> feel are worth addressing in this first patch series. Presumably, there\n> is a longer series that we _could_ write which would introduce a new\n> 'corrupt' field and then check for it here.\n> \n> But, I'm hesitant to write those patches, since I only have this one\n> call-site in mind. If we introduce 'corrupt', I feel it would be best to\n> use it uniformly, instead of only checking it here, and relying on other\n> bespoke mechanisms to detect corruption elsewhere.\n> \n> So, I'm content to write the pseudo-code you provided above (which is to\n> say, call and check both 'parse_commit_no_graph', _and_\n> 'get_commit_tree_oid'), because I think that it's expedient, and fix the\n> issue which I'm pointing out here.\n\nI'd actually be willing to just take the patch you have here, and\nconsider the \"parsed but we saw an error\" thing as an oddity of the\nobject code.  IOW, we shouldn't _have_ to be double-checking here.\nLooking for an error return from parse_commit() should really be all a\ncaller needs to do. Once that's fixed, then your code would just be\ndoing the right thing.\n\nThat said, there's another unhandled case, I think: lookup_tree() might\nreturn NULL (if somebody previously saw that oid as a non-tree), and\nparse_commit() wouldn't even notice and return an error!\n\nIMHO that's also something that parse_commit() should be returning an\nerror for. And it's probably a lot easier to trigger versus the \"parsed\nearlier but corrupted\" thing.\n\nSo it might be worth doing the NULL tree check here in the meantime. I\ndunno.\n\nBelow is a sketch of what I'm thinking parse_commit() should do:\n\n  - remember when an earlier parse returned an error, so we can repeat\n    that error (this requires some unfortunate bit-field adjusting)\n\n  - notice a lookup_tree failure\n\n  - likewise, notice a lookup_parent failure\n\n---\ndiff --git a/commit.c b/commit.c\nindex a98de16e3d..7e415932b7 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -391,7 +391,9 @@ const void *detach_commit_buffer(struct commit *commit, unsigned long *sizep)\n \treturn ret;\n }\n \n-int parse_commit_buffer(struct repository *r, struct commit *item, const void *buffer, unsigned long size, int check_graph)\n+static int parse_commit_buffer_1(struct repository *r, struct commit *item,\n+\t\t\t\t const void *buffer, unsigned long size,\n+\t\t\t\t int check_graph)\n {\n \tconst char *tail = buffer;\n \tconst char *bufptr = buffer;\n@@ -401,9 +403,6 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tconst int tree_entry_len = the_hash_algo->hexsz + 5;\n \tconst int parent_entry_len = the_hash_algo->hexsz + 7;\n \n-\tif (item->object.parsed)\n-\t\treturn 0;\n-\titem->object.parsed = 1;\n \ttail += size;\n \tif (tail <= bufptr + tree_entry_len + 1 || memcmp(bufptr, \"tree \", 5) ||\n \t\t\tbufptr[tree_entry_len] != '\\n')\n@@ -412,6 +411,10 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \t\treturn error(\"bad tree pointer in commit %s\",\n \t\t\t     oid_to_hex(&item->object.oid));\n \tset_commit_tree(item, lookup_tree(r, &parent));\n+\tif (!item->maybe_tree)\n+\t\treturn error(\"bad tree pointer %s in commit %s\",\n+\t\t\t     oid_to_hex(&parent),\n+\t\t\t     oid_to_hex(&item->object.oid));\n \tbufptr += tree_entry_len + 1; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n \tpptr = &item->parents;\n \n@@ -431,15 +434,19 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \t\tif (graft && (graft->nr_parent < 0 || grafts_replace_parents))\n \t\t\tcontinue;\n \t\tnew_parent = lookup_commit(r, &parent);\n-\t\tif (new_parent)\n-\t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n+\t\tif (!new_parent)\n+\t\t\treturn error(\"bad parent %s in commit %s\",\n+\t\t\t\t     oid_to_hex(&parent),\n+\t\t\t\t     oid_to_hex(&item->object.oid));\n+\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n \t}\n \tif (graft) {\n \t\tint i;\n \t\tstruct commit *new_parent;\n \t\tfor (i = 0; i < graft->nr_parent; i++) {\n \t\t\tnew_parent = lookup_commit(r,\n \t\t\t\t\t\t   &graft->parent[i]);\n+\t\t\t/* Here we ignore bogus grafts. Also should be an error? */\n \t\t\tif (!new_parent)\n \t\t\t\tcontinue;\n \t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n@@ -453,6 +460,23 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \treturn 0;\n }\n \n+int parse_commit_buffer(struct repository *r, struct commit *item,\n+\t\t\tconst void *buffer, unsigned long size,\n+\t\t\tint check_graph)\n+{\n+\tint ret;\n+\n+\tif (item->object.parsed)\n+\t\treturn item->object.corrupt ? -1 : 0;\n+\titem->object.parsed = 1;\n+\n+\tret = parse_commit_buffer_1(r, item, buffer, size, check_graph);\n+\tif (ret < 0)\n+\t\titem->object.corrupt = 1;\n+\n+\treturn ret;\n+}\n+\n int repo_parse_commit_internal(struct repository *r,\n \t\t\t       struct commit *item,\n \t\t\t       int quiet_on_missing,\ndiff --git a/object.h b/object.h\nindex 0120892bbd..b83d3964ad 100644\n--- a/object.h\n+++ b/object.h\n@@ -59,7 +59,7 @@ struct object_array {\n \n /*\n  * object flag allocation:\n- * revision.h:               0---------10                              25----28\n+ * revision.h:               0---------10                              24----27\n  * fetch-pack.c:             01\n  * negotiator/default.c:       2--5\n  * walker.c:                 0-2\n@@ -78,13 +78,14 @@ struct object_array {\n  * builtin/show-branch.c:    0-------------------------------------------26\n  * builtin/unpack-objects.c:                                 2021\n  */\n-#define FLAG_BITS  29\n+#define FLAG_BITS  28\n \n /*\n  * The object type is stored in 3 bits.\n  */\n struct object {\n \tunsigned parsed : 1;\n+\tunsigned corrupt : 1;\n \tunsigned type : TYPE_BITS;\n \tunsigned flags : FLAG_BITS;\n \tstruct object_id oid;\ndiff --git a/revision.h b/revision.h\nindex 4134dc6029..5c0b831b37 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -33,7 +33,7 @@\n #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)\n \n #define TOPO_WALK_EXPLORED\t(1u<<27)\n-#define TOPO_WALK_INDEGREE\t(1u<<28)\n+#define TOPO_WALK_INDEGREE\t(1u<<24)\n \n #define DECORATE_SHORT_REFS\t1\n #define DECORATE_FULL_REFS\t2\n"},{"id":"381925","messageId":"xmqqsgpao06z.fsf@gitster-ct.c.googlers.com","threadId":"51798","inReplyTo":"20190904030456.GA28836@sigill.intra.peff.net","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-05T22:19:48Z","receivedAt":"2019-09-05T22:19:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I don't think parse_commit_no_graph() returning 0 assures us that\n> get_commit_tree() and friends will return non-NULL.\n>\n> This is similar to the case discussed recently where a failed parse of a\n> tag may leave \"tag->tagged == NULL\" even though \"tag->obj.parsed\" is\n> set.\n>\n> Here an earlier parsing error could cause (*list)->object.parsed to be\n> true, but with (*list)->maybe_tree still NULL. Our call to\n> parse_commit_no_graph() here would silently return \"yep, already tried\n> to parse this\", and then we'd still segfault.\n> ...\n> And I think there's literally no way for this function to tell the\n> difference between \"no parent\" and \"there was an earlier error, but we\n> set the parsed flag anyway and the parent flag is invalid\".\n>\n> I think that argues against Junio's response in:\n\nFair enough.  Forcing later users to reattempt parsing (and failing\nthe same way) would be safer and it should also be sufficient as we\nare talking about how to handle a broken repository, i.e. an error\ncase.\n"},{"id":"381946","messageId":"20190906063503.GB5122@sigill.intra.peff.net","threadId":"51798","inReplyTo":"xmqqsgpao06z.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T06:35:03Z","receivedAt":"2019-09-06T06:35:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 05, 2019 at 03:19:48PM -0700, Junio C Hamano wrote:\n\n> > Here an earlier parsing error could cause (*list)->object.parsed to be\n> > true, but with (*list)->maybe_tree still NULL. Our call to\n> > parse_commit_no_graph() here would silently return \"yep, already tried\n> > to parse this\", and then we'd still segfault.\n> > ...\n> > And I think there's literally no way for this function to tell the\n> > difference between \"no parent\" and \"there was an earlier error, but we\n> > set the parsed flag anyway and the parent flag is invalid\".\n> >\n> > I think that argues against Junio's response in:\n> \n> Fair enough.  Forcing later users to reattempt parsing (and failing\n> the same way) would be safer and it should also be sufficient as we\n> are talking about how to handle a broken repository, i.e. an error\n> case.\n\nOne of the tricky things, and the reason I used a \"corrupt\" flag in my\nearlier sketch, is that the state after we encounter a parse error is\nunknown. So imagine parse_commit_buffer() sees that one of the parent\nlines is bogus, and we return an error. The caller gets to see whatever\nhalf-parsed state we managed to come up with.\n\nSo far so good. But now imagine we call parse_commit_buffer() again, and\nwe re-parse. How does that interact with the half-parsed state? Some of\nit works OK (e.g., lookup_tree() would find the same tree). Some not so\nmuch (I think we'd keep appending parents at each call).\n\nI guess this might not be too bad to handle. Value fields like\ntimestamp_t are OK to overwrite. Pointers to objects likewise, since the\nmemory is owned elsewhere. If we see existing parent pointers in an\nobject we're parsing, we could probably free them under the assumption\nthey're leftover cruft. Likewise for the \"tag\" field of \"struct tag\",\nwhich is owned by the struct and should be freed.\n\nBlobs and trees don't actually parse anything into their structs. So\nit's really just special-casing those two items.\n\n-Peff\n"},{"id":"381947","messageId":"20190906065606.GC5122@sigill.intra.peff.net","threadId":"51798","inReplyTo":"20190906063503.GB5122@sigill.intra.peff.net","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T06:56:06Z","receivedAt":"2019-09-06T06:56:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 02:35:03AM -0400, Jeff King wrote:\n\n> > Fair enough.  Forcing later users to reattempt parsing (and failing\n> > the same way) would be safer and it should also be sufficient as we\n> > are talking about how to handle a broken repository, i.e. an error\n> > case.\n> \n> One of the tricky things, and the reason I used a \"corrupt\" flag in my\n> earlier sketch, is that the state after we encounter a parse error is\n> unknown. So imagine parse_commit_buffer() sees that one of the parent\n> lines is bogus, and we return an error. The caller gets to see whatever\n> half-parsed state we managed to come up with.\n> \n> So far so good. But now imagine we call parse_commit_buffer() again, and\n> we re-parse. How does that interact with the half-parsed state? Some of\n> it works OK (e.g., lookup_tree() would find the same tree). Some not so\n> much (I think we'd keep appending parents at each call).\n> \n> I guess this might not be too bad to handle. Value fields like\n> timestamp_t are OK to overwrite. Pointers to objects likewise, since the\n> memory is owned elsewhere. If we see existing parent pointers in an\n> object we're parsing, we could probably free them under the assumption\n> they're leftover cruft. Likewise for the \"tag\" field of \"struct tag\",\n> which is owned by the struct and should be freed.\n> \n> Blobs and trees don't actually parse anything into their structs. So\n> it's really just special-casing those two items.\n\nSo here's something a bit more concrete to play with. Using the patch\nbelow, we maintain the invariant that if you called parse_commit() or\nequivalent and got a successful return, then commit->tree is always\nnon-NULL. That fixes the second \"missing tree\" test from elsewhere in\nthis thread:\n\n  https://public-inbox.org/git/042a8ba8b2a98c269f9cd1a8e88488b80d686f0d.1567720960.git.me@ttaylorr.com/\n\n_without_ applying the third patch (though the error message we expect\nin the test does not tweaked). And it likely also protects most of the\nother callers of get_commit_tree_oid(), assuming somewhere in their code\npath they call parse_commit() and actually check the error code.\n\nLikewise, tag->tagged is always non-NULL after a successful tag parse,\nwhich should fix the segfault discussed recently in:\n\n  https://public-inbox.org/git/20190824230944.GA14132@jessup.stsp.name/\n\nas well as probably others.\n\nAnd callers are still able to view the broken objects, but they have to\nignore the error return from the parse functions.\n\nThis seems like a promising direction. I'd probably break it into a few\nseparate patches, and it would be nice to have some tests (especially\none where we actually do try to parse multiple times).\n\n---\ndiff --git a/commit.c b/commit.c\nindex a98de16e3d..47e36fd13e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -398,20 +398,37 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tstruct object_id parent;\n \tstruct commit_list **pptr;\n \tstruct commit_graft *graft;\n+\tstruct tree *tree;\n \tconst int tree_entry_len = the_hash_algo->hexsz + 5;\n \tconst int parent_entry_len = the_hash_algo->hexsz + 7;\n \n \tif (item->object.parsed)\n \t\treturn 0;\n-\titem->object.parsed = 1;\n+\n+\tif (item->parents) {\n+\t\t/*\n+\t\t * Presumably this is leftover from an earlier failed parse;\n+\t\t * clear it out in preparation for us re-parsing (we'll hit the\n+\t\t * same error, but that's good, since it lets our caller know\n+\t\t * the result cannot be trusted.\n+\t\t */\n+\t\tfree_commit_list(item->parents);\n+\t\titem->parents = NULL;\n+\t}\n+\n \ttail += size;\n \tif (tail <= bufptr + tree_entry_len + 1 || memcmp(bufptr, \"tree \", 5) ||\n \t\t\tbufptr[tree_entry_len] != '\\n')\n \t\treturn error(\"bogus commit object %s\", oid_to_hex(&item->object.oid));\n \tif (get_oid_hex(bufptr + 5, &parent) < 0)\n \t\treturn error(\"bad tree pointer in commit %s\",\n \t\t\t     oid_to_hex(&item->object.oid));\n-\tset_commit_tree(item, lookup_tree(r, &parent));\n+\ttree = lookup_tree(r, &parent);\n+\tif (!tree)\n+\t\treturn error(\"bad tree pointer %s in commit %s\",\n+\t\t\t     oid_to_hex(&parent),\n+\t\t\t     oid_to_hex(&item->object.oid));\n+\tset_commit_tree(item, tree);\n \tbufptr += tree_entry_len + 1; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n \tpptr = &item->parents;\n \n@@ -450,6 +467,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tif (check_graph)\n \t\tload_commit_graph_info(r, item);\n \n+\titem->object.parsed = 1;\n \treturn 0;\n }\n \ndiff --git a/tag.c b/tag.c\nindex 5db870edb9..3dcebb4715 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -141,7 +141,11 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \n \tif (item->object.parsed)\n \t\treturn 0;\n-\titem->object.parsed = 1;\n+\n+\tif (item->tag) {\n+\t\t/* Left over from an earlier failed parse */\n+\t\tFREE_AND_NULL(item->tag);\n+\t}\n \n \tif (size < the_hash_algo->hexsz + 24)\n \t\treturn -1;\n@@ -167,10 +171,15 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \t} else if (!strcmp(type, tag_type)) {\n \t\titem->tagged = (struct object *)lookup_tag(r, &oid);\n \t} else {\n-\t\terror(\"Unknown type %s\", type);\n-\t\titem->tagged = NULL;\n+\t\treturn error(\"unknown tag type '%s' in %s\",\n+\t\t\t     type, oid_to_hex(&item->object.oid));\n \t}\n \n+\tif (!item->tagged)\n+\t\treturn error(\"bad tag pointer to %s in %s\",\n+\t\t\t     oid_to_hex(&oid),\n+\t\t\t     oid_to_hex(&item->object.oid));\n+\n \tif (bufptr + 4 < tail && starts_with(bufptr, \"tag \"))\n \t\t; \t\t/* good */\n \telse\n@@ -187,6 +196,7 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \telse\n \t\titem->date = 0;\n \n+\titem->object.parsed = 1;\n \treturn 0;\n }\n \n"},{"id":"381961","messageId":"36bf0064-b563-74ed-4ae5-01745ced5d2e@gmail.com","threadId":"51798","inReplyTo":"20190905064723.GC21450@sigill.intra.peff.net","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-06T16:48:05Z","receivedAt":"2019-09-06T16:48:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/5/2019 2:47 AM, Jeff King wrote:\n> On Wed, Sep 04, 2019 at 05:18:47PM -0400, Taylor Blau wrote:\n> \n>> All of this makes sense to me, so I'm wondering what part(s) of this you\n>> feel are worth addressing in this first patch series. Presumably, there\n>> is a longer series that we _could_ write which would introduce a new\n>> 'corrupt' field and then check for it here.\n>>\n>> But, I'm hesitant to write those patches, since I only have this one\n>> call-site in mind. If we introduce 'corrupt', I feel it would be best to\n>> use it uniformly, instead of only checking it here, and relying on other\n>> bespoke mechanisms to detect corruption elsewhere.\n>>\n>> So, I'm content to write the pseudo-code you provided above (which is to\n>> say, call and check both 'parse_commit_no_graph', _and_\n>> 'get_commit_tree_oid'), because I think that it's expedient, and fix the\n>> issue which I'm pointing out here.\n> \n> I'd actually be willing to just take the patch you have here, and\n> consider the \"parsed but we saw an error\" thing as an oddity of the\n> object code.  IOW, we shouldn't _have_ to be double-checking here.\n> Looking for an error return from parse_commit() should really be all a\n> caller needs to do. Once that's fixed, then your code would just be\n> doing the right thing.\n> \n> That said, there's another unhandled case, I think: lookup_tree() might\n> return NULL (if somebody previously saw that oid as a non-tree), and\n> parse_commit() wouldn't even notice and return an error!\n> \n> IMHO that's also something that parse_commit() should be returning an\n> error for. And it's probably a lot easier to trigger versus the \"parsed\n> earlier but corrupted\" thing.\n> \n> So it might be worth doing the NULL tree check here in the meantime. I\n> dunno.\n> \n> Below is a sketch of what I'm thinking parse_commit() should do:\n> \n>   - remember when an earlier parse returned an error, so we can repeat\n>     that error (this requires some unfortunate bit-field adjusting)\n> \n>   - notice a lookup_tree failure\n> \n>   - likewise, notice a lookup_parent failure\n> \n> ---\n> diff --git a/commit.c b/commit.c\n> index a98de16e3d..7e415932b7 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -391,7 +391,9 @@ const void *detach_commit_buffer(struct commit *commit, unsigned long *sizep)\n>  \treturn ret;\n>  }\n>  \n> -int parse_commit_buffer(struct repository *r, struct commit *item, const void *buffer, unsigned long size, int check_graph)\n> +static int parse_commit_buffer_1(struct repository *r, struct commit *item,\n> +\t\t\t\t const void *buffer, unsigned long size,\n> +\t\t\t\t int check_graph)\n>  {\n>  \tconst char *tail = buffer;\n>  \tconst char *bufptr = buffer;\n> @@ -401,9 +403,6 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n>  \tconst int tree_entry_len = the_hash_algo->hexsz + 5;\n>  \tconst int parent_entry_len = the_hash_algo->hexsz + 7;\n>  \n> -\tif (item->object.parsed)\n> -\t\treturn 0;\n> -\titem->object.parsed = 1;\n>  \ttail += size;\n>  \tif (tail <= bufptr + tree_entry_len + 1 || memcmp(bufptr, \"tree \", 5) ||\n>  \t\t\tbufptr[tree_entry_len] != '\\n')\n> @@ -412,6 +411,10 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n>  \t\treturn error(\"bad tree pointer in commit %s\",\n>  \t\t\t     oid_to_hex(&item->object.oid));\n>  \tset_commit_tree(item, lookup_tree(r, &parent));\n> +\tif (!item->maybe_tree)\n> +\t\treturn error(\"bad tree pointer %s in commit %s\",\n> +\t\t\t     oid_to_hex(&parent),\n> +\t\t\t     oid_to_hex(&item->object.oid));\n>  \tbufptr += tree_entry_len + 1; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n>  \tpptr = &item->parents;\n>  \n> @@ -431,15 +434,19 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n>  \t\tif (graft && (graft->nr_parent < 0 || grafts_replace_parents))\n>  \t\t\tcontinue;\n>  \t\tnew_parent = lookup_commit(r, &parent);\n> -\t\tif (new_parent)\n> -\t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n> +\t\tif (!new_parent)\n> +\t\t\treturn error(\"bad parent %s in commit %s\",\n> +\t\t\t\t     oid_to_hex(&parent),\n> +\t\t\t\t     oid_to_hex(&item->object.oid));\n> +\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n>  \t}\n>  \tif (graft) {\n>  \t\tint i;\n>  \t\tstruct commit *new_parent;\n>  \t\tfor (i = 0; i < graft->nr_parent; i++) {\n>  \t\t\tnew_parent = lookup_commit(r,\n>  \t\t\t\t\t\t   &graft->parent[i]);\n> +\t\t\t/* Here we ignore bogus grafts. Also should be an error? */\n>  \t\t\tif (!new_parent)\n>  \t\t\t\tcontinue;\n>  \t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n> @@ -453,6 +460,23 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n>  \treturn 0;\n>  }\n>  \n> +int parse_commit_buffer(struct repository *r, struct commit *item,\n> +\t\t\tconst void *buffer, unsigned long size,\n> +\t\t\tint check_graph)\n> +{\n> +\tint ret;\n> +\n> +\tif (item->object.parsed)\n> +\t\treturn item->object.corrupt ? -1 : 0;\n> +\titem->object.parsed = 1;\n> +\n> +\tret = parse_commit_buffer_1(r, item, buffer, size, check_graph);\n> +\tif (ret < 0)\n> +\t\titem->object.corrupt = 1;\n> +\n> +\treturn ret;\n> +}\n> +\n>  int repo_parse_commit_internal(struct repository *r,\n>  \t\t\t       struct commit *item,\n>  \t\t\t       int quiet_on_missing,\n> diff --git a/object.h b/object.h\n> index 0120892bbd..b83d3964ad 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -59,7 +59,7 @@ struct object_array {\n>  \n>  /*\n>   * object flag allocation:\n> - * revision.h:               0---------10                              25----28\n> + * revision.h:               0---------10                              24----27\n>   * fetch-pack.c:             01\n>   * negotiator/default.c:       2--5\n>   * walker.c:                 0-2\n> @@ -78,13 +78,14 @@ struct object_array {\n>   * builtin/show-branch.c:    0-------------------------------------------26\n>   * builtin/unpack-objects.c:                                 2021\n>   */\n> -#define FLAG_BITS  29\n> +#define FLAG_BITS  28\n>  \n>  /*\n>   * The object type is stored in 3 bits.\n>   */\n>  struct object {\n>  \tunsigned parsed : 1;\n> +\tunsigned corrupt : 1;\n>  \tunsigned type : TYPE_BITS;\n>  \tunsigned flags : FLAG_BITS;\n>  \tstruct object_id oid;\n> diff --git a/revision.h b/revision.h\n> index 4134dc6029..5c0b831b37 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -33,7 +33,7 @@\n>  #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)\n>  \n>  #define TOPO_WALK_EXPLORED\t(1u<<27)\n> -#define TOPO_WALK_INDEGREE\t(1u<<28)\n> +#define TOPO_WALK_INDEGREE\t(1u<<24)\n\nAs an aside, these flag bit modifications look fine, but would need to\nbe explained. I'm guessing that since you are adding a bit of data\nto struct object you want to avoid increasing the struct size across\na 32-bit boundary. Are we sure that bit 24 is not used anywhere else?\n(My search for \"1u<<24\" found nothing, and \"1 << 24\" found a bit in\nthe cache-entry flags, so this seems safe.)\n\nThanks,\n-Stolee\n"},{"id":"381963","messageId":"a2f6d2d9-e7ea-addd-97a0-f05fa3f1be51@gmail.com","threadId":"51798","inReplyTo":"20190904212121.GB20904@syl.local","subject":"Re: [RFC PATCH 0/1] commit-graph.c: handle corrupt commit trees","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-06T16:48:36Z","receivedAt":"2019-09-06T16:48:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/4/2019 5:21 PM, Taylor Blau wrote:\n> Hi Garima,\n> \n> On Wed, Sep 04, 2019 at 02:25:55PM -0400, Garima Singh wrote:\n>>\n>> On 9/3/2019 10:22 PM, Taylor Blau wrote:\n>>> Hi,\n>>>\n>>> I was running some of the new 'git commit-graph' commands, and noticed\n>>> that I could consistently get 'git commit-graph write --reachable' to\n>>> segfault when a commit's root tree is corrupt.\n>>>\n>>> I have an extremely-unfinished fix attached as an RFC PATCH below, but I\n>>> wanted to get a few thoughts on this before sending it out as a non-RFC.\n>>>\n>>> In my patch, I simply 'die()' when a commit isn't able to be parsed\n>>> (i.e., when 'parse_commit_no_graph' returns a non-zero code), but I\n>>> wanted to see if others thought that this was an OK approach. Some\n>>> thoughts:\n>>\n>> I like the idea of completely bailing if the commit can't be parsed too.\n>> Only question: Is there a reason you chose to die() instead of BUG() like\n>> the other two places in that function? What is the criteria of choosing one\n>> over the other?\n> \n> I did not call 'BUG' here because 'BUG' is traditionally used to\n> indicate an internal bug, e.g., an unexpected state or some such. On the\n> other side of that coin, 'BUG' is _not_ used to indicate repository\n> corruption, since that is not an issue in the Git codebase, rather in\n> the user's repository.\n> \n> Though, to be honest, I've never seen that rule written out explicitly\n> (maybe if it were to be written somewhere, it could be stored in\n> Documentation/CodingGuidelines?). I think that this is some good\n> #leftoverbits material.\n> \n>>>\n>>>    * It seems like we could write a commit-graph by placing a \"filler\"\n>>>      entry where the broken commit would have gone. I don't see any place\n>>>      where this is implemented currently, but this seems like a viable\n>>>      alternative to not writing _any_ commits into the commit-graph.\n>>\n>> I would rather we didn't do this cause it will probably kick open the can of\n>> always watching for that filler when we are working with the commit-graph.\n>> Or do we already do that today? Maybe @stolee can chime in on what we do in\n>> cases of shallow clones and other potential gaps in the walk\n> \n> Yeah, I think that the consensus is that it makes sense to just die\n> here, which is fine by me.\n\nI agree the die() is the best thing to do for now.\n\nIf we wanted to salvage as much as possible, then we could use these\ncorrupt marks and then use the \"reverse walk\" in compute_generation_numbers()\nto mark all commits that can reach the corrupt commit as corrupt.\nWe would then need to remove all corrupt commits from the list we are\nplanning to write.\n\nHowever, that is just hiding a corrupt object in the object database,\nwhich is not a situation we want to leave unnoticed.\n\nThanks,\n-Stolee\n"},{"id":"381966","messageId":"xmqqk1alnyxz.fsf@gitster-ct.c.googlers.com","threadId":"51798","inReplyTo":"20190906063503.GB5122@sigill.intra.peff.net","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-06T16:59:04Z","receivedAt":"2019-09-06T16:59:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So far so good. But now imagine we call parse_commit_buffer() again, and\n> we re-parse. How does that interact with the half-parsed state? Some of\n> it works OK (e.g., lookup_tree() would find the same tree). Some not so\n> much (I think we'd keep appending parents at each call).\n>\n> I guess this might not be too bad to handle. Value fields like\n> timestamp_t are OK to overwrite. Pointers to objects likewise, since the\n> memory is owned elsewhere. If we see existing parent pointers in an\n> object we're parsing, we could probably free them under the assumption\n> they're leftover cruft. Likewise for the \"tag\" field of \"struct tag\",\n> which is owned by the struct and should be freed.\n\nYeah, or clear them before returning with .corrupt bit set?\n"},{"id":"381969","messageId":"20190906170417.GA23181@sigill.intra.peff.net","threadId":"51798","inReplyTo":"36bf0064-b563-74ed-4ae5-01745ced5d2e@gmail.com","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T17:04:17Z","receivedAt":"2019-09-06T17:04:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 12:48:05PM -0400, Derrick Stolee wrote:\n\n> > diff --git a/revision.h b/revision.h\n> > index 4134dc6029..5c0b831b37 100644\n> > --- a/revision.h\n> > +++ b/revision.h\n> > @@ -33,7 +33,7 @@\n> >  #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)\n> >  \n> >  #define TOPO_WALK_EXPLORED\t(1u<<27)\n> > -#define TOPO_WALK_INDEGREE\t(1u<<28)\n> > +#define TOPO_WALK_INDEGREE\t(1u<<24)\n> \n> As an aside, these flag bit modifications look fine, but would need to\n> be explained. I'm guessing that since you are adding a bit of data\n> to struct object you want to avoid increasing the struct size across\n> a 32-bit boundary. Are we sure that bit 24 is not used anywhere else?\n> (My search for \"1u<<24\" found nothing, and \"1 << 24\" found a bit in\n> the cache-entry flags, so this seems safe.)\n\nYeah, I'd definitely break this up into several commits with explanation\n(though see an alternate I posted that just uses the parsed flag without\nany new bits).\n\nBit 24 isn't used according to the table in objects.h, which is\n_supposed_ to be the source of truth, though of course there's no\ncompiler-level checking. (One aside: is there a reason TOPO_WALK_* isn't\npart of ALL_REV_FLAGS?).\n\nAnd yes, the goal was to keep things to the 32-bit boundary. But in the\ncourse of this, I discovered something interesting: 64-bit systems are\nnow padding this up to the 8-byte boundary!\n\nThe culprit is the switch of GIT_MAX_RAWSZ for sha256. Before then, our\nobject_id was 20 bytes for sha1. Adding 4 bytes of flags still left us\nat 24 bytes, which is both 4- and 8-byte aligned.\n\nWith the switch to sha256, object_id is now 32 bytes. Adding 4 bytes\ntakes us to 36, and then 8-byte aligning the struct takes us to 40\nbytes, with 4 bytes of wasted padding.\n\nI'm sorely tempted to use this as an opportunity to move commit->index\ninto \"struct object\". That would actually shrink commit object sizes by\n4 bytes, and would let all object types do the commit-slab trick to\nstore object data with constant-time lookup. This would make it possible\nto migrate some uses of flags to per-operation bitfields (so e.g., two\ntraversals would have their _own_ flag data, and wouldn't risk stomping\non each other's bits).\n\nThe one downside would be that the index space would become more sparse.\nI.e., right now if you're only storing things for commits in a slab, you\nknow that every slot you allocate is for a commit. But if we allocate an\nindex for each object, then the commits are less likely to be together\n(so wasted memory and worse cache performance). That might be solvable\nby assigning a per-type index (with a few hacks to handle OBJ_NONE).\n\nAnyway, all of that is rather off the topic of this discussion.\n\n-Peff\n"},{"id":"381970","messageId":"20190906170451.GB23181@sigill.intra.peff.net","threadId":"51798","inReplyTo":"xmqqk1alnyxz.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T17:04:52Z","receivedAt":"2019-09-06T17:04:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 09:59:04AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > So far so good. But now imagine we call parse_commit_buffer() again, and\n> > we re-parse. How does that interact with the half-parsed state? Some of\n> > it works OK (e.g., lookup_tree() would find the same tree). Some not so\n> > much (I think we'd keep appending parents at each call).\n> >\n> > I guess this might not be too bad to handle. Value fields like\n> > timestamp_t are OK to overwrite. Pointers to objects likewise, since the\n> > memory is owned elsewhere. If we see existing parent pointers in an\n> > object we're parsing, we could probably free them under the assumption\n> > they're leftover cruft. Likewise for the \"tag\" field of \"struct tag\",\n> > which is owned by the struct and should be freed.\n> \n> Yeah, or clear them before returning with .corrupt bit set?\n\nThis was my attempt to avoid dealing with a .corrupt bit. :)\n\n-Peff\n"},{"id":"381972","messageId":"b1121a33-691a-e3ff-a212-48c0d7deae1b@gmail.com","threadId":"51798","inReplyTo":"20190906170417.GA23181@sigill.intra.peff.net","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-06T17:19:21Z","receivedAt":"2019-09-06T17:19:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\n\nOn 9/6/2019 1:04 PM, Jeff King wrote:\n> On Fri, Sep 06, 2019 at 12:48:05PM -0400, Derrick Stolee wrote:\n> \n>>> diff --git a/revision.h b/revision.h\n>>> index 4134dc6029..5c0b831b37 100644\n>>> --- a/revision.h\n>>> +++ b/revision.h\n>>> @@ -33,7 +33,7 @@\n>>>  #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)\n>>>  \n>>>  #define TOPO_WALK_EXPLORED\t(1u<<27)\n>>> -#define TOPO_WALK_INDEGREE\t(1u<<28)\n>>> +#define TOPO_WALK_INDEGREE\t(1u<<24)\n>>\n>> As an aside, these flag bit modifications look fine, but would need to\n>> be explained. I'm guessing that since you are adding a bit of data\n>> to struct object you want to avoid increasing the struct size across\n>> a 32-bit boundary. Are we sure that bit 24 is not used anywhere else?\n>> (My search for \"1u<<24\" found nothing, and \"1 << 24\" found a bit in\n>> the cache-entry flags, so this seems safe.)\n> \n> Yeah, I'd definitely break this up into several commits with explanation\n> (though see an alternate I posted that just uses the parsed flag without\n> any new bits).\n> \n> Bit 24 isn't used according to the table in objects.h, which is\n> _supposed_ to be the source of truth, though of course there's no\n> compiler-level checking. (One aside: is there a reason TOPO_WALK_* isn't\n> part of ALL_REV_FLAGS?).\n\nThis was an oversight on my part. Sorry.\n\n-Stolee\n"},{"id":"381973","messageId":"8a77647b-a924-fd51-f0c8-b30ef0dbc3b4@gmail.com","threadId":"51798","inReplyTo":"20190906170417.GA23181@sigill.intra.peff.net","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-06T17:20:48Z","receivedAt":"2019-09-06T17:20:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\n\nOn 9/6/2019 1:04 PM, Jeff King wrote:\n> On Fri, Sep 06, 2019 at 12:48:05PM -0400, Derrick Stolee wrote:\n> \n>>> diff --git a/revision.h b/revision.h\n>>> index 4134dc6029..5c0b831b37 100644\n>>> --- a/revision.h\n>>> +++ b/revision.h\n>>> @@ -33,7 +33,7 @@\n>>>  #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)\n>>>  \n>>>  #define TOPO_WALK_EXPLORED\t(1u<<27)\n>>> -#define TOPO_WALK_INDEGREE\t(1u<<28)\n>>> +#define TOPO_WALK_INDEGREE\t(1u<<24)\n>>\n>> As an aside, these flag bit modifications look fine, but would need to\n>> be explained. I'm guessing that since you are adding a bit of data\n>> to struct object you want to avoid increasing the struct size across\n>> a 32-bit boundary. Are we sure that bit 24 is not used anywhere else?\n>> (My search for \"1u<<24\" found nothing, and \"1 << 24\" found a bit in\n>> the cache-entry flags, so this seems safe.)\n> \n> Yeah, I'd definitely break this up into several commits with explanation\n> (though see an alternate I posted that just uses the parsed flag without\n> any new bits).\n> \n> Bit 24 isn't used according to the table in objects.h, which is\n> _supposed_ to be the source of truth, though of course there's no\n> compiler-level checking. (One aside: is there a reason TOPO_WALK_* isn't\n> part of ALL_REV_FLAGS?).\n> \n> And yes, the goal was to keep things to the 32-bit boundary. But in the\n> course of this, I discovered something interesting: 64-bit systems are\n> now padding this up to the 8-byte boundary!\n> \n> The culprit is the switch of GIT_MAX_RAWSZ for sha256. Before then, our\n> object_id was 20 bytes for sha1. Adding 4 bytes of flags still left us\n> at 24 bytes, which is both 4- and 8-byte aligned.\n> \n> With the switch to sha256, object_id is now 32 bytes. Adding 4 bytes\n> takes us to 36, and then 8-byte aligning the struct takes us to 40\n> bytes, with 4 bytes of wasted padding.\n> \n> I'm sorely tempted to use this as an opportunity to move commit->index\n> into \"struct object\". That would actually shrink commit object sizes by\n> 4 bytes, and would let all object types do the commit-slab trick to\n> store object data with constant-time lookup. This would make it possible\n> to migrate some uses of flags to per-operation bitfields (so e.g., two\n> traversals would have their _own_ flag data, and wouldn't risk stomping\n> on each other's bits).\n\nThis reminds me that I'm hoping to eventually get around to moving\n\"generation\" into a commit slab. That would reduce the space for people\nstill working without a commit-graph, and would allow updating to\ngeneration number v2 (which needs 64 bits of data).\n\n-Stolee\n"},{"id":"382069","messageId":"xmqqzhjdl8z6.fsf@gitster-ct.c.googlers.com","threadId":"51798","inReplyTo":"20190906170451.GB23181@sigill.intra.peff.net","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-09T16:39:41Z","receivedAt":"2019-09-09T16:39:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Sep 06, 2019 at 09:59:04AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > So far so good. But now imagine we call parse_commit_buffer() again, and\n>> > we re-parse. How does that interact with the half-parsed state? Some of\n>> > it works OK (e.g., lookup_tree() would find the same tree). Some not so\n>> > much (I think we'd keep appending parents at each call).\n>> >\n>> > I guess this might not be too bad to handle. Value fields like\n>> > timestamp_t are OK to overwrite. Pointers to objects likewise, since the\n>> > memory is owned elsewhere. If we see existing parent pointers in an\n>> > object we're parsing, we could probably free them under the assumption\n>> > they're leftover cruft. Likewise for the \"tag\" field of \"struct tag\",\n>> > which is owned by the struct and should be freed.\n>> \n>> Yeah, or clear them before returning with .corrupt bit set?\n>\n> This was my attempt to avoid dealing with a .corrupt bit. :)\n\nThen, clear them before returning with .parsed bit clear?\n"},{"id":"382070","messageId":"20190909165419.GA29955@sigill.intra.peff.net","threadId":"51798","inReplyTo":"xmqqzhjdl8z6.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-09T16:54:20Z","receivedAt":"2019-09-09T16:54:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 09, 2019 at 09:39:41AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Fri, Sep 06, 2019 at 09:59:04AM -0700, Junio C Hamano wrote:\n> >\n> >> Jeff King <peff@peff.net> writes:\n> >> \n> >> > So far so good. But now imagine we call parse_commit_buffer() again, and\n> >> > we re-parse. How does that interact with the half-parsed state? Some of\n> >> > it works OK (e.g., lookup_tree() would find the same tree). Some not so\n> >> > much (I think we'd keep appending parents at each call).\n> >> >\n> >> > I guess this might not be too bad to handle. Value fields like\n> >> > timestamp_t are OK to overwrite. Pointers to objects likewise, since the\n> >> > memory is owned elsewhere. If we see existing parent pointers in an\n> >> > object we're parsing, we could probably free them under the assumption\n> >> > they're leftover cruft. Likewise for the \"tag\" field of \"struct tag\",\n> >> > which is owned by the struct and should be freed.\n> >> \n> >> Yeah, or clear them before returning with .corrupt bit set?\n> >\n> > This was my attempt to avoid dealing with a .corrupt bit. :)\n> \n> Then, clear them before returning with .parsed bit clear?\n\nBut then somebody calling parse_commit() and getting an error return\ndoesn't get to see the full extent of what we managed to parse. Which I\nthought was one of your original points: people should be able to get\nwhatever information we can get out of even a corrupted or malformed\nobject.\n\nIf we keep that requirement, then we have to either clear them at the\nstart of the re-parsing step, or we have to avoid re-parsing entirely by\nusing an extra bit to say \"parsed but had an error\".  I've sent messy\n\"something like this\" patches for both strategies.\n\nI think the latter is cleaner conceptually, but the former doesn't\nrequire giving up a flag bit.\n\n-Peff\n"}]}