Fix potential segfault on cloning invalid tag

Commit Message

Sohom Datta Oct. 30, 2020, 11:46 a.m. UTC

From: Sohom Datta <[e-mail]>

Git allows users to create tags pointing to object hashes
that may or may not be commits. When a tag that doesn't
point to a commit is used with the -b (--branch) parameter
of git clone, git segfaults as it assumes that the tag will
always reference a commit.

Add a check to make sure that lookup_commit_reference returns a commit
before detaching HEAD.

Signed-off-by: Sohom Datta <[e-mail]>
---
    Fix potential segfault on cloning invalid tag
    
    The bug can be reproduced by running git tag 1.4.0 $(git rev-parse
    :filename) on the parent repository and then cloning the repo using git
    clone --branch 1.4.0 file://path/to/repo. The output should be something
    along the lines of:
    
    Cloning into '<path/to/repo>'...
    remote: Enumerating objects: 8, done.
    remote: Counting objects: 100% (8/8), done.
    remote: Compressing objects: 100% (5/5), done.
    remote: Total 8 (delta 1), reused 0 (delta 0), pack-reused 0
    Receiving objects: 100% (8/8), done.
    Resolving deltas: 100% (1/1), done.
    error: object d670460b4b4aece5915caf5c68d12f560a9fe3e4 is a blob, not a commit
    zsh: segmentation fault (core dumped)

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

 builtin/clone.c           | 2 ++
 t/t5610-clone-detached.sh | 5 +++++
 2 files changed, 7 insertions(+)


base-commit: ad27df6a5cff694add500ab8c7f97234feb4a91f

Comments

Jeff King Oct. 30, 2020, 3:09 p.m. UTC | #1

On Fri, Oct 30, 2020 at 11:46:41AM +0000, Sohom Datta via GitGitGadget wrote:

> From: Sohom Datta <[e-mail]>
> 
> Git allows users to create tags pointing to object hashes
> that may or may not be commits. When a tag that doesn't
> point to a commit is used with the -b (--branch) parameter
> of git clone, git segfaults as it assumes that the tag will
> always reference a commit.
> 
> Add a check to make sure that lookup_commit_reference returns a commit
> before detaching HEAD.

This seemed eerily familiar, so I dug up these threads:

  - https://lore.kernel.org/git/[e-mail]/

  - https://lore.kernel.org/git/[e-mail]/

The interesting things are:

  - there are some other cases where we might see a non-commit, which
    are also worth covering

  - it would be friendlier to the user to put in a fallback value for
    HEAD. We already transferred the whole history, so deleting it is a
    bit less friendly than leaving it in a state the user can recover
    from locally

I think the patches there were moving in a good direction, but it looks
like they just got stalled.

-Peff

Patch

diff --git a/builtin/clone.c b/builtin/clone.c
index a0841923cf..b4760ac887 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -727,6 +727,8 @@  static void update_head(const struct ref *our, const struct ref *remote,
 	} else if (our) {
 		struct commit *c = lookup_commit_reference(the_repository,
 							   &our->old_oid);
+		if ( !c )
+			die(_("%s does not point to a commit."), our->name);
 		/* --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);
diff --git a/t/t5610-clone-detached.sh b/t/t5610-clone-detached.sh
index 8b0d607df1..c7fd2c5f5c 100755
--- a/t/t5610-clone-detached.sh
+++ b/t/t5610-clone-detached.sh
@@ -15,6 +15,7 @@  test_expect_success 'setup' '
 	echo two >file &&
 	git commit -a -m two &&
 	git tag two &&
+	git tag four $(git rev-parse :file) &&
 	echo three >file &&
 	git commit -a -m three
 '
@@ -72,5 +73,9 @@  test_expect_success 'cloned HEAD matches' '
 test_expect_success 'cloned HEAD is detached' '
 	head_is_detached detached-orphan
 '
+test_expect_success 'cloning invalid tag' '
+	test_must_fail git clone "file://$PWD" -b four 2>err &&
+	test_i18ngrep "does not point to a commit." err
+'
 
 test_done