{"thread":{"id":"22002","subject":"[PATCH] Prevent git blame from segfaulting on a missing author name","startedAt":"2009-12-22T04:22:43Z","lastAt":"2009-12-22T07:25:15Z","messageCount":2,"participants":["David Reiss","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"130231","messageId":"4B304993.2040600@facebook.com","threadId":"22002","inReplyTo":null,"subject":"[PATCH] Prevent git blame from segfaulting on a missing author name","fromName":"David Reiss","fromEmail":"dreiss@facebook.com","sentAt":"2009-12-22T04:22:43Z","receivedAt":"2009-12-22T04:22:43Z","isPatch":true,"sender":{"key":"dreiss@facebook.com","avatar":null},"body":"The author name should never be missing in a valid commit, but\ngit shouldn't segfault no matter what is in the object database.\n\nSigned-off-by: David Reiss <dreiss@facebook.com>\n---\ngit blame was segfaulting on a repro produced by piping mtn git_export\nfrom the Pidgin repository to git fast-import.  This was the most obvious\nfix, but I'm not sure if it is the best solution.\n\nHere's a script that reproduces the segfault.\n\n#!/bin/sh\nset -e\ngit init\necho line > afile\ngit add afile\nTREE=`git write-tree`\ncat >badcommit <<EOF\ntree $TREE\nauthor <noname> 1234567890 +0000\ncommitter David Reiss <dreiss@facebook.com> 1234567890 +0000\n\nsome message\nEOF\nCOMMIT=`git hash-object -t commit -w badcommit`\necho \"git --no-pager blame $COMMIT -- afile\"\ngit --no-pager blame $COMMIT -- afile\n\n\n builtin-blame.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex d4e25a5..5e19c79 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1326,7 +1326,7 @@ static void get_ac_line(const char *inbuf, const char *what,\n \ttimepos = tmp;\n \n \t*tmp = 0;\n-\twhile (*tmp != ' ')\n+\twhile (tmp > person && *tmp != ' ')\n \t\ttmp--;\n \tmailpos = tmp + 1;\n \t*tmp = 0;\n-- \n1.6.3.3\n"},{"id":"130232","messageId":"7viqbz4gis.fsf@alter.siamese.dyndns.org","threadId":"22002","inReplyTo":"4B304993.2040600@facebook.com","subject":"Re: [PATCH] Prevent git blame from segfaulting on a missing author name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-22T07:25:15Z","receivedAt":"2009-12-22T07:25:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Reiss <dreiss@facebook.com> writes:\n\n> The author name should never be missing in a valid commit, but\n> git shouldn't segfault no matter what is in the object database.\n>\n> Signed-off-by: David Reiss <dreiss@facebook.com>\n> ---\n> git blame was segfaulting on a repro produced by piping mtn git_export\n> from the Pidgin repository to git fast-import.  This was the most obvious\n> fix, but I'm not sure if it is the best solution.\n\nThanks.\n\nWhile it is _unusual_ not to have a human readable name, if the commits\ncome from foreign systems (e.g. CVS/SVN), there often are not sufficient\ninformation in the source to fabricate names, so we should tolerate them.\n\nWe might want to also teach fast-import to warn when asked (i.e. when we\nare feeding from a foreign interface that is designed to read from a\nsource that is capable of recording real names), but we shouldn't prevent\nit from creating such a commit.\n\n> Here's a script that reproduces the segfault.\n\nPlease make that into a new test in an existing test suite somewhere in t/\ndirectory.\n\n\nI think we probably should prepare ourselves to be fed with even more\nbroken commits, perhaps like this, if we are fixing it..\n\n builtin-blame.c |   13 ++++++++++---\n 1 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex d4e25a5..14830a3 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1305,6 +1305,7 @@ static void get_ac_line(const char *inbuf, const char *what,\n \terror_out:\n \t\t/* Ugh */\n \t\t*tz = \"(unknown)\";\n+\t\tstrcpy(person, *tz);\n \t\tstrcpy(mail, *tz);\n \t\t*time = 0;\n \t\treturn;\n@@ -1314,20 +1315,26 @@ static void get_ac_line(const char *inbuf, const char *what,\n \ttmp = person;\n \ttmp += len;\n \t*tmp = 0;\n-\twhile (*tmp != ' ')\n+\twhile (person < tmp && *tmp != ' ')\n \t\ttmp--;\n+\tif (tmp == person)\n+\t\tgoto error_out;\n \t*tz = tmp+1;\n \ttzlen = (person+len)-(tmp+1);\n \n \t*tmp = 0;\n-\twhile (*tmp != ' ')\n+\twhile (person < tmp && *tmp != ' ')\n \t\ttmp--;\n+\tif (tmp == person)\n+\t\tgoto error_out;\n \t*time = strtoul(tmp, NULL, 10);\n \ttimepos = tmp;\n \n \t*tmp = 0;\n-\twhile (*tmp != ' ')\n+\twhile (person < tmp && *tmp != ' ')\n \t\ttmp--;\n+\tif (tmp <= person)\n+\t\treturn;\n \tmailpos = tmp + 1;\n \t*tmp = 0;\n \tmaillen = timepos - tmp;\n"}]}