{"thread":{"id":"11609","subject":"[PATCH 2/2] git-fsck: remove commit test already done by parse_commit_buffer","startedAt":"2008-01-13T18:11:09Z","lastAt":"2008-01-14T21:42:29Z","messageCount":7,"participants":["Martin Koegler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"65262","messageId":"12002478702664-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"11609","inReplyTo":null,"subject":"[PATCH 1/2] parse_commit_buffer: don't parse invalid commits","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-13T18:11:09Z","receivedAt":"2008-01-13T18:11:09Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n---\n\ncommit.c |   35 ++++++++++++++++++++++++-----------\n 1 files changed, 24 insertions(+), 11 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex f074811..5b52daa 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -48,22 +48,34 @@ struct commit *lookup_commit(const unsigned char *sha1)\n \treturn check_commit(obj, sha1, 0);\n }\n \n-static unsigned long parse_commit_date(const char *buf)\n+static int parse_commit_date(const char *buf, const char *tail, unsigned long *date)\n {\n-\tunsigned long date;\n+\tconst char *dateptr;\n \n+\tif (buf + 6 >= tail)\n+\t\treturn 0;\n \tif (memcmp(buf, \"author\", 6))\n \t\treturn 0;\n-\twhile (*buf++ != '\\n')\n+\twhile (buf < tail && *buf++ != '\\n')\n \t\t/* nada */;\n+\tif (buf + 9 >= tail)\n+\t\treturn 0;\n \tif (memcmp(buf, \"committer\", 9))\n \t\treturn 0;\n-\twhile (*buf++ != '>')\n+\twhile (buf < tail && *buf++ != '>')\n \t\t/* nada */;\n-\tdate = strtoul(buf, NULL, 10);\n-\tif (date == ULONG_MAX)\n-\t\tdate = 0;\n-\treturn date;\n+\tif (buf >= tail)\n+\t\treturn 0;\n+\tdateptr = buf;\n+\twhile (buf < tail && *buf++ != '\\n')\n+\t\t/* nada */;\n+\tif (buf >= tail)\n+\t\treturn 0;\n+\t/* dateptr < buf && buf[-1] == '\\n', so strtoul will stop at buf-1 */\n+\t*date = strtoul(dateptr, NULL, 10);\n+\tif (*date == ULONG_MAX)\n+\t\t*date = 0;\n+\treturn 1;\n }\n \n static struct commit_graft **commit_graft;\n@@ -236,9 +248,9 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n \t\treturn 0;\n \titem->object.parsed = 1;\n \ttail += size;\n-\tif (tail <= bufptr + 5 || memcmp(bufptr, \"tree \", 5))\n+\tif (tail <= bufptr + 46 || memcmp(bufptr, \"tree \", 5) || bufptr[45] != '\\n')\n \t\treturn error(\"bogus commit object %s\", sha1_to_hex(item->object.sha1));\n-\tif (tail <= bufptr + 45 || get_sha1_hex(bufptr + 5, parent) < 0)\n+\tif (get_sha1_hex(bufptr + 5, parent) < 0)\n \t\treturn error(\"bad tree pointer in commit %s\",\n \t\t\t     sha1_to_hex(item->object.sha1));\n \titem->tree = lookup_tree(parent);\n@@ -275,7 +287,8 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n \t\t\tn_refs++;\n \t\t}\n \t}\n-\titem->date = parse_commit_date(bufptr);\n+\tif (!parse_commit_date(bufptr, tail, &item->date))\n+\t\treturn error(\"bogus commit date in object %s\", sha1_to_hex(item->object.sha1));\n \n \tif (track_object_refs) {\n \t\tunsigned i = 0;\n-- \n1.4.4.4\n"},{"id":"65261","messageId":"1200247870151-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"11609","inReplyTo":"12002478702664-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"[PATCH 2/2] git-fsck: remove commit test already done by parse_commit_buffer","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-13T18:11:10Z","receivedAt":"2008-01-13T18:11:10Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n---\n builtin-fsck.c |   12 ------------\n 1 files changed, 0 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex e4874f6..a77d8c0 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -360,18 +360,6 @@ static int fsck_commit(struct commit *commit)\n \t\tfprintf(stderr, \"Checking commit %s\\n\",\n \t\t\tsha1_to_hex(commit->object.sha1));\n \n-\tif (memcmp(buffer, \"tree \", 5))\n-\t\treturn objerror(&commit->object, \"invalid format - expected 'tree' line\");\n-\tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n-\t\treturn objerror(&commit->object, \"invalid 'tree' line format - bad sha1\");\n-\tbuffer += 46;\n-\twhile (!memcmp(buffer, \"parent \", 7)) {\n-\t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n-\t\t\treturn objerror(&commit->object, \"invalid 'parent' line format - bad sha1\");\n-\t\tbuffer += 48;\n-\t}\n-\tif (memcmp(buffer, \"author \", 7))\n-\t\treturn objerror(&commit->object, \"invalid format - expected 'author' line\");\n \tfree(commit->buffer);\n \tcommit->buffer = NULL;\n \tif (!commit->tree)\n-- \n1.4.4.4\n"},{"id":"65282","messageId":"7vzlv9nwdj.fsf@gitster.siamese.dyndns.org","threadId":"11609","inReplyTo":"12002478702664-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"Re: [PATCH 1/2] parse_commit_buffer: don't parse invalid commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-13T23:23:04Z","receivedAt":"2008-01-13T23:23:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I think I saw this patch and vaguely recall commenting on it,\nbut your message is not very helpful locating the previous\nthread to see what kind of improvements are made to this new\nround.\n"},{"id":"65301","messageId":"20080114064654.GA1229@auto.tuwien.ac.at","threadId":"11609","inReplyTo":"7vzlv9nwdj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] parse_commit_buffer: don't parse invalid commits","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-14T06:46:54Z","receivedAt":"2008-01-14T06:46:54Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Jan 13, 2008 at 03:23:04PM -0800, Junio C Hamano wrote:\n> I think I saw this patch and vaguely recall commenting on it,\n> but your message is not very helpful locating the previous\n> thread to see what kind of improvements are made to this new\n> round.\n\nhttp://marc.info/?l=git&m=119965688320973&w=2\n\nmfg Martin Kögler\n"},{"id":"65305","messageId":"7vk5mclvk3.fsf@gitster.siamese.dyndns.org","threadId":"11609","inReplyTo":"12002478702664-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"Re: [PATCH 1/2] parse_commit_buffer: don't parse invalid commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-14T07:23:40Z","receivedAt":"2008-01-14T07:23:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n\n> +\tif (!parse_commit_date(bufptr, tail, &item->date))\n> +\t\treturn error(\"bogus commit date in object %s\", sha1_to_hex(item->object.sha1));\n>  \n>  \tif (track_object_refs) {\n>  \t\tunsigned i = 0;\n\nI suspect this might be an undesirable regression.\n\nIf somebody managed to create a commit with a bogus \"author\"\nline and wanted to clean up the history, your previous one at\nleast gave something usable back, even though it had to come up\nwith a bogus date.  It gave the rest of the data back without\nbarfing.  And it was easy to see which \"resurrected\" commit had\na missing author date (bogus ones always gave 0 timestamp).\n\nThis round you made it to error out, and callers that check the\nreturn value of parse_commit() would stop traversing the\nhistory, even if the commit in question has perfectly valid\n\"parent \" lines, thinking \"ah, this commit object is faulty\".\nIt actively interferes with attempts to resurrect data from\nhistory that contains a faulty commit.\n\nYour previous version was much better with respect to this\nissue.  It was about being more careful not to read outside the\ncommit object buffer, while still allowing the data from a\nhistory that has an unfortunate commit with broken author line\nto be resurrected more easily.\n\nI do not think the checks done by fsck and parse_commit should\nshare the same strictness.  They serve different purposes.\n"},{"id":"65334","messageId":"20080114205826.GB5058@auto.tuwien.ac.at","threadId":"11609","inReplyTo":"7vk5mclvk3.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] parse_commit_buffer: don't parse invalid commits","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-14T20:58:26Z","receivedAt":"2008-01-14T20:58:26Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Jan 13, 2008 at 11:23:40PM -0800, Junio C Hamano wrote:\n> Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n> \n> > +\tif (!parse_commit_date(bufptr, tail, &item->date))\n> > +\t\treturn error(\"bogus commit date in object %s\", sha1_to_hex(item->object.sha1));\n> >  \n> >  \tif (track_object_refs) {\n> >  \t\tunsigned i = 0;\n>\n> I suspect this might be an undesirable regression.\n\nYou seem to have missed my reply to your last mail:\nhttp://marc.info/?l=git&m=119969163624138&w=2\n\nI asked you, what you would think about this change, but got no\nanswer. Anyway, now I know your opinion ;-)\n\n> If somebody managed to create a commit with a bogus \"author\"\n> line and wanted to clean up the history, your previous one at\n> least gave something usable back, even though it had to come up\n> with a bogus date.  It gave the rest of the data back without\n> barfing.  And it was easy to see which \"resurrected\" commit had\n> a missing author date (bogus ones always gave 0 timestamp).\n>\n> This round you made it to error out, and callers that check the\n> return value of parse_commit() would stop traversing the\n> history, even if the commit in question has perfectly valid\n> \"parent \" lines, thinking \"ah, this commit object is faulty\".\n> It actively interferes with attempts to resurrect data from\n> history that contains a faulty commit.\n\nOn the other hand, it is possible, that somebody pushed such a commit\nout, if he does not notice it. Then its difficult to get rid of the\nbroken commit. [Hasn't happend a broken commit on pu recently?]\n\nparse_commit_date is not very strict, so its likely, that it miss such\na commit. Commit parsing is too common function to slow it down with\nfurther checks.\n\n> Your previous version was much better with respect to this\n> issue.  It was about being more careful not to read outside the\n> commit object buffer, while still allowing the data from a\n> history that has an unfortunate commit with broken author line\n> to be resurrected more easily.\n\nI'll repost a version, which reverts this change.\n\n> I do not think the checks done by fsck and parse_commit should\n> share the same strictness.  They serve different purposes.\n\nMaybe I can improve fsck.\n\nmfg Martin Kögler\n"},{"id":"65339","messageId":"7vabn8jd8a.fsf@gitster.siamese.dyndns.org","threadId":"11609","inReplyTo":"20080114205826.GB5058@auto.tuwien.ac.at","subject":"Re: [PATCH 1/2] parse_commit_buffer: don't parse invalid commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-14T21:42:29Z","receivedAt":"2008-01-14T21:42:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n\n> You seem to have missed my reply to your last mail:\n> http://marc.info/?l=git&m=119969163624138&w=2\n\nYeah, apparently I did.\n\n> On the other hand, it is possible, that somebody pushed such a commit\n> out, if he does not notice it. Then its difficult to get rid of the\n> broken commit. [Hasn't happend a broken commit on pu recently?]\n\nThat's true, but I wonder if there is a reasonable middle\nground.  For example, we could keep the warning you added (by\ncalling error() to identify which commit object is suspicious)\nwithout making the whole function return that error.  Then a\nbroken commit could have been catched while reviewing the series\nbefore the commit gets pushed out.\n\n>> I do not think the checks done by fsck and parse_commit should\n>> share the same strictness.  They serve different purposes.\n>\n> Maybe I can improve fsck.\n\nThanks.  It should notice bogus commits but it should allow\nchecking objects referenced by them unless the particular\nbogosity is severe enough to make \"tree \" and \"parent \" lines in\nthem invalid.\n\nIOW, the information on the \"author\" line is not that important.\nIt has zero importance from the structural viewpoint, just like\nthe commit log message contents does not have any structural\nimportance (it does not even affect how the log output is sorted\nat all, while the timestamp on the \"committer\" line has some say\nin it).\n\nNoticing a bogus \"author\" or \"committer\" line is like noticing\nthat your commit log message has garbage characters in it.\nMaybe a future option to fsck might want to check that all log\nmessages are encoded in UTF-8, and you may catch some that are\nin ISO-8859-1.  After flagging them as such, the program should\nstill continue digging the history down to check its ancestors,\nas such a breakage does not affect the structural integrity of\nthe history.\n"}]}