git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Oct 19, 2017, 17:49 UTC
Message-ID
<20171019174936.izojvrh5w35s3adi@sigill.intra.peff.net>
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
Previous: Jeff KingNext: Eric Sunshine
Message 12 of 20 in “commit: check result of resolve_ref_unsafe”
  1. commit: check result of resolve_ref_unsafeAndrey Okoshkin, Oct 18, 2017
  2. Jeff KingOct 18, 2017
  3. Junio C HamanoOct 19, 2017
  4. Jeff KingOct 19, 2017
  5. Andrey OkoshkinOct 19, 2017
  6. commit: check result of resolve_ref_unsafeAndrey Okoshkin, Oct 19, 2017
  7. Jeff KingOct 19, 2017
  8. 1/4 test-ref-store: avoid passing NULL to printfJeff King, Oct 19, 2017
  9. 2/4 remote: handle broken symrefsJeff King, Oct 19, 2017
  10. Jeff KingOct 19, 2017
  11. 3/4 log: handle broken HEAD in decoration checkJeff King, Oct 19, 2017
  12. 4/4 worktree: handle broken symrefs in find_shared_symref()Jeff King, Oct 19, 2017
  13. Eric SunshineOct 21, 2017
  14. Jeff KingOct 21, 2017
  15. Junio C HamanoOct 22, 2017
  16. Andrey OkoshkinOct 20, 2017
  17. commit: check result of resolve_ref_unsafeAndrey Okoshkin, Oct 20, 2017
  18. commit: check result of resolve_ref_unsafeAndrey Okoshkin, Oct 20, 2017
  19. Jeff KingOct 21, 2017
  20. Junio C HamanoOct 22, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.