{"thread":{"id":"46994","subject":"[PATCH] commit: check result of resolve_ref_unsafe","startedAt":"2017-10-18T17:00:59Z","lastAt":"2017-10-22T00:47:03Z","messageCount":20,"participants":["Andrey Okoshkin","Jeff King","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"330602","messageId":"0e396c24-167f-901e-9122-cdc17164ec1e@samsung.com","threadId":"46994","inReplyTo":"CGME20171018170047epcas2p4310be357e11e194d6d08ac3bdc478ba3@epcas2p4.samsung.com","subject":"[PATCH] commit: check result of resolve_ref_unsafe","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-18T17:00:43Z","receivedAt":"2017-10-18T17:00:59Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Add check of the resolved HEAD reference while printing of a commit summary.\nresolve_ref_unsafe() may return NULL pointer if underlying calls of lstat() or\nopen() fail in files_read_raw_ref().\nSuch situation can be caused by race: file becomes inaccessible to this moment.\n\nSigned-off-by: Andrey Okoshkin \n---\nHello,\nI've injected a fault to git binary with the internal tool for fault tolerance\nevaluation.\n\nlstat() or open() calls return '-1', errno is set to 'EACCES':\n#0 0x6559a4 in files_read_raw_ref refs/files-backend.c:686\n#1 0x642816 in refs_read_raw_ref /home/tesla/devel/repos/git/refs.c:1392\n#2 0x642a69 in refs_resolve_ref_unsafe /home/tesla/devel/repos/git/refs.c:1431\n#3 0x6443ce in resolve_ref_unsafe /home/tesla/devel/repos/git/refs.c:1483\n#4 0x44822e in print_summary builtin/commit.c:1485\n#5 0x44822e in cmd_commit builtin/commit.c:1817\n#6 0x4084f5 in run_builtin /home/tesla/devel/repos/git/git.c:342\n#7 0x4084f5 in handle_builtin /home/tesla/devel/repos/git/git.c:550\n#8 0x40997b in run_argv /home/tesla/devel/repos/git/git.c:602\n#9 0x40997b in cmd_main /home/tesla/devel/repos/git/git.c:679\n#10 0x408087 in main /home/tesla/devel/repos/git/common-main.c:43\n\nAs a result git crashes silently with SIGSEGV at 'strcmp(head, \"HEAD\")':\n#0 0x447c16 in print_summary builtin/commit.c:1486\n#1 0x447c16 in cmd_commit builtin/commit.c:1817\n#2 0x4084f5 in run_builtin /home/tesla/devel/repos/git/git.c:342\n#3 0x4084f5 in handle_builtin /home/tesla/devel/repos/git/git.c:550\n#4 0x40997b in run_argv /home/tesla/devel/repos/git/git.c:602\n#5 0x40997b in cmd_main /home/tesla/devel/repos/git/git.c:679\n#6 0x408087 in main /home/tesla/devel/repos/git/common-main.c:43\n\nIt seems that in a real life it's very difficult to reproduce such behaviour\nbecause the readability of '.git' directory is checked before. But still the\nNULL pointer result returned by resolve_ref_unsafe() is not checked anyhow.\nThat's why I'm not sure whether it's a bug or not.\n\nBest regards,\nAndrey\n\n builtin/commit.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1a0da71a4..71a58dea3 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1483,6 +1483,8 @@ static void print_summary(const char *prefix, const struct object_id *oid,\n \tdiff_setup_done(&rev.diffopt);\n \n \thead = resolve_ref_unsafe(\"HEAD\", 0, junk_oid.hash, NULL);\n+\tif (!head)\n+\t\tBUG(\"unable to resolve HEAD reference\");\n \tif (!strcmp(head, \"HEAD\"))\n \t\thead = _(\"detached HEAD\");\n \telse\n-- \n2.14.2\n"},{"id":"330605","messageId":"20171018183453.lr67zw455bkdmrlz@sigill.intra.peff.net","threadId":"46994","inReplyTo":"0e396c24-167f-901e-9122-cdc17164ec1e@samsung.com","subject":"Re: [PATCH] commit: check result of resolve_ref_unsafe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-18T18:34:53Z","receivedAt":"2017-10-18T18:35:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 18, 2017 at 08:00:43PM +0300, Andrey Okoshkin wrote:\n\n> Add check of the resolved HEAD reference while printing of a commit summary.\n> resolve_ref_unsafe() may return NULL pointer if underlying calls of lstat() or\n> open() fail in files_read_raw_ref().\n> Such situation can be caused by race: file becomes inaccessible to this moment.\n\nYeah, I think we've had several bugs over the years with not checking\nthe result of resolve_ref_unsafe(). Simply because it's so rare for it\nto fail without the READING flag, especially on HEAD, these bugs tend to\nlinger.\n\nBut I agree we should be handling this case, and that it could trigger\nin real life because of a race or other weird intermittent failure.\n\nI was able to trigger it by doing this in one terminal:\n\n while true; do\n git commit --allow-empty -m foo\n\n # we may see any of:\n # - success\n # - not a git repo (because HEAD is broken when we do setup)\n # - can't lock HEAD (because it's broken when we take the lock)\n # - a segfault (HEAD is broken when we try to print the summary)\n # but we only care about the last one\n ret=$?\n test $ret = 0 || test $ret = 128 || break\n done\n\nand this in another:\n\n # pick some valid sha1\n sha1=$(git rev-parse HEAD)\n\n # flip back and forth between broken and valid states\n while true; do\n echo trash >.git/HEAD\n echo $sha >.git/HEAD\n done\n\nObviously this is silly, but it does eventually trigger the segfault.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 1a0da71a4..71a58dea3 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1483,6 +1483,8 @@ static void print_summary(const char *prefix, const struct object_id *oid,\n> \tdiff_setup_done(&rev.diffopt);\n> \n> \thead = resolve_ref_unsafe(\"HEAD\", 0, junk_oid.hash, NULL);\n> +\tif (!head)\n> +\t\tBUG(\"unable to resolve HEAD reference\");\n\nChecking !head here is the right thing to do, but I don't think this is\na BUG(). It's not a logic error in the program, but rather an unexpected\nresult. So probably:\n\n die(\"unable to resolve HEAD reference\");\n\nwould be more appropriate. It's also possible that we could simply\ncontinue. We _did_ make the commit, but we're just failing at the\ninformational bits. This should be sufficiently uncommon that I think\ndying is probably fine. We maybe could say:\n\n die(\"unable to resolve HEAD after creating commit\")\n\nor something so that the user has some clue that the commit did in fact\nhappen (depending on the error, further commands may or may not see the\nupdated value of HEAD).\n\nTangential to your patch, I also wondered why we did not pass\nRESOLVE_REF_READING to resolve_ref_unsafe(). I think the answer is that\nfor symref lookups, we normally don't pass it so that we can handle\ndangling symrefs. Of course we _just_ wrote HEAD ourselves, so we'd\nexpect it to exist, so it shouldn't really matter either way.\n\n-Peff\n"},{"id":"330617","messageId":"xmqqo9p4j7eu.fsf@gitster.mtv.corp.google.com","threadId":"46994","inReplyTo":"20171018183453.lr67zw455bkdmrlz@sigill.intra.peff.net","subject":"Re: [PATCH] commit: check result of resolve_ref_unsafe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-19T00:41:29Z","receivedAt":"2017-10-19T00:41:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Tangential to your patch, I also wondered why we did not pass\n> RESOLVE_REF_READING to resolve_ref_unsafe(). I think the answer is that\n> for symref lookups, we normally don't pass it so that we can handle\n> dangling symrefs. Of course we _just_ wrote HEAD ourselves, so we'd\n> expect it to exist, so it shouldn't really matter either way.\n\nIf we do expect it to exist, shouldn't we checking with READING and\ncatching a funny situation if it arises?\n"},{"id":"330619","messageId":"20171019024919.ram3bzabt5fra2lc@sigill.intra.peff.net","threadId":"46994","inReplyTo":"xmqqo9p4j7eu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: check result of resolve_ref_unsafe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T02:49:19Z","receivedAt":"2017-10-19T02:49:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 09:41:29AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Tangential to your patch, I also wondered why we did not pass\n> > RESOLVE_REF_READING to resolve_ref_unsafe(). I think the answer is that\n> > for symref lookups, we normally don't pass it so that we can handle\n> > dangling symrefs. Of course we _just_ wrote HEAD ourselves, so we'd\n> > expect it to exist, so it shouldn't really matter either way.\n> \n> If we do expect it to exist, shouldn't we checking with READING and\n> catching a funny situation if it arises?\n\nMaybe. In the normal case it would not matter. If we _do_ find it gone,\nI guess that is a sign that somebody is racing with us and changed HEAD\nafter we made the commit.\n\nTBH, I think the \"most right\" thing would be to actually capture the ref\nthat HEAD points to when we actually make the commit, remember it, and\nthen report it here (the whole function of this code is just to say \"I\nmade a commit on branch X\"). But I don't know how much trouble it is\nworth going to for that. It's buried behind a ref_transaction, and I\ndon't think builtin/commit.c ever sees the real name (though maybe it\nwould be a good feature of the transaction to record the destinations of\nany symrefs we updated).\n\n-Peff\n"},{"id":"330630","messageId":"62255b66-67be-e6cf-92e9-bed92c6b2899@samsung.com","threadId":"46994","inReplyTo":"20171019024919.ram3bzabt5fra2lc@sigill.intra.peff.net","subject":"Re: [PATCH] commit: check result of resolve_ref_unsafe","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-19T09:33:30Z","receivedAt":"2017-10-19T09:34:45Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"I also think it's a good idea to record the destination of the updated\nsymref. But for now the most simple solution is just to catch the\nunreadable HEAD error.\nMaybe for the future improvement it would be nice to redesign\nprint_summary passing some ref_transaction results to it.\n\n19.10.2017 05:49, Jeff King wrote:\n> TBH, I think the \"most right\" thing would be to actually capture the ref\n> that HEAD points to when we actually make the commit, remember it, and\n> then report it here (the whole function of this code is just to say \"I\n> made a commit on branch X\"). But I don't know how much trouble it is\n> worth going to for that. It's buried behind a ref_transaction, and I\n> don't think builtin/commit.c ever sees the real name (though maybe it\n> would be a good feature of the transaction to record the destinations of\n> any symrefs we updated).\n\n-- \nBest regards,\nAndrey Okoshkin\n"},{"id":"330631","messageId":"5fa1f5c6-249e-2aa9-5e9f-c00ebe2c0d9d@samsung.com","threadId":"46994","inReplyTo":"0e396c24-167f-901e-9122-cdc17164ec1e@samsung.com","subject":"[PATCH v2] commit: check result of resolve_ref_unsafe","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-19T09:36:50Z","receivedAt":"2017-10-19T09:37:04Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Add check of the resolved HEAD reference while printing of a commit summary.\nresolve_ref_unsafe() may return NULL pointer if underlying calls of \nlstat() or\nopen() fail in files_read_raw_ref().\nSuch situation can be caused by race: file becomes inaccessible to this \nmoment.\n\nSigned-off-by: Andrey Okoshkin \n---\nThank you for your review.\n\nChanges since the previous patch:\n* BUG is replaced with die, message;\n* Message is changed.\n\n builtin/commit.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1a0da71a4..cc27c9af7 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1483,6 +1483,8 @@ static void print_summary(const char *prefix, \nconst struct object_id *oid,\n \tdiff_setup_done(&rev.diffopt);\n\n \thead = resolve_ref_unsafe(\"HEAD\", 0, junk_oid.hash, NULL);\n+\tif (!head)\n+\t\tdie(_(\"unable to resolve HEAD after creating commit\"));\n \tif (!strcmp(head, \"HEAD\"))\n \t\thead = _(\"detached HEAD\");\n \telse\n-- \n2.14.2\n"},{"id":"330648","messageId":"20171019174452.hd3c47ocducddvgr@sigill.intra.peff.net","threadId":"46994","inReplyTo":"5fa1f5c6-249e-2aa9-5e9f-c00ebe2c0d9d@samsung.com","subject":"Re: [PATCH v2] commit: check result of resolve_ref_unsafe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T17:44:53Z","receivedAt":"2017-10-19T17:45:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 12:36:50PM +0300, Andrey Okoshkin wrote:\n\n> Add check of the resolved HEAD reference while printing of a commit summary.\n> resolve_ref_unsafe() may return NULL pointer if underlying calls of lstat()\n> or\n> open() fail in files_read_raw_ref().\n> Such situation can be caused by race: file becomes inaccessible to this\n> moment.\n> \n> Signed-off-by: Andrey Okoshkin \n> ---\n> Thank you for your review.\n> \n> Changes since the previous patch:\n> * BUG is replaced with die, message;\n> * Message is changed.\n\nThanks, this looks good to me. One other possible minor improvement:\n\n> \thead = resolve_ref_unsafe(\"HEAD\", 0, junk_oid.hash, NULL);\n> +\tif (!head)\n> +\t\tdie(_(\"unable to resolve HEAD after creating commit\"));\n\nShould we use die_errno() here to report the value of errno? I think\nresolve_ref_unsafe() should set it consistently (even an internal\nproblem, like an illegally-formatted refname, yields EINVAL).\n\nI grepped the code base looking for other instances of the same problem,\nand found four of them. Patches to follow.\n\nUnlike this one, I ended up quietly returning an error in most cases.\nThe individual commit messages discuss the reasoning for each case, but\nI do wonder if we ought to simply die() in each case out of an abundance\nof caution (either the repo has a broken symref, or some weird\nfilesystem error occurred, but either way it may be best not to\ncontinue). I dunno.\n\nThese are all independent, so can be applied in any order or combination\nwith respect to each other and to your patch.\n\n [1/4]: test-ref-store: avoid passing NULL to printf\n [2/4]: remote: handle broken symrefs\n [3/4]: log: handle broken HEAD in decoration check\n [4/4]: worktree: handle broken symrefs in find_shared_symref()\n\n builtin/remote.c | 2 +-\n log-tree.c | 2 +-\n t/helper/test-ref-store.c | 2 +-\n worktree.c | 3 ++-\n 4 files changed, 5 insertions(+), 4 deletions(-)\n\n-Peff\n"},{"id":"330649","messageId":"20171019174620.4ajc3yxogzdcqksj@sigill.intra.peff.net","threadId":"46994","inReplyTo":"20171019174452.hd3c47ocducddvgr@sigill.intra.peff.net","subject":"[PATCH 1/4] test-ref-store: avoid passing NULL to printf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T17:46:21Z","receivedAt":"2017-10-19T17:46:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's possible for resolve_ref_unsafe() to return NULL (e.g.,\nif we are reading and the ref does not exist), in which case\nwe'll pass NULL to printf. On glibc systems this produces\n\"(null)\", but on others it may segfault.\n\nThe tests don't expect any such case, but if we ever did\ntrigger this, we would prefer to cleanly fail the test with\nunexpected input rather than segfault. Let's manually\nreplace NULL with \"(null)\". The exact value doesn't matter,\nas it won't match any possible ref the caller could expect\n(and anyway, the exit code of the program will tell whether\n\"ref\" is valid or not).\n\nSigned-off-by: Jeff King \n---\n t/helper/test-ref-store.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 05d8c4d8af..6ec2670044 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -135,7 +135,7 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)\n \n \tref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n \t\t\t\t sha1, &flags);\n-\tprintf(\"%s %s 0x%x\\n\", sha1_to_hex(sha1), ref, flags);\n+\tprintf(\"%s %s 0x%x\\n\", sha1_to_hex(sha1), ref ? ref : \"(null)\", flags);\n \treturn ref ? 0 : 1;\n }\n \n-- \n2.15.0.rc1.560.g5f0609e481\n\n"},{"id":"330650","messageId":"20171019174730.urelfyc2kj37fokg@sigill.intra.peff.net","threadId":"46994","inReplyTo":"20171019174452.hd3c47ocducddvgr@sigill.intra.peff.net","subject":"[PATCH 2/4] remote: handle broken symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T17:47:30Z","receivedAt":"2017-10-19T17:47:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's possible for resolve_ref_unsafe() to return NULL with a\nREF_ISSYMREF flag if a symref points to a broken ref. In\nthis case, the read_remote_branches() function will segfault\npassing the name to xstrdup().\n\nThis is hard to trigger in practice, since this function is\nused as a callback to for_each_ref(), which will skip broken\nrefs in the first place (so it would have to be broken\nracily, or for us to see a transient filesystem error).\n\nIf we see such a racy broken outcome let's treat it as \"not\na symref\". This is exactly the same thing that would happen\nin the non-racy case (our function would not be called at\nall, as for_each_ref would skip the broken symref).\n\nSigned-off-by: Jeff King \n---\n builtin/remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 4f5cac96b0..bc89623695 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -565,7 +565,7 @@ static int read_remote_branches(const char *refname,\n \t\titem = string_list_append(rename->remote_branches, xstrdup(refname));\n \t\tsymref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,\n \t\t\t\t\t NULL, &flag);\n-\t\tif (flag & REF_ISSYMREF)\n+\t\tif (symref && (flag & REF_ISSYMREF))\n \t\t\titem->util = xstrdup(symref);\n \t\telse\n \t\t\titem->util = NULL;\n-- \n2.15.0.rc1.560.g5f0609e481\n\n"},{"id":"330652","messageId":"20171019174900.4a5qypeabbt4mgwc@sigill.intra.peff.net","threadId":"46994","inReplyTo":"20171019174452.hd3c47ocducddvgr@sigill.intra.peff.net","subject":"[PATCH 3/4] log: handle broken HEAD in decoration check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T17:49:01Z","receivedAt":"2017-10-19T17:49:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The resolve_ref_unsafe() function may return NULL even with\na REF_ISSYMREF flag if a symref points to a broken ref. As a\nresult, it's possible for the decoration code's \"is this\nbranch the current HEAD\" check to segfault when it passes\nthe NULL to starts_with().\n\nThis is unlikely in practice, since we can only reach this\ncode if we already resolved HEAD to a matching sha1 earlier.\nBut it's possible if HEAD racily becomes broken, or if\nthere's a transient filesystem error.\n\nWe can fix this by returning early in the broken case, since\nNULL could not possibly match any of our branch names.\n\nSigned-off-by: Jeff King \n---\n log-tree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex cea056234d..580b3a98a0 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -198,7 +198,7 @@ static const struct name_decoration *current_pointed_by_HEAD(const struct name_d\n \n \t/* Now resolve and find the matching current branch */\n \tbranch_name = resolve_ref_unsafe(\"HEAD\", 0, NULL, &rru_flags);\n-\tif (!(rru_flags & REF_ISSYMREF))\n+\tif (!branch_name || !(rru_flags & REF_ISSYMREF))\n \t\treturn NULL;\n \n \tif (!starts_with(branch_name, \"refs/\"))\n-- \n2.15.0.rc1.560.g5f0609e481\n\n"},{"id":"330653","messageId":"20171019174936.izojvrh5w35s3adi@sigill.intra.peff.net","threadId":"46994","inReplyTo":"20171019174452.hd3c47ocducddvgr@sigill.intra.peff.net","subject":"[PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T17:49:36Z","receivedAt":"2017-10-19T17:49:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The refs_resolve_ref_unsafe() function may return NULL even\nwith a REF_ISSYMREF flag if a symref points to a broken ref.\nAs a result, it's possible for find_shared_symref() to\nsegfault when it passes NULL to strcmp().\n\nThis is hard to trigger for most code paths. We typically\npass HEAD to the function as the symref to resolve, and\nprograms like \"git branch\" will bail much earlier if HEAD\nisn't valid.\n\nI did manage to trigger it through one very obscure\nsequence:\n\n # You have multiple notes refs which conflict.\n git notes add -m base\n git notes --ref refs/notes/foo add -m foo\n\n # There's left-over cruft in NOTES_MERGE_REF that\n # makes it a broken symref (in this case we point\n # to a syntactically invalid ref).\n echo \"ref: refs/heads/master.lock\" >.git/NOTES_MERGE_REF\n\n # You try to merge the notes. We read the broken value in\n # order to complain that another notes-merge is\n # in-progress, but we segfault in find_shared_symref().\n git notes merge refs/notes/foo\n\nThis is obviously silly and almost certainly impossible to\ntrigger accidentally, but it does show that the bug is\ntriggerable from at least one code path. In addition, it\nwould trigger if we saw a transient filesystem error when\nresolving the pointed-to ref.\n\nWe can fix this by treating NULL the same as a non-matching\nsymref. Arguably we'd prefer to tell know if a symref points\nto \"refs/heads/foo\", but \"refs/heads/foo\" is broken. But\nrefs_resolve_ref_unsafe() isn't capable of giving us that\ninformation, so this is the best we can do.\n\nSigned-off-by: Jeff King \n---\n worktree.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex 70015629dc..f8c40f2f5f 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -327,7 +327,8 @@ const struct worktree *find_shared_symref(const char *symref,\n \t\trefs = get_worktree_ref_store(wt);\n \t\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n \t\t\t\t\t\t\tNULL, &flags);\n-\t\tif ((flags & REF_ISSYMREF) && !strcmp(symref_target, target)) {\n+\t\tif ((flags & REF_ISSYMREF) &&\n+\t\t symref_target && !strcmp(symref_target, target)) {\n \t\t\texisting = wt;\n \t\t\tbreak;\n \t\t}\n-- \n2.15.0.rc1.560.g5f0609e481\n"},{"id":"330654","messageId":"20171019175315.aewiewjk3fvhifa4@sigill.intra.peff.net","threadId":"46994","inReplyTo":"20171019174730.urelfyc2kj37fokg@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] remote: handle broken symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T17:53:16Z","receivedAt":"2017-10-19T17:53:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 01:47:30PM -0400, Jeff King wrote:\n\n> This is hard to trigger in practice, since this function is\n> used as a callback to for_each_ref(), which will skip broken\n> refs in the first place (so it would have to be broken\n> racily, or for us to see a transient filesystem error).\n> \n> If we see such a racy broken outcome let's treat it as \"not\n> a symref\". This is exactly the same thing that would happen\n> in the non-racy case (our function would not be called at\n> all, as for_each_ref would skip the broken symref).\n\nThe fact that we have to re-resolve the ref here to find the symref\npoints to a short-coming in the for_each_ref() interface. It resolved\nthe ref already to get us the oid, so it should (or at least could) know\nthe symref details already. But it doesn't record them or make them\navailable to callers.\n\nDitto for patch 3. It doesn't use for_each_ref(), but I suspect it could\nbe recording the value of HEAD more carefully from the prior lookup,\navoiding the re-resolution completely.\n\nRefactoring for_each_ref() is probably a bit big for a #leftoverbits,\nbut looking into the case in patch 3 might not be.\n\n-Peff\n"},{"id":"330722","messageId":"19dc9ff5-54f3-290a-2dc8-c0be8e4047eb@samsung.com","threadId":"46994","inReplyTo":"20171019174452.hd3c47ocducddvgr@sigill.intra.peff.net","subject":"Re: [PATCH v2] commit: check result of resolve_ref_unsafe","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-20T10:40:40Z","receivedAt":"2017-10-20T10:41:09Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"\n19.10.2017 20:44, Jeff King wrote:\n> On Thu, Oct 19, 2017 at 12:36:50PM +0300, Andrey Okoshkin wrote:\n> \n> Thanks, this looks good to me. One other possible minor improvement:\n> \n>> \thead = resolve_ref_unsafe(\"HEAD\", 0, junk_oid.hash, NULL);\n>> +\tif (!head)\n>> +\t\tdie(_(\"unable to resolve HEAD after creating commit\"));\n> \n> Should we use die_errno() here to report the value of errno? I think\n> resolve_ref_unsafe() should set it consistently (even an internal\n> problem, like an illegally-formatted refname, yields EINVAL).\nThanks, sounds relevant.\nAlso as an alternative solution it's possible to use warning_errno \n(instead of die_errno) and continue with 'head' set to something like\n'unreadable|bad HEAD'.\n\n> I grepped the code base looking for other instances of the same problem,\n> and found four of them. Patches to follow.\n> \n> Unlike this one, I ended up quietly returning an error in most cases.\n> The individual commit messages discuss the reasoning for each case, but\n> I do wonder if we ought to simply die() in each case out of an abundance\n> of caution (either the repo has a broken symref, or some weird\n> filesystem error occurred, but either way it may be best not to\n> continue). I dunno.\n> \n> These are all independent, so can be applied in any order or combination\n> with respect to each other and to your patch.\n> \n> [1/4]: test-ref-store: avoid passing NULL to printf\n> [2/4]: remote: handle broken symrefs\n> [3/4]: log: handle broken HEAD in decoration check\n> [4/4]: worktree: handle broken symrefs in find_shared_symref()\nGood changes, it's better to apply.\n\n-- \nBest regards,\nAndrey\n"},{"id":"330724","messageId":"c5f72436-df4e-5c51-d58b-8699dfcfd70c@samsung.com","threadId":"46994","inReplyTo":"20171019174452.hd3c47ocducddvgr@sigill.intra.peff.net","subject":"[PATCH v3] commit: check result of resolve_ref_unsafe","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-20T11:03:28Z","receivedAt":"2017-10-20T11:03:38Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Add check of the resolved HEAD reference while printing of a commit summary.\nresolve_ref_unsafe() may return NULL pointer if underlying calls of lstat() or\nopen() fail in files_read_raw_ref().\nSuch situation can be caused by race: file becomes inaccessible to this moment.\n\nSigned-off-by: Andrey Okoshkin \n---\ndie_errno fits better here as resolve_ref_unsafe preserves errno value.\n\nChanges since the previous patch:\n* die is replaced with die_errno.\n builtin/commit.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1a0da71a4..b52829090 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1483,6 +1483,8 @@ static void print_summary(const char *prefix, const struct object_id *oid,\n \tdiff_setup_done(&rev.diffopt);\n \n \thead = resolve_ref_unsafe(\"HEAD\", 0, junk_oid.hash, NULL);\n+\tif (!head)\n+\t\tdie_errno(_(\"unable to resolve HEAD after creating commit\"));\n \tif (!strcmp(head, \"HEAD\"))\n \t\thead = _(\"detached HEAD\");\n \telse\n-- \n2.14.2\n\n"},{"id":"330728","messageId":"a9cce39c-e432-d5a7-d9ad-6b67d9e04d23@samsung.com","threadId":"46994","inReplyTo":"c5f72436-df4e-5c51-d58b-8699dfcfd70c@samsung.com","subject":"[PATCH v4] commit: check result of resolve_ref_unsafe","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-20T13:09:30Z","receivedAt":"2017-10-20T13:09:41Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Add check of the resolved HEAD reference while printing of a commit summary.\nresolve_ref_unsafe() may return NULL pointer if underlying calls of lstat() or\nopen() fail in files_read_raw_ref().\nSuch situation can be caused by race: file becomes inaccessible to this moment.\n\nSigned-off-by: Andrey Okoshkin \n---\nHello,\nI think this way is better for user experience:\n* git doesn't crash;\n* warning is shown;\n* commit has been successfully created then it's safe to show a summary message\nwith already known information and without resolved HEAD.\n\nBest regards,\nAndrey\n\n builtin/commit.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1a0da71a4..647d6ab3e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1483,7 +1483,10 @@ static void print_summary(const char *prefix, const struct object_id *oid,\n \tdiff_setup_done(&rev.diffopt);\n \n \thead = resolve_ref_unsafe(\"HEAD\", 0, junk_oid.hash, NULL);\n-\tif (!strcmp(head, \"HEAD\"))\n+\tif (!head) {\n+\t\twarning_errno(_(\"unable to resolve HEAD after creating commit\"));\n+\t\thead = _(\"unresolvable HEAD\");\n+\t} else if (!strcmp(head, \"HEAD\"))\n \t\thead = _(\"detached HEAD\");\n \telse\n \t\tskip_prefix(head, \"refs/heads/\", &head);\n-- \n2.14.2\n"},{"id":"330776","messageId":"20171021061921.xtocfdfik6ehhwzw@sigill.intra.peff.net","threadId":"46994","inReplyTo":"a9cce39c-e432-d5a7-d9ad-6b67d9e04d23@samsung.com","subject":"Re: [PATCH v4] commit: check result of resolve_ref_unsafe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-21T06:19:21Z","receivedAt":"2017-10-21T06:19:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 20, 2017 at 04:09:30PM +0300, Andrey Okoshkin wrote:\n\n> Add check of the resolved HEAD reference while printing of a commit summary.\n> resolve_ref_unsafe() may return NULL pointer if underlying calls of lstat() or\n> open() fail in files_read_raw_ref().\n> Such situation can be caused by race: file becomes inaccessible to this moment.\n> \n> Signed-off-by: Andrey Okoshkin \n> ---\n> Hello,\n> I think this way is better for user experience:\n> * git doesn't crash;\n> * warning is shown;\n> * commit has been successfully created then it's safe to show a summary message\n> with already known information and without resolved HEAD.\n\nI'm on the fence between this and the die_errno() version. Given that\nthis would basically never happen in practice, I don't think it matters\ntoo much. And that makes me want to just err on the side of simplicity.\nBut it's not like this is all that complex, either.\n\n-Peff\n"},{"id":"330786","messageId":"CAPig+cT3=BNcM+WZWhpAq6Ch=5xC47oW_S-9=YfEQL7rRkyPhw@mail.gmail.com","threadId":"46994","inReplyTo":"20171019174936.izojvrh5w35s3adi@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-21T10:49:15Z","receivedAt":"2017-10-21T10:49:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 19, 2017 at 1:49 PM, Jeff King wrote:\n> The refs_resolve_ref_unsafe() function may return NULL even\n> with a REF_ISSYMREF flag if a symref points to a broken ref.\n> As a result, it's possible for find_shared_symref() to\n> segfault when it passes NULL to strcmp().\n> [...]\n> We can fix this by treating NULL the same as a non-matching\n> symref. Arguably we'd prefer to tell know if a symref points\n\ns/tell//\n\n> to \"refs/heads/foo\", but \"refs/heads/foo\" is broken. But\n> refs_resolve_ref_unsafe() isn't capable of giving us that\n> information, so this is the best we can do.\n>\n> Signed-off-by: Jeff King \n"},{"id":"330789","messageId":"20171021192601.2nvkmzsctujzxjbm@sigill.intra.peff.net","threadId":"46994","inReplyTo":"CAPig+cT3=BNcM+WZWhpAq6Ch=5xC47oW_S-9=YfEQL7rRkyPhw@mail.gmail.com","subject":"Re: [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-21T19:26:01Z","receivedAt":"2017-10-21T19:26:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 21, 2017 at 06:49:15AM -0400, Eric Sunshine wrote:\n\n> On Thu, Oct 19, 2017 at 1:49 PM, Jeff King wrote:\n> > The refs_resolve_ref_unsafe() function may return NULL even\n> > with a REF_ISSYMREF flag if a symref points to a broken ref.\n> > As a result, it's possible for find_shared_symref() to\n> > segfault when it passes NULL to strcmp().\n> > [...]\n> > We can fix this by treating NULL the same as a non-matching\n> > symref. Arguably we'd prefer to tell know if a symref points\n> \n> s/tell//\n\nRight, thank you.\n\n-Peff\n"},{"id":"330791","messageId":"xmqqd15gf1re.fsf@gitster.mtv.corp.google.com","threadId":"46994","inReplyTo":"20171021192601.2nvkmzsctujzxjbm@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-22T00:46:13Z","receivedAt":"2017-10-22T00:46:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Oct 21, 2017 at 06:49:15AM -0400, Eric Sunshine wrote:\n>\n>> On Thu, Oct 19, 2017 at 1:49 PM, Jeff King wrote:\n>> > The refs_resolve_ref_unsafe() function may return NULL even\n>> > with a REF_ISSYMREF flag if a symref points to a broken ref.\n>> > As a result, it's possible for find_shared_symref() to\n>> > segfault when it passes NULL to strcmp().\n>> > [...]\n>> > We can fix this by treating NULL the same as a non-matching\n>> > symref. Arguably we'd prefer to tell know if a symref points\n>> \n>> s/tell//\n>\n> Right, thank you.\n>\n> -Peff\n\nThanks. Already tweaked in.\n"},{"id":"330792","messageId":"xmqq8tg4f1qb.fsf@gitster.mtv.corp.google.com","threadId":"46994","inReplyTo":"20171021061921.xtocfdfik6ehhwzw@sigill.intra.peff.net","subject":"Re: [PATCH v4] commit: check result of resolve_ref_unsafe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-22T00:46:52Z","receivedAt":"2017-10-22T00:47:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Hello,\n>> I think this way is better for user experience:\n>> * git doesn't crash;\n>> * warning is shown;\n>> * commit has been successfully created then it's safe to show a summary message\n>> with already known information and without resolved HEAD.\n>\n> I'm on the fence between this and the die_errno() version. Given that\n> this would basically never happen in practice, I don't think it matters\n> too much. And that makes me want to just err on the side of simplicity.\n> But it's not like this is all that complex, either.\n\nTrue. I've queued v3 for now.\n\nThanks, both.\n"}]}