patchSegmentation Fault on non-commit --branch clone

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(-)

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:

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()`.

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`.

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.

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

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

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

Jeff King <peff@peff.net> writes:

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(-)

On Sat, Nov 02, 2019 at 07:16:23PM +0900, Junio C Hamano wrote:

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.

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

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.

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

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.

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

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:

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

and then again in the second case-arm:

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

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).

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).

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

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

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

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

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.

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.

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

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

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.

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.

Thanks.

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(-)

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

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;

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) {
       ...
   

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

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

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

Jeff King <peff@peff.net> writes:

Yup.

Yes.

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.

Yup.

All good comments. 

Thanks

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

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

Jeff King <peff@peff.net> writes:

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