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

From: Jeff King <peff@peff.net>
Date: Nov 1, 2019, 19:43 UTC
Message-ID: <20191101194339.GB1169@sigill.intra.peff.net>
In-Reply-To: <20191101002432.GA49846@carpenter.lan>

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:

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 16 quoted lines

> 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