patchcommit: check result of resolve_ref_unsafe

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.

Signed-off-by: Andrey Okoshkin <a.okoshkin@samsung.com>
---
Hello,
I've injected a fault to git binary with the internal tool for fault tolerance
evaluation.

lstat() or open() calls return '-1', errno is set to 'EACCES': #0 0x6559a4 in files_read_raw_ref refs/files-backend.c:686 #1 0x642816 in refs_read_raw_ref /home/tesla/devel/repos/git/refs.c:1392 #2 0x642a69 in refs_resolve_ref_unsafe /home/tesla/devel/repos/git/refs.c:1431 #3 0x6443ce in resolve_ref_unsafe /home/tesla/devel/repos/git/refs.c:1483 #4 0x44822e in print_summary builtin/commit.c:1485 #5 0x44822e in cmd_commit builtin/commit.c:1817 #6 0x4084f5 in run_builtin /home/tesla/devel/repos/git/git.c:342 #7 0x4084f5 in handle_builtin /home/tesla/devel/repos/git/git.c:550 #8 0x40997b in run_argv /home/tesla/devel/repos/git/git.c:602 #9 0x40997b in cmd_main /home/tesla/devel/repos/git/git.c:679 #10 0x408087 in main /home/tesla/devel/repos/git/common-main.c:43

As a result git crashes silently with SIGSEGV at 'strcmp(head, "HEAD")': #0 0x447c16 in print_summary builtin/commit.c:1486 #1 0x447c16 in cmd_commit builtin/commit.c:1817 #2 0x4084f5 in run_builtin /home/tesla/devel/repos/git/git.c:342 #3 0x4084f5 in handle_builtin /home/tesla/devel/repos/git/git.c:550 #4 0x40997b in run_argv /home/tesla/devel/repos/git/git.c:602 #5 0x40997b in cmd_main /home/tesla/devel/repos/git/git.c:679 #6 0x408087 in main /home/tesla/devel/repos/git/common-main.c:43

It seems that in a real life it's very difficult to reproduce such behaviour because the readability of '.git' directory is checked before. But still the NULL pointer result returned by resolve_ref_unsafe() is not checked anyhow. That's why I'm not sure whether it's a bug or not.

Best regards, Andrey

 builtin/commit.c | 2 ++
 1 file changed, 2 insertions(+)

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

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.

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

Jeff King <peff@peff.net> writes:

If we do expect it to exist, shouldn't we checking with READING and catching a funny situation if it arises?

On Thu, Oct 19, 2017 at 09:41:29AM +0900, Junio C Hamano wrote:

Maybe. In the normal case it would not matter. If we _do_ find it gone, I guess that is a sign that somebody is racing with us and changed HEAD after we made the commit.

TBH, I think the "most right" thing would be to actually capture the ref that HEAD points to when we actually make the commit, remember it, and then report it here (the whole function of this code is just to say "I made a commit on branch X"). But I don't know how much trouble it is worth going to for that. It's buried behind a ref_transaction, and I don't think builtin/commit.c ever sees the real name (though maybe it would be a good feature of the transaction to record the destinations of any symrefs we updated).

-Peff

I also think it's a good idea to record the destination of the updated symref. But for now the most simple solution is just to catch the unreadable HEAD error. Maybe for the future improvement it would be nice to redesign print_summary passing some ref_transaction results to it.

19.10.2017 05:49, Jeff King 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.

Signed-off-by: Andrey Okoshkin <a.okoshkin@samsung.com>
---
Thank you for your review.

Changes since the previous patch:
* BUG is replaced with die, message;
* Message is changed.

  builtin/commit.c | 2 ++
  1 file changed, 2 insertions(+)

On Thu, Oct 19, 2017 at 12:36:50PM +0300, Andrey Okoshkin wrote:

Thanks, this looks good to me. One other possible minor improvement:

Should we use die_errno() here to report the value of errno? I think resolve_ref_unsafe() should set it consistently (even an internal problem, like an illegally-formatted refname, yields EINVAL).

I grepped the code base looking for other instances of the same problem, and found four of them. Patches to follow.

Unlike this one, I ended up quietly returning an error in most cases. The individual commit messages discuss the reasoning for each case, but I do wonder if we ought to simply die() in each case out of an abundance of caution (either the repo has a broken symref, or some weird filesystem error occurred, but either way it may be best not to continue). I dunno.

These are all independent, so can be applied in any order or combination with respect to each other and to your patch.

  [1/4]: test-ref-store: avoid passing NULL to printf
  [2/4]: remote: handle broken symrefs
  [3/4]: log: handle broken HEAD in decoration check
  [4/4]: worktree: handle broken symrefs in find_shared_symref()

 builtin/remote.c          | 2 +-
 log-tree.c                | 2 +-
 t/helper/test-ref-store.c | 2 +-
 worktree.c                | 3 ++-
 4 files changed, 5 insertions(+), 4 deletions(-)

-Peff

It's possible for resolve_ref_unsafe() to return NULL (e.g., if we are reading and the ref does not exist), in which case we'll pass NULL to printf. On glibc systems this produces "(null)", but on others it may segfault.

The tests don't expect any such case, but if we ever did trigger this, we would prefer to cleanly fail the test with unexpected input rather than segfault. Let's manually replace NULL with "(null)". The exact value doesn't matter, as it won't match any possible ref the caller could expect (and anyway, the exit code of the program will tell whether "ref" is valid or not).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-ref-store.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

It's possible for resolve_ref_unsafe() to return NULL with a REF_ISSYMREF flag if a symref points to a broken ref. In this case, the read_remote_branches() function will segfault passing the name to xstrdup().

This is hard to trigger in practice, since this function is used as a callback to for_each_ref(), which will skip broken refs in the first place (so it would have to be broken racily, or for us to see a transient filesystem error).

If we see such a racy broken outcome let's treat it as "not a symref". This is exactly the same thing that would happen in the non-racy case (our function would not be called at all, as for_each_ref would skip the broken symref).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Thu, Oct 19, 2017 at 01:47:30PM -0400, Jeff King wrote:

The fact that we have to re-resolve the ref here to find the symref points to a short-coming in the for_each_ref() interface. It resolved the ref already to get us the oid, so it should (or at least could) know the symref details already. But it doesn't record them or make them available to callers.

Ditto for patch 3. It doesn't use for_each_ref(), but I suspect it could be recording the value of HEAD more carefully from the prior lookup, avoiding the re-resolution completely.

Refactoring for_each_ref() is probably a bit big for a #leftoverbits, but looking into the case in patch 3 might not be.

-Peff

The resolve_ref_unsafe() function may return NULL even with a REF_ISSYMREF flag if a symref points to a broken ref. As a result, it's possible for the decoration code's "is this branch the current HEAD" check to segfault when it passes the NULL to starts_with().

This is unlikely in practice, since we can only reach this code if we already resolved HEAD to a matching sha1 earlier. But it's possible if HEAD racily becomes broken, or if there's a transient filesystem error.

We can fix this by returning early in the broken case, since NULL could not possibly match any of our branch names.

Signed-off-by: Jeff King <peff@peff.net>
---
 log-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The refs_resolve_ref_unsafe() function may return NULL even with a REF_ISSYMREF flag if a symref points to a broken ref. As a result, it's possible for find_shared_symref() to segfault when it passes NULL to strcmp().

This is hard to trigger for most code paths. We typically pass HEAD to the function as the symref to resolve, and programs like "git branch" will bail much earlier if HEAD isn't valid.

I did manage to trigger it through one very obscure sequence:

  # You have multiple notes refs which conflict.
  git notes add -m base
  git notes --ref refs/notes/foo add -m foo

  # There's left-over cruft in NOTES_MERGE_REF that
  # makes it a broken symref (in this case we point
  # to a syntactically invalid ref).
  echo "ref: refs/heads/master.lock" >.git/NOTES_MERGE_REF

  # You try to merge the notes. We read the broken value in
  # order to complain that another notes-merge is
  # in-progress, but we segfault in find_shared_symref().
  git notes merge refs/notes/foo

This is obviously silly and almost certainly impossible to trigger accidentally, but it does show that the bug is triggerable from at least one code path. In addition, it would trigger if we saw a transient filesystem error when resolving the pointed-to ref.

We can fix this by treating NULL the same as a non-matching symref. Arguably we'd prefer to tell know if a symref points to "refs/heads/foo", but "refs/heads/foo" is broken. But refs_resolve_ref_unsafe() isn't capable of giving us that information, so this is the best we can do.

Signed-off-by: Jeff King <peff@peff.net>
---
 worktree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On Thu, Oct 19, 2017 at 1:49 PM, Jeff King <peff@peff.net> wrote:

s/tell//

On Sat, Oct 21, 2017 at 06:49:15AM -0400, Eric Sunshine wrote:

Right, thank you.

-Peff

Jeff King <peff@peff.net> writes:

Thanks.  Already tweaked in.

19.10.2017 20:44, Jeff King wrote:

Thanks, sounds relevant. Also as an alternative solution it's possible to use warning_errno (instead of die_errno) and continue with 'head' set to something like 'unreadable|bad HEAD'.

Good changes, it's better to apply.

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.

Signed-off-by: Andrey Okoshkin <a.okoshkin@samsung.com>
---
die_errno fits better here as resolve_ref_unsafe preserves errno value.

Changes since the previous patch:
* die is replaced with die_errno.
 builtin/commit.c | 2 ++
 1 file changed, 2 insertions(+)

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.

Signed-off-by: Andrey Okoshkin <a.okoshkin@samsung.com>
---
Hello,
I think this way is better for user experience:
* git doesn't crash;
* warning is shown;
* commit has been successfully created then it's safe to show a summary message
with already known information and without resolved HEAD.

Best regards, Andrey

 builtin/commit.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

On Fri, Oct 20, 2017 at 04:09:30PM +0300, Andrey Okoshkin wrote:

I'm on the fence between this and the die_errno() version. Given that this would basically never happen in practice, I don't think it matters too much. And that makes me want to just err on the side of simplicity. But it's not like this is all that complex, either.

-Peff

Jeff King <peff@peff.net> writes:

True.  I've queued v3 for now.

Thanks, both.