Subject: [PATCH v2] Avoid segfault and provide fallback when cloning invalid branch/tag

From: "Sohom Datta via GitGitGadget" <[email-masked]>
Date: Wed, 04 Nov 2020 17:21:25 +0000

From: Sohom Datta <[email-masked]>

The Git command to clone a specific branch of a git repository always
assumes that the commit pointed to by the branch are going to valid. It
thus segfaults and crashes whenever a tag or a branch with a invalid
commit is specified.

Aborting the operation is not desirable since the user is left with no
usable git folder to work with despite git having done most of the work
in setting everything up.

This commit is based of David Berardi's commit at
https://lore.kernel.org/git/[email-masked]/

Make git fallback on creating a unborn master branch when encountering
a invalid commit.

Signed-off-by: Sohom Datta <[email-masked]>
---
    Fix potential segfault on cloning invalid tag
    
    Changes since v1:
    
     * Reworked the patch to use the fallback approach based on feedback
       from Jeff King.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-906%2Fsohomdatta1%2Fsegfault-while-cloning-invalid-tag-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-906/sohomdatta1/segfault-while-cloning-invalid-tag-v2
Pull-Request: https://github.com/git/git/pull/906

Range-diff vs v1:

 1:  576b6049b2 < -:  ---------- Fix potential segfault on cloning invalid tag
 -:  ---------- > 1:  02b50572ff Avoid segfault and provide fallback when cloning invalid branch/tag

 builtin/clone.c         | 69 ++++++++++++++++++++++++++++++++++++++---
 t/t5609-clone-branch.sh | 15 +++++++++
 2 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index a0841923cf..53930f7536 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -711,10 +711,63 @@ static void update_remote_refs(const struct ref *refs,
 	}
 }
 
-static void update_head(const struct ref *our, const struct ref *remote,
+static struct commit * commit_lookup_helper (const struct ref *our,
+					      const struct ref *remote,
+					      const char *msg,
+					      int *err)
+{
+	const struct object_id *tip = NULL;
+	struct commit *tip_commit =  NULL;
+
+	if (our)
+		tip = &our->old_oid;
+	else if (remote)
+		tip = &remote->old_oid;
+	else {
+		/*
+		 * We have no local branch requested with "-b", and the
+		 * remote HEAD is unborn. There's nothing to update HEAD
+		 * to, but this state is not an error.
+		 */
+		return NULL;
+	}
+
+	if ( !lookup_object(the_repository, tip)) {
+		/**
+		 * We have a object id associated with the tip of the branch
+		 * but the object id doesn't resolve to a object. This will be
+		 * handled downstream in update_ref().
+		 */
+		return NULL;
+	}
+
+	tip_commit = lookup_commit_reference_gently(the_repository, tip, 1);
+	if (!tip_commit) {
+		/*
+		 * The given non-commit cannot be checked out,
+		 * so have a 'master' branch and leave it unborn.
+		 */
+		error(_("non-commit branch cannot be checked out."));
+		create_symref("HEAD", "refs/heads/master" ,msg);
+		*err = -1;
+		return NULL;
+	}
+
+	return tip_commit;
+}
+
+static int update_head(const struct ref *our,
+			const struct ref *remote,
 			const char *msg)
 {
 	const char *head;
+	int err = 0;
+	const struct commit * c;
+	c = commit_lookup_helper(our, remote, msg, &err);
+	if (err < 0) {
+		return -1;
+	}
+
 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
 		/* Local default branch link */
 		if (create_symref("HEAD", our->name, NULL) < 0)
@@ -725,8 +778,6 @@ static void update_head(const struct ref *our, const struct ref *remote,
 			install_branch_config(0, head, remote_name, our->name);
 		}
 	} else if (our) {
-		struct commit *c = lookup_commit_reference(the_repository,
-							   &our->old_oid);
 		/* --branch specifies a non-branch (i.e. tags), detach HEAD */
 		update_ref(msg, "HEAD", &c->object.oid, NULL, REF_NO_DEREF,
 			   UPDATE_REFS_DIE_ON_ERR);
@@ -739,6 +790,8 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		update_ref(msg, "HEAD", &remote->old_oid, NULL, REF_NO_DEREF,
 			   UPDATE_REFS_DIE_ON_ERR);
 	}
+
+	return 0;
 }
 
 static int git_sparse_checkout_init(const char *repo)
@@ -1346,7 +1399,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			   branch_top.buf, reflog_msg.buf, transport,
 			   !is_local);
 
-	update_head(our_head_points_at, remote_head, reflog_msg.buf);
+	err = update_head(our_head_points_at, remote_head, reflog_msg.buf);
 
 	/*
 	 * We want to show progress for recursive submodule clones iff
@@ -1365,7 +1418,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 
 	junk_mode = JUNK_LEAVE_REPO;
-	err = checkout(submodule_progress);
+	if ( err == 0 ) {
+		/*
+		 * Only try to checkout if we successfully updated HEAD; otherwise
+		 * HEAD isn't pointing to the thing the user wanted.
+		 */
+		err = checkout(submodule_progress);
+	}
 
 	free(remote_name);
 	strbuf_release(&reflog_msg);
diff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh
index 6e7a7be052..a589db9aa0 100755
--- a/t/t5609-clone-branch.sh
+++ b/t/t5609-clone-branch.sh
@@ -20,6 +20,9 @@ test_expect_success 'setup' '
 	 echo one >file && git add file && git commit -m one &&
 	 git checkout -b two &&
 	 echo two >file && git add file && git commit -m two &&
+	 blob=$(git rev-parse HEAD:file) &&
+	 echo $blob > .git/refs/heads/broken-tag &&
+	 echo $blob > .git/refs/heads/broken-head &&
 	 git checkout master) &&
 	mkdir empty &&
 	(cd empty && git init)
@@ -67,4 +70,16 @@ test_expect_success 'clone -b not allowed with empty repos' '
 	test_must_fail git clone -b branch empty clone-branch-empty
 '
 
+test_expect_success 'cloning -b for invalid tag must fail and fallback on remote head' '
+	test_must_fail git clone -b broken-tag parent broken-tag 2>error &&
+	test_i18ngrep "non-commit branch cannot be checked out." error &&
+	(cd broken-tag && check_HEAD master)
+'
+
+test_expect_success 'cloning -b for broken head must fail and fallback on remote head' '
+	test_must_fail git clone -b broken-head parent broken-head &&
+	test_i18ngrep "non-commit branch cannot be checked out." error &&
+	(cd broken-head && check_HEAD master)
+'
+
 test_done

base-commit: 7f7ebe054af6d831b999d6c2241b9227c4e4e08d
-- 
gitgitgadget