From cc61cf465f7777d0aebd1a35ca72650fef8f253f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Oct 2017 13:46:21 -0400 Subject: test-ref-store: avoid passing NULL to printf 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 Signed-off-by: Junio C Hamano --- 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; } -- cgit v1.2.1 From 752848df0f18a9f8eb808a5b54c862c176d86e6d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Oct 2017 13:47:30 -0400 Subject: remote: handle broken symrefs 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 Signed-off-by: Junio C Hamano --- 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; -- cgit v1.2.1 From d79be4983bdf6598f106710a4826752a96f5dd58 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Oct 2017 13:49:01 -0400 Subject: log: handle broken HEAD in decoration check 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 Signed-off-by: Junio C Hamano --- 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/")) -- cgit v1.2.1 From dbd2b55cb7b06e94096b8c18852a94732e3f76a8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Oct 2017 13:49:36 -0400 Subject: worktree: handle broken symrefs in find_shared_symref() 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 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 Signed-off-by: Junio C Hamano --- 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; } -- cgit v1.2.1