[PATCH] Segmentation Fault on non-commit --branch clone

[PATCH] Segmentation Fault on non-commit --branch clone

[Davide Berardi]

Fixed segmentation fault that can be triggered using
$ git clone --branch $object $repository
with object pointing to a non-commit ref (e.g. a blob).

Signed-off-by: Davide Berardi <berardi.dav@gmail.com>

---
 builtin/clone.c         | 26 ++++++++++++++++++++++++++
 refs.h                  |  7 +++++++
 t/t5609-clone-branch.sh | 22 +++++++++++++++++++++-
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f665b28ccc..0f4a18302c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -704,11 +704,32 @@ static void update_remote_refs(const struct ref *refs,
 	}
 }
 
+static int fallback_on_noncommit(const struct ref *check,
+				 const struct ref *remote,
+				 const char *msg)
+{
+	if (check == NULL)
+		return 1;
+	struct commit *c = lookup_commit_reference_gently(the_repository,
+						   &check->old_oid, 1);
+	if (c == NULL) {
+		/* Fallback HEAD to fallback refs */
+		warning(_("%s is not a valid commit object, HEAD will fallback to %s"),
+			check->name, FALLBACK_REF);
+		update_ref(msg, FALLBACK_REF, &remote->old_oid, NULL,
+			   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);
+	}
+
+	return c == NULL;
+}
+
 static void update_head(const struct ref *our, const struct ref *remote,
 			const char *msg)
 {
 	const char *head;
 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
+		if (fallback_on_noncommit(our, remote, msg) != 0)
+			return;
 		/* Local default branch link */
 		if (create_symref("HEAD", our->name, NULL) < 0)
 			die(_("unable to update HEAD"));
@@ -718,12 +739,17 @@ static void update_head(const struct ref *our, const struct ref *remote,
 			install_branch_config(0, head, option_origin, our->name);
 		}
 	} else if (our) {
+		if (fallback_on_noncommit(our, remote, msg) != 0)
+			return;
+		/* here commit is valid */
 		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);
 	} else if (remote) {
+		if (fallback_on_noncommit(remote, remote, msg) != 0)
+			return;
 		/*
 		 * We know remote HEAD points to a non-branch, or
 		 * HEAD points to a branch but we don't know which one.
diff --git a/refs.h b/refs.h
index 730d05ad91..35ee6eb058 100644
--- a/refs.h
+++ b/refs.h
@@ -497,6 +497,13 @@ enum action_on_err {
 	UPDATE_REFS_QUIET_ON_ERR
 };
 
+/*
+ * In case of a remote HEAD pointing to a non-commit update_head
+ * will make HEAD reference fallback to this value, master ref
+ * should be safe.
+ */
+#define FALLBACK_REF "refs/heads/master"
+
 /*
  * Begin a reference transaction.  The reference transaction must
  * be freed by calling ref_transaction_free().
diff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh
index 6e7a7be052..0680cf5a89 100755
--- a/t/t5609-clone-branch.sh
+++ b/t/t5609-clone-branch.sh
@@ -20,7 +20,13 @@ 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 &&
-	 git checkout master) &&
+	 git checkout master &&
+	 # Create dummy objects
+	 _B=$(git rev-list --objects --all | grep -m 1 "^[^ ]\+ [^ ]\+" | \
+	      awk "{print \$1}") &&
+	 echo "${_B}" >> .git/refs/tags/broken-tag &&
+	 echo "${_B}" >> .git/refs/heads/broken-head
+	) &&
 	mkdir empty &&
 	(cd empty && git init)
 '
@@ -67,4 +73,18 @@ test_expect_success 'clone -b not allowed with empty repos' '
 	test_must_fail git clone -b branch empty clone-branch-empty
 '
 
+test_expect_success 'clone -b with broken tag will fallback to master' '
+	git clone -b broken-tag parent clone-broken-tag &&
+	(cd clone-broken-tag &&
+	 check_HEAD master
+	)
+'
+
+test_expect_success 'clone -b with broken head will fallback to master' '
+	git clone -b broken-head parent clone-broken-head &&
+	(cd clone-broken-head &&
+	 check_HEAD master
+	)
+'
+
 test_done
-- 
2.22.0

Re: [PATCH] Segmentation Fault on non-commit --branch clone

[Johannes Schindelin]

Hi Davide,

I wonder whether you might want to reword the Subject: such that it
reflects what the patch does instead of what the problem is that
motivated you to work on the patch (the patch does not cause the
segmentation fault, after all, but it fixes it).

On Fri, 1 Nov 2019, Davide Berardi wrote:

> Fixed segmentation fault that can be triggered using
> $ git clone --branch $object $repository
> with object pointing to a non-commit ref (e.g. a blob).
>
> Signed-off-by: Davide Berardi <berardi.dav@gmail.com>
>
> ---
> builtin/clone.c         | 26 ++++++++++++++++++++++++++
> refs.h                  |  7 +++++++
> t/t5609-clone-branch.sh | 22 +++++++++++++++++++++-
> 3 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index f665b28ccc..0f4a18302c 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -704,11 +704,32 @@ static void update_remote_refs(const struct ref *refs,
> 	}
> }
>
> +static int fallback_on_noncommit(const struct ref *check,
> +				 const struct ref *remote,
> +				 const char *msg)
> +{
> +	if (check == NULL)
> +		return 1;
> +	struct commit *c = lookup_commit_reference_gently(the_repository,
> +						   &check->old_oid, 1);
> +	if (c == NULL) {
> +		/* Fallback HEAD to fallback refs */
> +		warning(_("%s is not a valid commit object, HEAD will fallback
> to %s"),
> +			check->name, FALLBACK_REF);

Quite honestly, I do not think that it is a good idea to fall back in
this case. The user asked for something that cannot be accomplished, and
the best way to handle this is to exit with an error, i.e. `die()`.

> +		update_ref(msg, FALLBACK_REF, &remote->old_oid, NULL,
> +			   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);
> +	}
> +
> +	return c == NULL;
> +}
> +
> static void update_head(const struct ref *our, const struct ref *remote,
> 			const char *msg)
> {
> 	const char *head;
> 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
> +		if (fallback_on_noncommit(our, remote, msg) != 0)
> +			return;
> 		/* Local default branch link */
> 		if (create_symref("HEAD", our->name, NULL) < 0)
> 			die(_("unable to update HEAD"));
> @@ -718,12 +739,17 @@ static void update_head(const struct ref *our, const
> struct ref *remote,
> 			install_branch_config(0, head, option_origin,
> 		our->name);
> 		}
> 	} else if (our) {
> +		if (fallback_on_noncommit(our, remote, msg) != 0)
> +			return;
> +		/* here commit is valid */
> 		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);
> 	} else if (remote) {
> +		if (fallback_on_noncommit(remote, remote, msg) != 0)
> +			return;
> 		/*
> 		 * We know remote HEAD points to a non-branch, or
> 		 * HEAD points to a branch but we don't know which one.
> diff --git a/refs.h b/refs.h
> index 730d05ad91..35ee6eb058 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -497,6 +497,13 @@ enum action_on_err {
> 	UPDATE_REFS_QUIET_ON_ERR
> };
>
> +/*
> + * In case of a remote HEAD pointing to a non-commit update_head
> + * will make HEAD reference fallback to this value, master ref
> + * should be safe.
> + */
> +#define FALLBACK_REF "refs/heads/master"
> +
> /*
>  * Begin a reference transaction.  The reference transaction must
>  * be freed by calling ref_transaction_free().
> diff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh
> index 6e7a7be052..0680cf5a89 100755
> --- a/t/t5609-clone-branch.sh
> +++ b/t/t5609-clone-branch.sh
> @@ -20,7 +20,13 @@ 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 &&
> -	 git checkout master) &&
> +	 git checkout master &&
> +	 # Create dummy objects
> +	 _B=$(git rev-list --objects --all | grep -m 1 "^[^ ]\+ [^ ]\+" | \

Hmm. That naming convention (`_B`) is a new one, and does not really
align with the existing code in the test suite, does it?

Further, it seems that you pick some semi-random object from the object
database. I think you can do better: if you want to get the hash of a
blob, use `git rev-parse HEAD:file`, if you want the hash of a tree, use
the same command with `HEAD:`, if you want a commit, with `HEAD`.

> +	      awk "{print \$1}") &&
> +	 echo "${_B}" >> .git/refs/tags/broken-tag &&

In the Git test suite, we only use curlies to expand shell variables
when necessary, i.e. when followed by a valid variable name character.

> +	 echo "${_B}" >> .git/refs/heads/broken-head
> +	) &&
> 	mkdir empty &&
> 	(cd empty && git init)
> '
> @@ -67,4 +73,18 @@ test_expect_success 'clone -b not allowed with empty repos'
> '
> 	test_must_fail git clone -b branch empty clone-branch-empty
> '
>
> +test_expect_success 'clone -b with broken tag will fallback to master' '
> +	git clone -b broken-tag parent clone-broken-tag &&

As I said earlier, I think this should fail, i.e. something like

	test_must_fail git clone ... 2>err &&
	test_i18ngrep [something-from-that-error-message] err

Thanks,
Johannes

> +	(cd clone-broken-tag &&
> +	 check_HEAD master
> +	)
> +'
> +
> +test_expect_success 'clone -b with broken head will fallback to master' '
> +	git clone -b broken-head parent clone-broken-head &&
> +	(cd clone-broken-head &&
> +	 check_HEAD master
> +	)
> +'
> +
> test_done
> --
> 2.22.0
>
>
>

Re: [PATCH] Segmentation Fault on non-commit --branch clone

[Jeff King]

On Fri, Nov 01, 2019 at 08:08:10PM +0100, Johannes Schindelin wrote:

> > +static int fallback_on_noncommit(const struct ref *check,
> > +				 const struct ref *remote,
> > +				 const char *msg)
> > +{
> > +	if (check == NULL)
> > +		return 1;
> > +	struct commit *c = lookup_commit_reference_gently(the_repository,
> > +						   &check->old_oid, 1);
> > +	if (c == NULL) {
> > +		/* Fallback HEAD to fallback refs */
> > +		warning(_("%s is not a valid commit object, HEAD will fallback
> > to %s"),
> > +			check->name, FALLBACK_REF);
> 
> Quite honestly, I do not think that it is a good idea to fall back in
> this case. The user asked for something that cannot be accomplished, and
> the best way to handle this is to exit with an error, i.e. `die()`.

The main reason I proposed falling back here is that the user can
correct the situation without having to redo the clone from scratch
(which might have been very expensive). And we cannot just leave HEAD
empty there; we have to put _something_ in it.

I do think it's important, though, that we don't just fall back; we
should still report an error exit from the program (just as we do for
the similar case when clone's checkout step fails). Otherwise something
as simple as:

  git clone -b $url repo &&
  cd repo &&
  do_something

could have quite unexpected results.

I don't know how often this would actually help users, though. It _is_ a
pretty rare situation to ask for a non-commit. So maybe it's all
over-engineering, and we should start with just die(). If somebody comes
along later and wants to enhance it, it should be pretty
straightforward.

-Peff

Re: [PATCH] Segmentation Fault on non-commit --branch clone

[Jeff King]

On Fri, Nov 01, 2019 at 01:24:32AM +0100, Davide Berardi wrote:

> Fixed segmentation fault that can be triggered using
> $ git clone --branch $object $repository
> with object pointing to a non-commit ref (e.g. a blob).

Thanks for working on this. I left some thoughts on the overall fallback
scheme in the other part of the thread, and other than I agree with all
of Dscho's review.

A few other comments:

> +static int fallback_on_noncommit(const struct ref *check,
> +				 const struct ref *remote,
> +				 const char *msg)
> +{
> +	if (check == NULL)
> +		return 1;

I wondered in what circumstances "check" would be NULL. In the first
conditional we pass "our" after checking it's non-NULL:

> 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
> +		if (fallback_on_noncommit(our, remote, msg) != 0)

and then again in the second case-arm:

> 	} else if (our) {
> +		if (fallback_on_noncommit(our, remote, msg) != 0)

and then in the third we pass remote after checking that it's not NULL:

> 	} else if (remote) {
> +		if (fallback_on_noncommit(remote, remote, msg) != 0)
> +			return;

So I think this NULL check can go away. In general I don't mind
functions being defensive, but it's hard to decide whether it's doing
the right thing since it's not a case we think can come up (it could be
marked with a BUG() assertion, but IMHO it's not worth it; most
functions require their arguments to be non-NULL, so checking it would
be unusual in our code base).

> +static int fallback_on_noncommit(const struct ref *check,
> +				 const struct ref *remote,
> +				 const char *msg)
> [...]
> +	return c == NULL;

The return value for this function is unusual for our code base. If it's
just an error return, we'd usually use "0" for success and a negative
value for errors (i.e., mimicking system calls).

> diff --git a/refs.h b/refs.h
> index 730d05ad91..35ee6eb058 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -497,6 +497,13 @@ enum action_on_err {
> 	UPDATE_REFS_QUIET_ON_ERR
> };
> 
> +/*
> + * In case of a remote HEAD pointing to a non-commit update_head
> + * will make HEAD reference fallback to this value, master ref
> + * should be safe.
> + */
> +#define FALLBACK_REF "refs/heads/master"
> +
> /*

Since this is only used in one spot, I think it's better to keep it
localized to that function. E.g., with:

  static const char fallback_ref[] = "refs/heads/master";

That way it's clear that no other code depends on it.

-Peff

Re: [PATCH] Segmentation Fault on non-commit --branch clone

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Davide,
>
> I wonder whether you might want to reword the Subject: such that it
> reflects what the patch does instead of what the problem is that
> motivated you to work on the patch (the patch does not cause the
> segmentation fault, after all, but it fixes it).

Good point.

    Subject: clone: do not segfault on "clone -b B" where B is a non-commit

    The code in "git clone -b B" to decide what branch to check out
    assumed that B points at a commit object without checking,
    leading to dereferencing a NULL pointer and causing a segfault.

    Just aborting the operation when it happens is not a very
    attractive option because we would be left with a directory
    without .git/HEAD that cannot be used as a valid repository the
    user can attempt to recover from the situation by checking out
    something.

    Fall back to use the 'master' branch, which is what we use when
    the command without the "-b" option cannot figure out what
    branch the remote side points with its HEAD.

or something like that, perhaps?

I am not sure if the existing code is careful enough setting up the
resulting local 'master' branch, or needs more changes associated
with this patch, though.  For example, does it want to be set to
integrate with the 'master' branch on the remote side by setting
"branch.master.remote" and "branch.master.merge" configuration, or
do we want to turn them off?  I _think_ the answer is that we want
to behave as if the user said "-b master" instead of "-b B" (with B
that does not point at a commit), but I am not sure.  Also, don't we
try to be a bit noisier when the fallback fails?  For example, if
the user said "clone -b master" and the 'master' points at an object
that is not a commit, falling back and writing refs/heads/master in
the HEAD would leave us in the same position as we did not have any
fallback.

I skimmed your review and I think I agree with most (if not all) of
them.  Thanks.

Re: [PATCH] Segmentation Fault on non-commit --branch clone

[Junio C Hamano]

Davide Berardi <berardi.dav@gmail.com> writes:

> +static int fallback_on_noncommit(const struct ref *check,
> +				 const struct ref *remote,
> +				 const char *msg)
> +{
> +	if (check == NULL)
> +		return 1;
> +	struct commit *c = lookup_commit_reference_gently(the_repository,
> +						   &check->old_oid, 1);

This is decl-after-stmt.  Can check be NULL, though?  IOW, the first
two lines in this function should go.

> +	if (c == NULL) {

We tend to say "if (!c) {" instead.

> +		/* Fallback HEAD to fallback refs */

You are falling back to just a single ref (i.e. s/refs/ref/) but
more importantly, what the code is doing is obvious enough without
this comment.  What we want commenting on is _why_ we do this.

	/*
	 * The ref specified to be checked out does not point at a
	 * commit so pointing HEAD at it will leave us a broken
         * repository.  But we need to have _something_ plausible
	 * in HEAD---otherwise the result would not be a repository.
	 */

would explain why we point HEAD to the default 'master' branch.

> +		warning(_("%s is not a valid commit object, HEAD will fallback to %s"),
> +			check->name, FALLBACK_REF);
> +		update_ref(msg, FALLBACK_REF, &remote->old_oid, NULL,
> +			   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);

Having said that, I was hoping that this would use the help from the
guess_remote_head() like the case without "-b" option does, so that
we do not have to use a hardcoded default.

> +	}
> +
> +	return c == NULL;

Our API convention is 0 for success and negavive for failure.

> +}
> +
> static void update_head(const struct ref *our, const struct ref *remote,
> 			const char *msg)
> {
> 	const char *head;
> 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
> +		if (fallback_on_noncommit(our, remote, msg) != 0)
> +			return;
> 		/* Local default branch link */
> 		if (create_symref("HEAD", our->name, NULL) < 0)
> 			die(_("unable to update HEAD"));
> @@ -718,12 +739,17 @@ static void update_head(const str

Here in the patch context is a "do this in a non-bare repository"
guard, and crucially, we do this:

> 			install_branch_config(0, head, option_origin, our->name);

That is, we add configuration for our->name (which is now
"refs/heads/master"), but I do not think we updated any of the other
field in *our to make the world a consistent place.  Is the object
pointed at by our local refs/heads/master in a reasonable
relationship with the object at the tip of the 'master' branch at
the remote site, or is can totally be unrelated because we made no
attempt to make our->old_oid or whatever field consistent with the
"corrected" reality?

Given the potential complication, and given that we are doing a
corretive action only so that we leave some repository the user can
fix manually, I am inclined to say that we should avoid falling into
this codepath.  I'll wonder about a totally different approach later
in this message that makes the fallback_on_noncommit() helper and
change to these existing parts of the update_head() function
unnecessary.

> 		}
> 	} else if (our) {
> +		if (fallback_on_noncommit(our, remote, msg) != 0)
> +			return;
> +		/* here commit is valid */
> 		struct commit *c = lookup_commit_reference(the_repository,
> 							   &our->old_oid);

What makes us certain that commit is valid?  our->old_oid is not
adjusted by the fallback_on_noncommit() function, but we did check
if it is a commit by doing the exact same lookup_commit_reference()
in there already, and we know it found a commit (otherwise the
function would have returned a non-zero to signal an error).

But it also means that we are making a redundant and unnecessary
call if the code is structured better.

This makes me wonder why we are not adding a single call to a helper
function at the beginning of the function, something like

	const struct oid *tip = NULL;
	struct commit *tip_commit = NULL;

	if (our)
		tip = &our->old_oid;
	else if (remote)
		tip = &remote->old_oid;

	if (!tip)
		return;

	tip_commit = lookup_commit_reference_gently(the_repository, tip);

Then, if !tip_commit, we know we need to fall back to something.  I
actually think it would probably be cleanest if we added

	if (!tip_commit) {
		/*
		 * The given non-commit cannot be checked out,
                 * so have a 'master' branch and leave it unborn.
                 */
		warning(_"non-commit cannot be checked out");
		create_symref("HEAD", "refs/heads/master", msg);
		return;
	}

That is, we always check out an unborn 'master' branch (which would
yield another warning at the beginning of checkout() function, which
is what we want) doing minimum damage, without even configuring any
remote tracking information.

The rest of the update_head() function could be left as-is, but with
the "see what we would be checking out first" approach, we probably
can lose some code (like the call to lookup_commit_reference() in
the "detached HEAD" case), without adding any extra logic.

> 	} else if (remote) {
> +		if (fallback_on_noncommit(remote, remote, msg) != 0)
> +			return;
> 		/*
> 		 * We know remote HEAD points to a non-branch, or
> 		 * HEAD points to a branch but we don't know which one.

Thanks.

Re: [PATCH] Segmentation Fault on non-commit --branch clone

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't know how often this would actually help users, though. It _is_ a
> pretty rare situation to ask for a non-commit. So maybe it's all
> over-engineering, and we should start with just die(). If somebody comes
> along later and wants to enhance it, it should be pretty
> straightforward.

I like that; after update_head() finishes, there are a few clean-up
things that the caller wants to do besides a checkout() call, but if
we make update_head() return a failure, perhaps the caller side
change would be as small as the attached patch.  That would go nicely
with the "make the result just barely usable" approach of leaving an
unborn master branch I suggested in a separate message, I would think.

 builtin/clone.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index c46ee29f0a..fa0558fa3e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1246,7 +1246,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			   branch_top.buf, reflog_msg.buf, transport,
 			   !is_local, filter_options.choice);
 
-	update_head(our_head_points_at, remote_head, reflog_msg.buf);
+	err = update_head(our_head_points_at, remote_head, reflog_msg.buf) < 0;
 
 	/*
 	 * We want to show progress for recursive submodule clones iff
@@ -1265,8 +1265,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 
 	junk_mode = JUNK_LEAVE_REPO;
-	fetch_if_missing = 1;
-	err = checkout(submodule_progress);
+	if (!err) {
+		fetch_if_missing = 1;
+		err = checkout(submodule_progress);
+	}
 
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);

[PATCH v2] clone: Don't segfault on -b specifing a non-commit

[Davide Berardi]

The code in "git clone -b B" to decide what branch to check out
assumed that B points at a commit object without checking,
leading to dereferencing a NULL pointer and causing a segmentation
fault.

Just aborting the operation when it happens is not a very
attractive option because we would be left with a directory
without .git/HEAD that cannot be used as a valid repository the
user can attempt to recover from the situation by checking out
something.

Fall back to use the 'master' branch, which is what we use when
the command without the "-b" option cannot figure out what
branch the remote side points with its HEAD.

Signed-off-by: Davide Berardi <berardi.dav@gmail.com>
---
 builtin/clone.c         | 49 ++++++++++++++++++++++++++++++++++++-----
 commit.c                | 16 +++++++++++---
 commit.h                |  4 ++++
 t/t5609-clone-branch.sh | 16 +++++++++++++-
 4 files changed, 75 insertions(+), 10 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f665b28ccc..f185b7f193 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -704,10 +704,46 @@ static void update_remote_refs(const struct ref *refs,
 	}
 }
 
-static void update_head(const struct ref *our, const struct ref *remote,
+static struct commit *lookup_commit_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;
+
+	if (!tip)
+		return NULL;
+
+	tip_commit = lookup_commit_reference_gently_err(the_repository, tip, 1, err);
+	if (!tip_commit) {
+		/*
+		 * The given non-commit cannot be checked out,
+		 * so have a 'master' branch and leave it unborn.
+		 */
+		warning(_("non-commit cannot be checked out"));
+		create_symref("HEAD", "refs/heads/master", msg);
+		return NULL;
+	}
+
+	return tip_commit;
+}
+
+static int update_head(const struct ref *our, const struct ref *remote,
 			const char *msg)
 {
+	int err = 0;
 	const char *head;
+	struct commit *c = NULL;
+
+	c = lookup_commit_helper(our, remote, msg, &err);
+	if (err)
+		return -1;
+
 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
 		/* Local default branch link */
 		if (create_symref("HEAD", our->name, NULL) < 0)
@@ -718,8 +754,6 @@ static void update_head(const struct ref *our, const struct ref *remote,
 			install_branch_config(0, head, option_origin, 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);
@@ -732,6 +766,7 @@ 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 err;
 }
 
 static int checkout(int submodule_progress)
@@ -1249,7 +1284,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			   branch_top.buf, reflog_msg.buf, transport,
 			   !is_local, filter_options.choice);
 
-	update_head(our_head_points_at, remote_head, reflog_msg.buf);
+	err = update_head(our_head_points_at, remote_head, reflog_msg.buf) < 0;
 
 	/*
 	 * We want to show progress for recursive submodule clones iff
@@ -1268,8 +1303,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 
 	junk_mode = JUNK_LEAVE_REPO;
-	fetch_if_missing = 1;
-	err = checkout(submodule_progress);
+	if (!err) {
+		fetch_if_missing = 1;
+		err = checkout(submodule_progress);
+	}
 
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);
diff --git a/commit.c b/commit.c
index a98de16e3d..ffb5230f0f 100644
--- a/commit.c
+++ b/commit.c
@@ -26,16 +26,26 @@ int save_commit_buffer = 1;
 
 const char *commit_type = "commit";
 
-struct commit *lookup_commit_reference_gently(struct repository *r,
-		const struct object_id *oid, int quiet)
+struct commit *lookup_commit_reference_gently_err(struct repository *r,
+		const struct object_id *oid, int quiet, int *err)
 {
+	struct commit *retval;
 	struct object *obj = deref_tag(r,
 				       parse_object(r, oid),
 				       NULL, 0);
 
 	if (!obj)
 		return NULL;
-	return object_as_type(r, obj, OBJ_COMMIT, quiet);
+	retval = object_as_type(r, obj, OBJ_COMMIT, quiet);
+	if (!retval && err)
+		*err = 1;
+	return retval;
+}
+
+struct commit *lookup_commit_reference_gently(struct repository *r,
+		const struct object_id *oid, int quiet)
+{
+	return lookup_commit_reference_gently_err(r, oid, quiet, NULL);
 }
 
 struct commit *lookup_commit_reference(struct repository *r, const struct object_id *oid)
diff --git a/commit.h b/commit.h
index f5295ca7f3..157c5dc526 100644
--- a/commit.h
+++ b/commit.h
@@ -70,6 +70,10 @@ struct commit *lookup_commit_reference(struct repository *r,
 struct commit *lookup_commit_reference_gently(struct repository *r,
 					      const struct object_id *oid,
 					      int quiet);
+struct commit *lookup_commit_reference_gently_err(struct repository *r,
+						  const struct object_id *oid,
+						  int quiet,
+						  int *err);
 struct commit *lookup_commit_reference_by_name(const char *name);
 
 /*
diff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh
index 6e7a7be052..d57f750eeb 100755
--- a/t/t5609-clone-branch.sh
+++ b/t/t5609-clone-branch.sh
@@ -20,7 +20,10 @@ 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 &&
-	 git checkout master) &&
+	 git checkout master &&
+	 blob=$(git rev-parse HEAD:file)  &&
+	 echo $blob > .git/refs/heads/broken-tag &&
+	 echo $blob > .git/refs/heads/broken-head) &&
 	mkdir empty &&
 	(cd empty && git init)
 '
@@ -67,4 +70,15 @@ test_expect_success 'clone -b not allowed with empty repos' '
 	test_must_fail git clone -b branch empty clone-branch-empty
 '
 
+test_expect_success 'clone -b with a non-commit tag must fallback' '
+	test_must_fail git clone -b broken-tag parent clone-broken-tag &&
+	(cd clone-broken-tag &&
+	 check_HEAD master)
+'
+test_expect_success 'clone -b with a non-commit head must fallback' '
+	test_must_fail git clone -b broken-head parent clone-broken-head &&
+	(cd clone-broken-head &&
+	 check_HEAD master)
+'
+
 test_done
-- 
2.22.0

Re: [PATCH] Segmentation Fault on non-commit --branch clone

[Davide Berardi]

On Sat, Nov 02, 2019 at 07:16:23PM +0900, Junio C Hamano wrote:
>Jeff King <peff@peff.net> writes:
>
>> I don't know how often this would actually help users, though. It _is_ a
>> pretty rare situation to ask for a non-commit. So maybe it's all
>> over-engineering, and we should start with just die(). If somebody comes
>> along later and wants to enhance it, it should be pretty
>> straightforward.
>
>I like that; after update_head() finishes, there are a few clean-up
>things that the caller wants to do besides a checkout() call, but if
>we make update_head() return a failure, perhaps the caller side
>change would be as small as the attached patch.  That would go nicely
>with the "make the result just barely usable" approach of leaving an
>unborn master branch I suggested in a separate message, I would think.
>
Thank you all for your precious comments, I've tried to implement
your suggestions and I've sent the patch here.

The problem with the proposed approach was that the code was
incompatible with some tests (specifically the tests which specifies an
empty .git directory would fail and fallback to the unborn master
branch).

The lookup commit have two error-paths:

1. the commit cannot be found;
2. the commit is found and cannot be casted to a commit (whoops!).

so, I've returned the second condition using an auxiliary variable and
declaring a new lookup_commit function keeping compatibility with the
old one.

I'm sorry for my errors but I'm far for an expert of git internals,
thank you (all) for your time and kindness.

ciao,
D.

Re: [PATCH] Segmentation Fault on non-commit --branch clone

[Junio C Hamano]

Davide Berardi <berardi.dav@gmail.com> writes:

> The problem with the proposed approach was that the code was
> incompatible with some tests (specifically the tests which specifies an
> empty .git directory would fail and fallback to the unborn master
> branch).
>
> The lookup commit have two error-paths:
>
> 1. the commit cannot be found;
> 2. the commit is found and cannot be casted to a commit (whoops!).
>
> so, I've returned the second condition using an auxiliary variable and
> declaring a new lookup_commit function keeping compatibility with the
> old one.

It's more like three, I think.

  1a. The given object name is a sentinel, "no such object", value.

  1b. The given object name is meant to name a real object, but
      there is no object with such a name in the repository.

  2.  The given object name names an existing object, but it does
      not peel to a commit.

Traditionally, we had only lookup_commit() and died on any of the
above.  Later, we added the _gently() variant for callers that want
to use a commit and also want to handle an error case where the
object name they are handed by their callers does not peel to a
commit.

In the "unborn repository, empty .git" case, are you getting a
random object name, or null_oid (aka case 1a. above)?  If that is
the case, then your solution to introduce another variant of
lookup_commit() that takes *err parameter is a wrong approach would
not differenciate between 1a. and 1b., which would not help, as I
suspect that we still do want to treat 1b. as an error.

Wouldn't it be cleaner to catch 1a. upfront, e.g.

	if (our)
		tip = &our->old_oid;
	else if (remote)
		tip = &remote->old_oid;
	else
		return NULL;

	if (is_null_oid(tip))
        	return NULL;

	tip_commit = lookup_commit_reference_gently(the_repository, tip, 1);
	if (!tip_commit) {
		warning(...);
		create_symref(...);
		return NULL;
	}

I am not offhand sure if the places we return NULL upfront want to
also create HEAD symref, or that is something automatically happens
for us in the downstream of this function, though.

Thanks.

Re: [PATCH v2] clone: Don't segfault on -b specifing a non-commit

[Jeff King]

On Sun, Nov 03, 2019 at 06:07:18PM +0000, Davide Berardi wrote:

> -static void update_head(const struct ref *our, const struct ref *remote,
> +static struct commit *lookup_commit_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;
> +
> +	if (!tip)
> +		return NULL;
> +
> +	tip_commit = lookup_commit_reference_gently_err(the_repository, tip, 1, err);
> +	if (!tip_commit) {
> +		/*
> +		 * The given non-commit cannot be checked out,
> +		 * so have a 'master' branch and leave it unborn.
> +		 */
> +		warning(_("non-commit cannot be checked out"));
> +		create_symref("HEAD", "refs/heads/master", msg);
> +		return NULL;
> +	}
> +
> +	return tip_commit;
> +}

I like the logic flow in this function, which is IMHO clearer than the
existing code. But the "err" thing puzzled me for a moment. I think you
are trying to tell the difference between the case that both "our" and
"remote" are NULL, and the case that we saw a non-commit. In either case
we return NULL, but only one is an error.

But:

  - I don't think that logic needs to extend down into
    lookup_commit_reference_gently(); a NULL return from it would always
    be an error, wouldn't it?

  - we could probably simplify this by just inlining it into
    update_head(). Something like:

      if (our)
              tip = &our->old_oid;
      else if (remote)
              tip = &remote->old_oid;

      if (!tip) {
	      /*
	       * 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 0;
      }

      tip_commit = lookup_commit_reference_gently(...);
      if (!tip_commit) {
              /*
	       * The given non-commit cannot be checked out, etc...
	       */
	      warning(...);
	      create_symref(...);
	      return -1;
      }

      ...and then existing code to use tip_commit...

      /*
       * we'd always return 0 here, because our update_ref calls die on
       * error
       */
      return 0;

> @@ -1268,8 +1303,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	junk_mode = JUNK_LEAVE_REPO;
> -	fetch_if_missing = 1;
> -	err = checkout(submodule_progress);
> +	if (!err) {
> +		fetch_if_missing = 1;
> +		err = checkout(submodule_progress);
> +	}

This part makes sense. We might want an explanatory comment along the
lines of:

  /*
   * Only try to checkout if we successfully updated HEAD; otherwise
   * HEAD isn't pointing to the thing the user wanted.
   /
   if (!err) {
       ...
   

> -struct commit *lookup_commit_reference_gently(struct repository *r,
> -		const struct object_id *oid, int quiet)
> +struct commit *lookup_commit_reference_gently_err(struct repository *r,
> +		const struct object_id *oid, int quiet, int *err)

And this part I think could just go away, if you take my suggestion
above.

> diff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh
> index 6e7a7be052..d57f750eeb 100755
> --- a/t/t5609-clone-branch.sh
> +++ b/t/t5609-clone-branch.sh
> @@ -20,7 +20,10 @@ 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 &&
> -	 git checkout master) &&
> +	 git checkout master &&
> +	 blob=$(git rev-parse HEAD:file)  &&
> +	 echo $blob > .git/refs/heads/broken-tag &&
> +	 echo $blob > .git/refs/heads/broken-head) &&

Minor style nit, but we usually avoid the space after ">".

> +test_expect_success 'clone -b with a non-commit tag must fallback' '
> +	test_must_fail git clone -b broken-tag parent clone-broken-tag &&
> +	(cd clone-broken-tag &&
> +	 check_HEAD master)
> +'
> +test_expect_success 'clone -b with a non-commit head must fallback' '
> +	test_must_fail git clone -b broken-head parent clone-broken-head &&
> +	(cd clone-broken-head &&
> +	 check_HEAD master)
> +'

OK, this second one covers the first conditional from update_head():

  if (our && skip_prefix(our->name, "refs/heads/", &head)) {

and the first one covers the second conditional:

  } else if (our) {

Should we cover the third conditional, too?

I think it would be the case that the remote HEAD is pointing to a
non-commit (since that's a corrupt repository, it might make sense
create a separate sub-repository).

-Peff

Re: [PATCH v2] clone: Don't segfault on -b specifing a non-commit

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I like the logic flow in this function, which is IMHO clearer than the
> existing code. But the "err" thing puzzled me for a moment. I think you
> are trying to tell the difference between the case that both "our" and
> "remote" are NULL, and the case that we saw a non-commit. In either case
> we return NULL, but only one is an error.

Yup.

> But:
>
>   - I don't think that logic needs to extend down into
>     lookup_commit_reference_gently(); a NULL return from it would always
>     be an error, wouldn't it?

Yes.

>   - we could probably simplify this by just inlining it into
>     update_head(). Something like:
>
>       if (our)
>               tip = &our->old_oid;
>       else if (remote)
>               tip = &remote->old_oid;
>
>       if (!tip) {
> 	      /*
> 	       * 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 0;
>       }

I somehow had an impression that Davide was protecting against the
case where tip->old_oid is null_oid (cloning from an empty repo?);
NULL return from lookup_commit_reference_gently(null_oid) would not
deserve a warning from this codepath, and should work just like the
way it has worked before these changes.

>       tip_commit = lookup_commit_reference_gently(...);
>       if (!tip_commit) {
>               /*
>> -	fetch_if_missing = 1;
>> -	err = checkout(submodule_progress);
>> +	if (!err) {
>> +		fetch_if_missing = 1;
>> +		err = checkout(submodule_progress);
>> +	}
>
> This part makes sense. We might want an explanatory comment along the
> lines of:
>
>   /*
>    * Only try to checkout if we successfully updated HEAD; otherwise
>    * HEAD isn't pointing to the thing the user wanted.
>    /
>    if (!err) {
>        ...

Yup.

>> diff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh
>
> I think it would be the case that the remote HEAD is pointing to a
> non-commit (since that's a corrupt repository, it might make sense
> create a separate sub-repository).

All good comments. 

Thanks

Re: [PATCH v2] clone: Don't segfault on -b specifing a non-commit

[Jeff King]

On Wed, Nov 06, 2019 at 10:36:18AM +0900, Junio C Hamano wrote:

> >   - we could probably simplify this by just inlining it into
> >     update_head(). Something like:
> >
> >       if (our)
> >               tip = &our->old_oid;
> >       else if (remote)
> >               tip = &remote->old_oid;
> >
> >       if (!tip) {
> > 	      /*
> > 	       * 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 0;
> >       }
> 
> I somehow had an impression that Davide was protecting against the
> case where tip->old_oid is null_oid (cloning from an empty repo?);
> NULL return from lookup_commit_reference_gently(null_oid) would not
> deserve a warning from this codepath, and should work just like the
> way it has worked before these changes.

I'm not sure if we need to worry about that case. Right now it would
just segfault, I think, which means it's not something that comes up
regularly (though whether the _best_ thing might be to consider a
non-error, I don't know without understanding why we'd see a null_oid in
the first place).

If we did want to handle it, though, I think it would be easy with the
setup I described; update_head() could check is_null_oid() itself.

-Peff

Re: [PATCH v2] clone: Don't segfault on -b specifing a non-commit

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If we did want to handle it, though, I think it would be easy with the
> setup I described; update_head() could check is_null_oid() itself.

Yup, that was what I had in my review on the previous round ;-)