[PATCH] commit: check result of resolve_ref_unsafe

[PATCH] commit: check result of resolve_ref_unsafe

[Andrey Okoshkin]

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

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");
 	if (!strcmp(head, "HEAD"))
 		head = _("detached HEAD");
 	else
-- 
2.14.2

Re: [PATCH] commit: check result of resolve_ref_unsafe

[Jeff King]

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.

> 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

Re: [PATCH] commit: check result of resolve_ref_unsafe

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

Re: [PATCH] commit: check result of resolve_ref_unsafe

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> If we do expect it to exist, shouldn't we checking with READING and
> catching a funny situation if it arises?

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

Re: [PATCH] commit: check result of resolve_ref_unsafe

[Andrey Okoshkin]

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

-- 
Best regards,
Andrey Okoshkin

[PATCH v2] commit: check result of resolve_ref_unsafe

[Andrey Okoshkin]

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

diff --git a/builtin/commit.c b/builtin/commit.c
index 1a0da71a4..cc27c9af7 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)
+		die(_("unable to resolve HEAD after creating commit"));
  	if (!strcmp(head, "HEAD"))
  		head = _("detached HEAD");
  	else
-- 
2.14.2

Re: [PATCH v2] commit: check result of resolve_ref_unsafe

[Jeff King]

On Thu, Oct 19, 2017 at 12:36:50PM +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.
> 
> 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.

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

>  	head = resolve_ref_unsafe("HEAD", 0, junk_oid.hash, NULL);
> +	if (!head)
> +		die(_("unable to resolve HEAD after creating commit"));

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

[PATCH 1/4] test-ref-store: avoid passing NULL to printf

[Jeff King]

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(-)

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 05d8c4d8af..6ec2670044 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -135,7 +135,7 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
 
 	ref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
 				      sha1, &flags);
-	printf("%s %s 0x%x\n", sha1_to_hex(sha1), ref, flags);
+	printf("%s %s 0x%x\n", sha1_to_hex(sha1), ref ? ref : "(null)", flags);
 	return ref ? 0 : 1;
 }
 
-- 
2.15.0.rc1.560.g5f0609e481

[PATCH 2/4] remote: handle broken symrefs

[Jeff King]

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(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 4f5cac96b0..bc89623695 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -565,7 +565,7 @@ static int read_remote_branches(const char *refname,
 		item = string_list_append(rename->remote_branches, xstrdup(refname));
 		symref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,
 					    NULL, &flag);
-		if (flag & REF_ISSYMREF)
+		if (symref && (flag & REF_ISSYMREF))
 			item->util = xstrdup(symref);
 		else
 			item->util = NULL;
-- 
2.15.0.rc1.560.g5f0609e481

Re: [PATCH 2/4] remote: handle broken symrefs

[Jeff King]

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

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

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

[PATCH 3/4] log: handle broken HEAD in decoration check

[Jeff King]

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(-)

diff --git a/log-tree.c b/log-tree.c
index cea056234d..580b3a98a0 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -198,7 +198,7 @@ static const struct name_decoration *current_pointed_by_HEAD(const struct name_d
 
 	/* Now resolve and find the matching current branch */
 	branch_name = resolve_ref_unsafe("HEAD", 0, NULL, &rru_flags);
-	if (!(rru_flags & REF_ISSYMREF))
+	if (!branch_name || !(rru_flags & REF_ISSYMREF))
 		return NULL;
 
 	if (!starts_with(branch_name, "refs/"))
-- 
2.15.0.rc1.560.g5f0609e481

[PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()

[Jeff King]

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(-)

diff --git a/worktree.c b/worktree.c
index 70015629dc..f8c40f2f5f 100644
--- a/worktree.c
+++ b/worktree.c
@@ -327,7 +327,8 @@ const struct worktree *find_shared_symref(const char *symref,
 		refs = get_worktree_ref_store(wt);
 		symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
 							NULL, &flags);
-		if ((flags & REF_ISSYMREF) && !strcmp(symref_target, target)) {
+		if ((flags & REF_ISSYMREF) &&
+		    symref_target && !strcmp(symref_target, target)) {
 			existing = wt;
 			break;
 		}
-- 
2.15.0.rc1.560.g5f0609e481

Re: [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()

[Eric Sunshine]

On Thu, Oct 19, 2017 at 1:49 PM, Jeff King <peff@peff.net> wrote:
> 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().
> [...]
> 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

s/tell//

> 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>

Re: [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()

[Jeff King]

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

> On Thu, Oct 19, 2017 at 1:49 PM, Jeff King <peff@peff.net> wrote:
> > 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().
> > [...]
> > 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
> 
> s/tell//

Right, thank you.

-Peff

Re: [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Oct 21, 2017 at 06:49:15AM -0400, Eric Sunshine wrote:
>
>> On Thu, Oct 19, 2017 at 1:49 PM, Jeff King <peff@peff.net> wrote:
>> > 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().
>> > [...]
>> > 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
>> 
>> s/tell//
>
> Right, thank you.
>
> -Peff

Thanks.  Already tweaked in.

Re: [PATCH v2] commit: check result of resolve_ref_unsafe

[Andrey Okoshkin]

19.10.2017 20:44, Jeff King wrote:
> On Thu, Oct 19, 2017 at 12:36:50PM +0300, Andrey Okoshkin wrote:
> 
> Thanks, this looks good to me. One other possible minor improvement:
> 
>>   	head = resolve_ref_unsafe("HEAD", 0, junk_oid.hash, NULL);
>> +	if (!head)
>> +		die(_("unable to resolve HEAD after creating commit"));
> 
> 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).
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'.

> 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()
Good changes, it's better to apply.

-- 
Best regards,
Andrey

[PATCH v3] commit: check result of resolve_ref_unsafe

[Andrey Okoshkin]

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

diff --git a/builtin/commit.c b/builtin/commit.c
index 1a0da71a4..b52829090 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)
+		die_errno(_("unable to resolve HEAD after creating commit"));
 	if (!strcmp(head, "HEAD"))
 		head = _("detached HEAD");
 	else
-- 
2.14.2

[PATCH v4] commit: check result of resolve_ref_unsafe

[Andrey Okoshkin]

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(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1a0da71a4..647d6ab3e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1483,7 +1483,10 @@ 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 (!strcmp(head, "HEAD"))
+	if (!head) {
+		warning_errno(_("unable to resolve HEAD after creating commit"));
+		head = _("unresolvable HEAD");
+	} else if (!strcmp(head, "HEAD"))
 		head = _("detached HEAD");
 	else
 		skip_prefix(head, "refs/heads/", &head);
-- 
2.14.2

Re: [PATCH v4] commit: check result of resolve_ref_unsafe

[Jeff King]

On Fri, Oct 20, 2017 at 04:09:30PM +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.
> 
> 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.

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

Re: [PATCH v4] commit: check result of resolve_ref_unsafe

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

True.  I've queued v3 for now.

Thanks, both.