git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Sohom Datta via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Jeff King <peff@peff.net>,
	Sohom Datta <sohom.datta@learner.manipal.edu>,
	Sohom Datta <sohom.datta@learner.manipal.edu>
Subject: [PATCH v2] Avoid segfault and provide fallback when cloning invalid branch/tag
Date: Wed, 04 Nov 2020 17:21:25 +0000	[thread overview]
Message-ID: <pull.906.v2.git.git.1604510485432.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.906.git.git.1604058401991.gitgitgadget@gmail.com>

From: Sohom Datta <sohom.datta@learner.manipal.edu>

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/20191103180716.GA72007@carpenter.lan/

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

Signed-off-by: Sohom Datta <sohom.datta@learner.manipal.edu>
---
    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

  parent reply	other threads:[~2020-11-04 17:21 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-30 11:46 [PATCH] Fix potential segfault on cloning invalid tag Sohom Datta via GitGitGadget
2020-10-30 15:09 ` Jeff King
2020-11-04 17:21 ` Sohom Datta via GitGitGadget [this message]
2020-11-04 19:31   ` [PATCH v2] Avoid segfault and provide fallback when cloning invalid branch/tag Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.906.v2.git.git.1604510485432.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=sohom.datta@learner.manipal.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).