All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Andrey Okoshkin <a.okoshkin@samsung.com>
Cc: gitster@pobox.com, git@vger.kernel.org, pclouds@gmail.com,
	l.s.r@web.de, avarab@gmail.com, krh@redhat.com,
	rctay89@gmail.com, Ivan Arishchenko <i.arishchenk@samsung.com>,
	Mikhail Labiuk <m.labiuk@samsung.com>
Subject: [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()
Date: Thu, 19 Oct 2017 13:49:36 -0400	[thread overview]
Message-ID: <20171019174936.izojvrh5w35s3adi@sigill.intra.peff.net> (raw)
In-Reply-To: <20171019174452.hd3c47ocducddvgr@sigill.intra.peff.net>

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

  parent reply	other threads:[~2017-10-19 17:49 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20171018170047epcas2p4310be357e11e194d6d08ac3bdc478ba3@epcas2p4.samsung.com>
2017-10-18 17:00 ` [PATCH] commit: check result of resolve_ref_unsafe Andrey Okoshkin
2017-10-18 18:34   ` Jeff King
2017-10-19  0:41     ` Junio C Hamano
2017-10-19  2:49       ` Jeff King
2017-10-19  9:33         ` Andrey Okoshkin
2017-10-19  9:36   ` [PATCH v2] " Andrey Okoshkin
2017-10-19 17:44     ` Jeff King
2017-10-19 17:46       ` [PATCH 1/4] test-ref-store: avoid passing NULL to printf Jeff King
2017-10-19 17:47       ` [PATCH 2/4] remote: handle broken symrefs Jeff King
2017-10-19 17:53         ` Jeff King
2017-10-19 17:49       ` [PATCH 3/4] log: handle broken HEAD in decoration check Jeff King
2017-10-19 17:49       ` Jeff King [this message]
2017-10-21 10:49         ` [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref() Eric Sunshine
2017-10-21 19:26           ` Jeff King
2017-10-22  0:46             ` Junio C Hamano
2017-10-20 10:40       ` [PATCH v2] commit: check result of resolve_ref_unsafe Andrey Okoshkin
2017-10-20 11:03       ` [PATCH v3] " Andrey Okoshkin
2017-10-20 13:09         ` [PATCH v4] " Andrey Okoshkin
2017-10-21  6:19           ` Jeff King
2017-10-22  0:46             ` Junio C Hamano

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=20171019174936.izojvrh5w35s3adi@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=a.okoshkin@samsung.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=i.arishchenk@samsung.com \
    --cc=krh@redhat.com \
    --cc=l.s.r@web.de \
    --cc=m.labiuk@samsung.com \
    --cc=pclouds@gmail.com \
    --cc=rctay89@gmail.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.