Re: [PATCH] commit: check result of resolve_ref_unsafe

From: Jeff King <peff@peff.net>
Date: Oct 18, 2017, 18:34 UTC
Message-ID: <20171018183453.lr67zw455bkdmrlz@sigill.intra.peff.net>
In-Reply-To: <0e396c24-167f-901e-9122-cdc17164ec1e@samsung.com>

On Wed, Oct 18, 2017 at 08:00:43PM +0300, Andrey Okoshkin wrote:

> Add check of the resolved HEAD reference while printing of a commit summary.
> resolve_ref_unsafe() may return NULL pointer if underlying calls of lstat() or
> open() fail in files_read_raw_ref().
> Such situation can be caused by race: file becomes inaccessible to this moment.

Yeah, I think we've had several bugs over the years with not checking the result of resolve_ref_unsafe(). Simply because it's so rare for it to fail without the READING flag, especially on HEAD, these bugs tend to linger.

But I agree we should be handling this case, and that it could trigger in real life because of a race or other weird intermittent failure.

I was able to trigger it by doing this in one terminal:

  while true; do
    git commit --allow-empty -m foo

    # we may see any of:
    #  - success
    #  - not a git repo (because HEAD is broken when we do setup)
    #  - can't lock HEAD (because it's broken when we take the lock)
    #  - a segfault (HEAD is broken when we try to print the summary)
    # but we only care about the last one
    ret=$?
    test $ret = 0 || test $ret = 128 || break
  done

and this in another:

  # pick some valid sha1
  sha1=$(git rev-parse HEAD)

  # flip back and forth between broken and valid states
  while true; do
    echo trash >.git/HEAD
    echo $sha >.git/HEAD
  done

Obviously this is silly, but it does eventually trigger the segfault.

Show 10 quoted lines

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 1a0da71a4..71a58dea3 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1483,6 +1483,8 @@ static void print_summary(const char *prefix, const struct object_id *oid,
>  	diff_setup_done(&rev.diffopt);
>  
>  	head = resolve_ref_unsafe("HEAD", 0, junk_oid.hash, NULL);
> +	if (!head)
> +		BUG("unable to resolve HEAD reference");

Checking !head here is the right thing to do, but I don't think this is a BUG(). It's not a logic error in the program, but rather an unexpected result. So probably:

  die("unable to resolve HEAD reference");

would be more appropriate. It's also possible that we could simply continue. We _did_ make the commit, but we're just failing at the informational bits. This should be sufficiently uncommon that I think dying is probably fine. We maybe could say:

  die("unable to resolve HEAD after creating commit")

or something so that the user has some clue that the commit did in fact happen (depending on the error, further commands may or may not see the updated value of HEAD).

Tangential to your patch, I also wondered why we did not pass RESOLVE_REF_READING to resolve_ref_unsafe(). I think the answer is that for symref lookups, we normally don't pass it so that we can handle dangling symrefs. Of course we _just_ wrote HEAD ourselves, so we'd expect it to exist, so it shouldn't really matter either way.

-Peff