[PATCH] Prevent git blame from segfaulting on a missing author name

[PATCH] Prevent git blame from segfaulting on a missing author name

[David Reiss]

The author name should never be missing in a valid commit, but
git shouldn't segfault no matter what is in the object database.
(Most of the C code was written by Junio.)

Signed-off-by: David Reiss <dreiss@facebook.com>
---
 builtin-blame.c  |   13 ++++++++++---
 t/t8003-blame.sh |   13 +++++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index d4e25a5..14830a3 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1305,6 +1305,7 @@ static void get_ac_line(const char *inbuf, const char *what,
 	error_out:
 		/* Ugh */
 		*tz = "(unknown)";
+		strcpy(person, *tz);
 		strcpy(mail, *tz);
 		*time = 0;
 		return;
@@ -1314,20 +1315,26 @@ static void get_ac_line(const char *inbuf, const char *what,
 	tmp = person;
 	tmp += len;
 	*tmp = 0;
-	while (*tmp != ' ')
+	while (person < tmp && *tmp != ' ')
 		tmp--;
+	if (tmp == person)
+		goto error_out;
 	*tz = tmp+1;
 	tzlen = (person+len)-(tmp+1);
 
 	*tmp = 0;
-	while (*tmp != ' ')
+	while (person < tmp && *tmp != ' ')
 		tmp--;
+	if (tmp == person)
+		goto error_out;
 	*time = strtoul(tmp, NULL, 10);
 	timepos = tmp;
 
 	*tmp = 0;
-	while (*tmp != ' ')
+	while (person < tmp && *tmp != ' ')
 		tmp--;
+	if (tmp <= person)
+		return;
 	mailpos = tmp + 1;
 	*tmp = 0;
 	maillen = timepos - tmp;
diff --git a/t/t8003-blame.sh b/t/t8003-blame.sh
index 13c25f1..ad834f2 100755
--- a/t/t8003-blame.sh
+++ b/t/t8003-blame.sh
@@ -144,4 +144,17 @@ test_expect_success 'blame path that used to be a directory' '
 	git blame HEAD^.. -- path
 '
 
+test_expect_success 'blame to a commit with no author name' '
+  TREE=`git rev-parse HEAD:`
+  cat >badcommit <<EOF
+tree $TREE
+author <noname> 1234567890 +0000
+committer David Reiss <dreiss@facebook.com> 1234567890 +0000
+
+some message
+EOF
+  COMMIT=`git hash-object -t commit -w badcommit`
+  git --no-pager blame $COMMIT -- uno >/dev/null
+'
+
 test_done
-- 
1.6.3.3

[PATCH] Prevent git blame from segfaulting on a missing author name

[David Reiss]

The author name should never be missing in a valid commit, but
git shouldn't segfault no matter what is in the object database.

Signed-off-by: David Reiss <dreiss@facebook.com>
---
git blame was segfaulting on a repro produced by piping mtn git_export
from the Pidgin repository to git fast-import.  This was the most obvious
fix, but I'm not sure if it is the best solution.

Here's a script that reproduces the segfault.

#!/bin/sh
set -e
git init
echo line > afile
git add afile
TREE=`git write-tree`
cat >badcommit <<EOF
tree $TREE
author <noname> 1234567890 +0000
committer David Reiss <dreiss@facebook.com> 1234567890 +0000

some message
EOF
COMMIT=`git hash-object -t commit -w badcommit`
echo "git --no-pager blame $COMMIT -- afile"
git --no-pager blame $COMMIT -- afile


 builtin-blame.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index d4e25a5..5e19c79 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1326,7 +1326,7 @@ static void get_ac_line(const char *inbuf, const char *what,
 	timepos = tmp;
 
 	*tmp = 0;
-	while (*tmp != ' ')
+	while (tmp > person && *tmp != ' ')
 		tmp--;
 	mailpos = tmp + 1;
 	*tmp = 0;
-- 
1.6.3.3

Re: [PATCH] Prevent git blame from segfaulting on a missing author name

[Junio C Hamano]

David Reiss <dreiss@facebook.com> writes:

> The author name should never be missing in a valid commit, but
> git shouldn't segfault no matter what is in the object database.
>
> Signed-off-by: David Reiss <dreiss@facebook.com>
> ---
> git blame was segfaulting on a repro produced by piping mtn git_export
> from the Pidgin repository to git fast-import.  This was the most obvious
> fix, but I'm not sure if it is the best solution.

Thanks.

While it is _unusual_ not to have a human readable name, if the commits
come from foreign systems (e.g. CVS/SVN), there often are not sufficient
information in the source to fabricate names, so we should tolerate them.

We might want to also teach fast-import to warn when asked (i.e. when we
are feeding from a foreign interface that is designed to read from a
source that is capable of recording real names), but we shouldn't prevent
it from creating such a commit.

> Here's a script that reproduces the segfault.

Please make that into a new test in an existing test suite somewhere in t/
directory.


I think we probably should prepare ourselves to be fed with even more
broken commits, perhaps like this, if we are fixing it..

 builtin-blame.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index d4e25a5..14830a3 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1305,6 +1305,7 @@ static void get_ac_line(const char *inbuf, const char *what,
 	error_out:
 		/* Ugh */
 		*tz = "(unknown)";
+		strcpy(person, *tz);
 		strcpy(mail, *tz);
 		*time = 0;
 		return;
@@ -1314,20 +1315,26 @@ static void get_ac_line(const char *inbuf, const char *what,
 	tmp = person;
 	tmp += len;
 	*tmp = 0;
-	while (*tmp != ' ')
+	while (person < tmp && *tmp != ' ')
 		tmp--;
+	if (tmp == person)
+		goto error_out;
 	*tz = tmp+1;
 	tzlen = (person+len)-(tmp+1);
 
 	*tmp = 0;
-	while (*tmp != ' ')
+	while (person < tmp && *tmp != ' ')
 		tmp--;
+	if (tmp == person)
+		goto error_out;
 	*time = strtoul(tmp, NULL, 10);
 	timepos = tmp;
 
 	*tmp = 0;
-	while (*tmp != ' ')
+	while (person < tmp && *tmp != ' ')
 		tmp--;
+	if (tmp <= person)
+		return;
 	mailpos = tmp + 1;
 	*tmp = 0;
 	maillen = timepos - tmp;