git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Davide Berardi <berardi.dav@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] Segmentation fault on non-commit objects.
Date: Tue, 29 Oct 2019 10:06:21 -0400	[thread overview]
Message-ID: <20191029140621.GC2843@sigill.intra.peff.net> (raw)
In-Reply-To: <20191029092735.GA84120@carpenter.lan>

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

  parent reply	other threads:[~2019-10-29 14:06 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-29  9:27 Davide Berardi
2019-10-29 13:11 ` Johannes Schindelin
2019-11-01  0:26   ` Davide Berardi
2019-10-29 14:06 ` Jeff King [this message]
2019-10-30  2:44   ` Junio C Hamano
2019-10-31  5:37     ` Jeff King
2019-10-31 10:43       ` Davide Berardi
2019-11-01  0:29       ` Davide Berardi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191029140621.GC2843@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=berardi.dav@gmail.com \
    --cc=git@vger.kernel.org \
    --subject='Re: [PATCH] Segmentation fault on non-commit objects.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).