Merge pull request #805 from nulltoken/fix/revwalk-email-parsing

Fix revwalk email parsing

12 files changed, 130 insertions, 37 deletions

diff --git a/include/git2/signature.h b/include/git2/signature.h
index cbf94269..cdbe6787 100644
--- a/include/git2/signature.h
+++ b/include/git2/signature.h
@@ -23,6 +23,9 @@ GIT_BEGIN_DECL
  * Create a new action signature. The signature must be freed
  * manually or using git_signature_free
  *
+ * Note: angle brackets ('<' and '>') characters are not allowed
+ * to be used in either the `name` or the `email` parameter.
+ *
  * @param sig_out new signature, in case of error NULL
  * @param name name of the person
  * @param email email of the person
diff --git a/src/revwalk.c b/src/revwalk.c
index 7bcdc4af..9dff283f 100644
--- a/src/revwalk.c
+++ b/src/revwalk.c
@@ -188,7 +188,7 @@ static int commit_quick_parse(git_revwalk *walk, commit_object *commit, git_rawo
 	const size_t parent_len = strlen("parent ") + GIT_OID_HEXSZ + 1;
 	unsigned char *buffer = raw->data;
 	unsigned char *buffer_end = buffer + raw->len;
-	unsigned char *parents_start;
+	unsigned char *parents_start, *committer_start;
 	int i, parents = 0;
 	int commit_time;
 
@@ -219,17 +219,34 @@ static int commit_quick_parse(git_revwalk *walk, commit_object *commit, git_rawo
 
 	commit->out_degree = (unsigned short)parents;
 
+	if ((committer_start = buffer = memchr(buffer, '\n', buffer_end - buffer)) == NULL)
+		return commit_error(commit, "object is corrupted");
+
+	buffer++;
+
 	if ((buffer = memchr(buffer, '\n', buffer_end - buffer)) == NULL)
 		return commit_error(commit, "object is corrupted");
 
-	if ((buffer = memchr(buffer, '<', buffer_end - buffer)) == NULL ||
-		(buffer = memchr(buffer, '>', buffer_end - buffer)) == NULL)
-		return commit_error(commit, "malformed author information");
+	/* Skip trailing spaces */
+	while (buffer > committer_start && git__isspace(*buffer))
+		buffer--;
+
+	/* Seek for the begining of the pack of digits */
+	while (buffer > committer_start && git__isdigit(*buffer))
+		buffer--;
 
-	while (*buffer == '>' || git__isspace(*buffer))
-		buffer++;
+	/* Skip potential timezone offset */
+	if ((buffer > committer_start) && (*buffer == '+' || *buffer == '-')) {
+		buffer--;
+
+		while (buffer > committer_start && git__isspace(*buffer))
+			buffer--;
+
+		while (buffer > committer_start && git__isdigit(*buffer))
+			buffer--;
+	}
 
-	if (git__strtol32(&commit_time, (char *)buffer, NULL, 10) < 0)
+	if ((buffer == committer_start) || (git__strtol32(&commit_time, (char *)(buffer + 1), NULL, 10) < 0))
 		return commit_error(commit, "cannot parse commit time");
 
 	commit->time = (time_t)commit_time;
diff --git a/src/signature.c b/src/signature.c
index 332bdf65..1f788356 100644
--- a/src/signature.c
+++ b/src/signature.c
@@ -40,7 +40,7 @@ static const char *skip_trailing_spaces(const char *buffer_start, const char *bu
 
 static int signature_error(const char *msg)
 {
-	giterr_set(GITERR_INVALID, "Failed to parse signature - %s", msg);
+	giterr_set(GITERR_INVALID, "Failed to process signature - %s", msg);
 	return -1;
 }
 
@@ -72,9 +72,16 @@ static int process_trimming(const char *input, char **storage, const char *input
 	return 0;
 }
 
+static bool contains_angle_brackets(const char *input)
+{
+	if (strchr(input, '<') != NULL)
+		return true;
+
+	return strchr(input, '>') != NULL;
+}
+
 int git_signature_new(git_signature **sig_out, const char *name, const char *email, git_time_t time, int offset)
 {
-	int error;
 	git_signature *p = NULL;
 
 	assert(name && email);
@@ -84,11 +91,18 @@ int git_signature_new(git_signature **sig_out, const char *name, const char *ema
 	p = git__calloc(1, sizeof(git_signature));
 	GITERR_CHECK_ALLOC(p);
 
-	if ((error = process_trimming(name, &p->name, name + strlen(name), 1)) < 0 ||
-		(error = process_trimming(email, &p->email, email + strlen(email), 1)) < 0)
+	if (process_trimming(name, &p->name, name + strlen(name), 1) < 0 ||
+		process_trimming(email, &p->email, email + strlen(email), 1) < 0)
 	{
 		git_signature_free(p);
-		return error;
+		return -1;
+	}
+		
+	if (contains_angle_brackets(p->email) ||
+		contains_angle_brackets(p->name))
+	{
+		git_signature_free(p);
+		return signature_error("Neither `name` nor `email` should contain angle brackets chars.");
 	}
 
 	p->when.time = time;
diff --git a/tests-clar/commit/signature.c b/tests-clar/commit/signature.c
index 290b11fa..9364efb1 100644
--- a/tests-clar/commit/signature.c
+++ b/tests-clar/commit/signature.c
@@ -13,17 +13,39 @@ static int try_build_signature(const char *name, const char *email, git_time_t t
 	return error;
 }
 
+static void assert_name_and_email(
+	const char *expected_name,
+	const char *expected_email,
+	const char *name,
+	const char *email)
+{
+	git_signature *sign;
+
+	cl_git_pass(git_signature_new(&sign, name, email, 1234567890, 60));
+	cl_assert_equal_s(expected_name, sign->name);
+	cl_assert_equal_s(expected_email, sign->email);
+
+	git_signature_free(sign);
+}
 
-void test_commit_signature__create_trim(void)
+void test_commit_signature__leading_and_trailing_spaces_are_trimmed(void)
 {
-   // creating a signature trims leading and trailing spaces
-   git_signature *sign;
-	cl_git_pass(git_signature_new(&sign, "  nulltoken ", "   emeric.fermas@gmail.com     ", 1234567890, 60));
-	cl_assert(strcmp(sign->name, "nulltoken") == 0);
-	cl_assert(strcmp(sign->email, "emeric.fermas@gmail.com") == 0);
-	git_signature_free((git_signature *)sign);
+	assert_name_and_email("nulltoken", "emeric.fermas@gmail.com", "  nulltoken ", "   emeric.fermas@gmail.com     ");
 }
 
+void test_commit_signature__angle_brackets_in_names_are_not_supported(void)
+{
+	cl_git_fail(try_build_signature("<Phil Haack", "phil@haack", 1234567890, 60));
+	cl_git_fail(try_build_signature("Phil>Haack", "phil@haack", 1234567890, 60));
+	cl_git_fail(try_build_signature("<Phil Haack>", "phil@haack", 1234567890, 60));
+}
+
+void test_commit_signature__angle_brackets_in_email_are_not_supported(void)
+{
+	cl_git_fail(try_build_signature("Phil Haack", ">phil@haack", 1234567890, 60));
+	cl_git_fail(try_build_signature("Phil Haack", "phil@>haack", 1234567890, 60));
+	cl_git_fail(try_build_signature("Phil Haack", "<phil@haack>", 1234567890, 60));
+}
 
 void test_commit_signature__create_empties(void)
 {
@@ -39,21 +61,13 @@ void test_commit_signature__create_empties(void)
 void test_commit_signature__create_one_char(void)
 {
    // creating a one character signature
-	git_signature *sign;
-	cl_git_pass(git_signature_new(&sign, "x", "foo@bar.baz", 1234567890, 60));
-	cl_assert(strcmp(sign->name, "x") == 0);
-	cl_assert(strcmp(sign->email, "foo@bar.baz") == 0);
-	git_signature_free((git_signature *)sign);
+	assert_name_and_email("x", "foo@bar.baz", "x", "foo@bar.baz");
 }
 
 void test_commit_signature__create_two_char(void)
 {
    // creating a two character signature
-	git_signature *sign;
-	cl_git_pass(git_signature_new(&sign, "xx", "x@y.z", 1234567890, 60));
-	cl_assert(strcmp(sign->name, "xx") == 0);
-	cl_assert(strcmp(sign->email, "x@y.z") == 0);
-	git_signature_free((git_signature *)sign);
+	assert_name_and_email("xx", "foo@bar.baz", "xx", "foo@bar.baz");
 }
 
 void test_commit_signature__create_zero_char(void)
diff --git a/tests-clar/network/remotelocal.c b/tests-clar/network/remotelocal.c
index 41922975..5e20b424 100644
--- a/tests-clar/network/remotelocal.c
+++ b/tests-clar/network/remotelocal.c
@@ -107,7 +107,7 @@ void test_network_remotelocal__retrieve_advertised_references(void)
 
 	cl_git_pass(git_remote_ls(remote, &count_ref__cb, &how_many_refs));
 
-	cl_assert_equal_i(how_many_refs, 21);
+	cl_assert_equal_i(how_many_refs, 22);
 }
 
 void test_network_remotelocal__retrieve_advertised_references_from_spaced_repository(void)
@@ -121,7 +121,7 @@ void test_network_remotelocal__retrieve_advertised_references_from_spaced_reposi
 
 	cl_git_pass(git_remote_ls(remote, &count_ref__cb, &how_many_refs));
 
-	cl_assert_equal_i(how_many_refs, 21);
+	cl_assert_equal_i(how_many_refs, 22);
 
 	git_remote_free(remote);	/* Disconnect from the "spaced repo" before the cleanup */
 	remote = NULL;
diff --git a/tests-clar/refs/branches/foreach.c b/tests-clar/refs/branches/foreach.c
index 51e3381f..b6e97379 100644
--- a/tests-clar/refs/branches/foreach.c
+++ b/tests-clar/refs/branches/foreach.c
@@ -48,7 +48,7 @@ static void assert_retrieval(unsigned int flags, unsigned int expected_count)
 
 void test_refs_branches_foreach__retrieve_all_branches(void)
 {
-	assert_retrieval(GIT_BRANCH_LOCAL | GIT_BRANCH_REMOTE, 9);
+	assert_retrieval(GIT_BRANCH_LOCAL | GIT_BRANCH_REMOTE, 10);
 }
 
 void test_refs_branches_foreach__retrieve_remote_branches(void)
@@ -58,7 +58,7 @@ void test_refs_branches_foreach__retrieve_remote_branches(void)
 
 void test_refs_branches_foreach__retrieve_local_branches(void)
 {
-	assert_retrieval(GIT_BRANCH_LOCAL, 7);
+	assert_retrieval(GIT_BRANCH_LOCAL, 8);
 }
 
 struct expectations {
diff --git a/tests-clar/refs/foreachglob.c b/tests-clar/refs/foreachglob.c
index 8bbcd71e..d1412a94 100644
--- a/tests-clar/refs/foreachglob.c
+++ b/tests-clar/refs/foreachglob.c
@@ -46,7 +46,7 @@ static void assert_retrieval(const char *glob, unsigned int flags, int expected_
 void test_refs_foreachglob__retrieve_all_refs(void)
 {
 	/* 7 heads (including one packed head) + 1 note + 2 remotes + 6 tags */
-	assert_retrieval("*", GIT_REF_LISTALL, 16);
+	assert_retrieval("*", GIT_REF_LISTALL, 17);
 }
 
 void test_refs_foreachglob__retrieve_remote_branches(void)
@@ -56,7 +56,7 @@ void test_refs_foreachglob__retrieve_remote_branches(void)
 
 void test_refs_foreachglob__retrieve_local_branches(void)
 {
-	assert_retrieval("refs/heads/*", GIT_REF_LISTALL, 7);
+	assert_retrieval("refs/heads/*", GIT_REF_LISTALL, 8);
 }
 
 void test_refs_foreachglob__retrieve_partially_named_references(void)
diff --git a/tests-clar/resources/testrepo.git/objects/1b/8cbad43e867676df601306689fe7c3def5e689 b/tests-clar/resources/testrepo.git/objects/1b/8cbad43e867676df601306689fe7c3def5e689
new file mode 100644
index 00000000..6048d4ba
--- /dev/null
+++ b/tests-clar/resources/testrepo.git/objects/1b/8cbad43e867676df601306689fe7c3def5e689
diff --git a/tests-clar/resources/testrepo.git/objects/25/8f0e2a959a364e40ed6603d5d44fbb24765b10 b/tests-clar/resources/testrepo.git/objects/25/8f0e2a959a364e40ed6603d5d44fbb24765b10
new file mode 100644
index 00000000..cb1ed571
--- /dev/null
+++ b/tests-clar/resources/testrepo.git/objects/25/8f0e2a959a364e40ed6603d5d44fbb24765b10
diff --git a/tests-clar/resources/testrepo.git/refs/heads/haacked b/tests-clar/resources/testrepo.git/refs/heads/haacked
new file mode 100644
index 00000000..17f59122
--- /dev/null
+++ b/tests-clar/resources/testrepo.git/refs/heads/haacked
@@ -0,0 +1 @@
+258f0e2a959a364e40ed6603d5d44fbb24765b10
diff --git a/tests-clar/revwalk/basic.c b/tests-clar/revwalk/basic.c
index a5a9b2ed..6f3c1c06 100644
--- a/tests-clar/revwalk/basic.c
+++ b/tests-clar/revwalk/basic.c
@@ -129,8 +129,8 @@ void test_revwalk_basic__glob_heads(void)
 		i++;
 	}
 
-	/* git log --branches --oneline | wc -l => 13 */
-	cl_assert(i == 13);
+	/* git log --branches --oneline | wc -l => 14 */
+	cl_assert(i == 14);
 }
 
 void test_revwalk_basic__push_head(void)
diff --git a/tests-clar/revwalk/signatureparsing.c b/tests-clar/revwalk/signatureparsing.c
new file mode 100644
index 00000000..94de1a34
--- /dev/null
+++ b/tests-clar/revwalk/signatureparsing.c
@@ -0,0 +1,44 @@
+#include "clar_libgit2.h"
+
+static git_repository *_repo;
+static git_revwalk *_walk;
+
+void test_revwalk_signatureparsing__initialize(void)
+{
+	cl_git_pass(git_repository_open(&_repo, cl_fixture("testrepo.git")));
+	cl_git_pass(git_revwalk_new(&_walk, _repo));
+}
+
+void test_revwalk_signatureparsing__cleanup(void)
+{
+	git_revwalk_free(_walk);
+	git_repository_free(_repo);
+}
+
+void test_revwalk_signatureparsing__do_not_choke_when_name_contains_angle_brackets(void)
+{
+	git_reference *ref;
+	git_oid commit_oid;
+	git_commit *commit;
+	const git_signature *signature;
+
+	/*
+	 * The branch below points at a commit with angle brackets in the committer/author name
+	 * committer <Yu V. Bin Haacked> <foo@example.com> 1323847743 +0100
+	 */
+	cl_git_pass(git_reference_lookup(&ref, _repo, "refs/heads/haacked"));
+
+	git_revwalk_push(_walk, git_reference_oid(ref));
+	cl_git_pass(git_revwalk_next(&commit_oid, _walk));
+
+	cl_git_pass(git_commit_lookup(&commit, _repo, git_reference_oid(ref)));
+
+	signature = git_commit_committer(commit);
+	cl_assert_equal_s("Yu V. Bin Haacked", signature->email);
+	cl_assert_equal_s("", signature->name);
+	cl_assert_equal_i(1323847743, (int)signature->when.time);
+	cl_assert_equal_i(60, signature->when.offset);
+
+	git_commit_free(commit);
+	git_reference_free(ref);
+}