On Tue, Oct 29, 2019 at 10:27:35AM +0100, Davide Berardi wrote:
> Fixed segmentation fault that can be triggered using
> $ git clone --branch $object $repository
> with object pointing to a non-commit (e.g. a blob).
One subtle thing here is that $object still needs to be at the tip of a
ref (an easy real-world case is "-b junio-gpg-pub" against git.git).
It might be nice to cover this with a test.
> diff --git a/builtin/clone.c b/builtin/clone.c
> index f665b28ccc..6ad2d8fe77 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -720,6 +720,9 @@ 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);
> + /* Check if --branch specifies a non-commit. */
> + if (c == NULL)
> + die(_("unable to update HEAD (cannot find commit)"));
This is definitely a strict improvement over the current behavior
(though I agree with Dscho's comments on the error message). A few
further thoughts:
- we'll have successfully completed the rest of the clone at this
point. Should we leave the objects and refs in place to allow the
user to fix it up, as we do when "git checkout" fails?
We'd have to leave _something_ in HEAD for it to be a valid repo. I
guess just "refs/heads/master" would be fine, or perhaps we could
fall back to whatever the other side had in their HEAD (i.e.,
pretending that "-b" wasn't specified).
- there's a related case just above the lines you touched: what
happens if the other side feeds us a non-commit in their
refs/heads/? That shouldn't happen (i.e., their repo is broken), but
should we be protecting ourselves on the receiving side more?
Likewise in "else" below just above your lines.
I think in either case we wouldn't segfault (because we don't try to
dereference to a commit ourselves), but we'd produce a bogus on-disk
state.
-Peff
