blame: do not decrement commit refcount in make_origin

When we create a blame origin, we try to look up the blob that is
to be blamed at a certain revision. When this lookup fails, e.g.
because the file did not exist at that certain revision, we fail
to create the blame origin and return `NULL`. The blame origin
that we have just allocated is thereby free'd with
`origin_decref`.

The `origin_decref` function does not only decrement reference
counts for the blame origin, though, but also for its commit and
blob. When this is done in the error case, we will cause an
uneven reference count for these objects. This may result in
hard-to-debug failures at seemingly unrelated code paths, where
we try to access these objects when they in fact have already
been free'd.

Fix the issue by refactoring `make_origin` such that we only
allocate the object after the only function that may fail so that
we do not have to call `origin_decref` at all. Also fix the
`pass_blame` function, which indirectly calls `make_origin`, to
free the commit when `make_origin` failed.

1 files changed, 18 insertions, 8 deletions

diff --git a/src/blame_git.c b/src/blame_git.c
index 700207edb..96785c75b 100644
--- a/src/blame_git.c
+++ b/src/blame_git.c
@@ -37,25 +37,27 @@ static void origin_decref(git_blame__origin *o)
 static int make_origin(git_blame__origin **out, git_commit *commit, const char *path)
 {
 	git_blame__origin *o;
+	git_object *blob;
 	size_t path_len = strlen(path), alloc_len;
 	int error = 0;
 
+	if ((error = git_object_lookup_bypath(&blob, (git_object*)commit,
+			path, GIT_OBJ_BLOB)) < 0)
+		return error;
+
 	GITERR_CHECK_ALLOC_ADD(&alloc_len, sizeof(*o), path_len);
 	GITERR_CHECK_ALLOC_ADD(&alloc_len, alloc_len, 1);
 	o = git__calloc(1, alloc_len);
 	GITERR_CHECK_ALLOC(o);
 
 	o->commit = commit;
+	o->blob = (git_blob *) blob;
 	o->refcnt = 1;
 	strcpy(o->path, path);
 
-	if (!(error = git_object_lookup_bypath((git_object**)&o->blob, (git_object*)commit,
-			path, GIT_OBJ_BLOB))) {
-		*out = o;
-	} else {
-		origin_decref(o);
-	}
-	return error;
+	*out = o;
+
+	return 0;
 }
 
 /* Locate an existing origin or create a new one. */
@@ -529,8 +531,16 @@ static int pass_blame(git_blame *blame, git_blame__origin *origin, uint32_t opt)
 			goto finish;
 		porigin = find_origin(blame, p, origin);
 
-		if (!porigin)
+		if (!porigin) {
+			/*
+			 * We only have to decrement the parent's
+			 * reference count when no porigin has
+			 * been created, as otherwise the commit
+			 * is assigned to the created object.
+			 */
+			git_commit_free(p);
 			continue;
+		}
 		if (porigin->blob && origin->blob &&
 		    !git_oid_cmp(git_blob_id(porigin->blob), git_blob_id(origin->blob))) {
 			pass_whole_blame(blame, origin, porigin);