{"thread":{"id":"39914","subject":"[PATCH] receive-pack: crash when checking with non-exist HEAD","startedAt":"2015-07-22T01:49:40Z","lastAt":"2015-07-23T17:49:39Z","messageCount":4,"participants":["Jiang Xin","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"266600","messageId":"CANYiYbE3Vy_gtFMKTMw1wHLDhU758nXaJnKaNy6WMNRz0fjUnw@mail.gmail.com","threadId":"39914","inReplyTo":null,"subject":"[PATCH] receive-pack: crash when checking with non-exist HEAD","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2015-07-22T01:49:40Z","receivedAt":"2015-07-22T01:49:40Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"If HEAD of a repository points to a conflict reference, such as:\n\n* There exist a reference named 'refs/heads/jx/feature1', but HEAD\n points to 'refs/heads/jx', or\n\n* There exist a reference named 'refs/heads/feature', but HEAD points\n to 'refs/heads/feature/bad'.\n\nWhen we push to delete a reference for this repo, such as:\n\n git push /path/to/bad-head-repo.git :some/good/reference\n\nThe git-receive-pack process will crash.\n\nThis is because if HEAD points to a conflict reference, the function\n`resolve_refdup(\"HEAD\", ...)` does not return a valid reference name,\nbut a null buffer. Later matching the delete reference against the null\nbuffer will cause git-receive-pack crash.\n\nSigned-off-by: Jiang Xin \n---\nI'm not sure this email is well-formed for git-am. Because gmail changed\nit's auth policy, I can not use git send-email command line to send mail.\nYou may know, in our China, we can not visit google/gmail directly, I\nmust access the outside world use VPN!\n\n builtin/receive-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 94d0571..04cb5a1 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -911,7 +911,7 @@ static const char *update(struct command *cmd,\nstruct shallow_info *si)\n return \"deletion prohibited\";\n }\n\n- if (!strcmp(namespaced_name, head_name)) {\n+ if (head_name && !strcmp(namespaced_name, head_name)) {\n switch (deny_delete_current) {\n case DENY_IGNORE:\n break;\n-- \n2.5.0.rc2.34.gfbdeabf.dirty\n"},{"id":"266629","messageId":"xmqqr3o0q6k7.fsf@gitster.dls.corp.google.com","threadId":"39914","inReplyTo":"CANYiYbE3Vy_gtFMKTMw1wHLDhU758nXaJnKaNy6WMNRz0fjUnw@mail.gmail.com","subject":"Re: [PATCH] receive-pack: crash when checking with non-exist HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-22T20:30:00Z","receivedAt":"2015-07-22T20:30:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> If HEAD of a repository points to a conflict reference, such as:\n>\n> * There exist a reference named 'refs/heads/jx/feature1', but HEAD\n> points to 'refs/heads/jx', or\n>\n> * There exist a reference named 'refs/heads/feature', but HEAD points\n> to 'refs/heads/feature/bad'.\n>\n> When we push to delete a reference for this repo, such as:\n>\n> git push /path/to/bad-head-repo.git :some/good/reference\n>\n> The git-receive-pack process will crash.\n\nI see a similar \"if head_name is NULL, don't bother.\" check in\nis_ref_checked_out() so in that sense this is a correct fix to the\nimmediate problem. That check came from 986e8239 (receive-pack:\ndetect push to current branch of non-bare repo, 2008-11-08).\n\nThis is a tangent, but if HEAD points at an unborn branch that\ncannot be created, wouldn't all other things break? \n\nFor example, in order to \"git commit\" from such a state to create\nthe root commit on that branch, existing unrelated branches whose\nnames collide with the branch must be removed, which would mean one\nof two things, either (1) you end up losing many unrelated work, or\n(2) the command refuses to work, not letting you to record the\ncommit. Neither is satisfactory, but we seem to choose (2), which\nis at least the safer of the two:\n\n $ git checkout master\n $ git checkout --orphan master/1\n $ git commit -m foo\n fatal: cannot lock ref 'HEAD': 'refs/heads/master' exists;\n cannot create 'refs/heads/master/1'\n\nWe may want to avoid putting us in such a situation in the first\nplace. Giving \"checkout --orphan\" an extra check might be a simple\nsmall thing we can do, i.e.\n\n $ git checkout master\n $ git checkout --orphan master/1\n fatal: 'master' branch exists, cannot create 'master/1'\n\nBut I suspect it would not protect us from different avenues that\ncan cause this kind of thing; e.g. to prevent this:\n\n $ git branch -D next\n $ git checkout --orphan next/1\n $ git fetch origin master:refs/heads/next\n\ncreation of a ref \"refs/heads/next\" here must notice HEAD points\nat \"refs/heads/next/1\" (that does not yet exist) and do something\nintelligent about it.\n\n> builtin/receive-pack.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 94d0571..04cb5a1 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -911,7 +911,7 @@ static const char *update(struct command *cmd,\n> struct shallow_info *si)\n> return \"deletion prohibited\";\n> }\n>\n> - if (!strcmp(namespaced_name, head_name)) {\n> + if (head_name && !strcmp(namespaced_name, head_name)) {\n> switch (deny_delete_current) {\n> case DENY_IGNORE:\n> break;\n"},{"id":"266643","messageId":"20150723055817.GA26794@peff.net","threadId":"39914","inReplyTo":"xmqqr3o0q6k7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] receive-pack: crash when checking with non-exist HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-23T05:58:18Z","receivedAt":"2015-07-23T05:58:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 22, 2015 at 01:30:00PM -0700, Junio C Hamano wrote:\n\n> I see a similar \"if head_name is NULL, don't bother.\" check in\n> is_ref_checked_out() so in that sense this is a correct fix to the\n> immediate problem. That check came from 986e8239 (receive-pack:\n> detect push to current branch of non-bare repo, 2008-11-08).\n\nYeah, I think this patch makes sense for the same reason as the check in\n986e8239: if our HEAD ref does not point to a branch, we cannot possibly\nbe trying to delete it.\n\nI do think the check is a little racy, though I'm not sure it is easy to\nfix. E.g., imagine one client pushes to create refs/heads/master just as\nsomebody else is trying to delete it. I don't think this is much\ndifferent than the case where somebody redirects HEAD to\nrefs/heads/master as we are trying to delete it, though. The checks are\ninherently racy because they are not done under locks (you would need to\nlock both HEAD and refs/heads/master in that case). It's probably not a\nbig deal in practice.\n\n> This is a tangent, but if HEAD points at an unborn branch that\n> cannot be created, wouldn't all other things break? \n> \n> For example, in order to \"git commit\" from such a state to create\n> the root commit on that branch, existing unrelated branches whose\n> names collide with the branch must be removed, which would mean one\n> of two things, either (1) you end up losing many unrelated work, or\n> (2) the command refuses to work, not letting you to record the\n> commit. Neither is satisfactory, but we seem to choose (2), which\n> is at least the safer of the two:\n> \n> $ git checkout master\n> $ git checkout --orphan master/1\n> $ git commit -m foo\n> fatal: cannot lock ref 'HEAD': 'refs/heads/master' exists;\n> cannot create 'refs/heads/master/1'\n\nYeah, that seems sensible. I think the \"way out\" for the user here would\npresumably be:\n\n git symbolic-ref HEAD refs/heads/something-else\n\nthough of course they could also rename the other ref.\n\n> We may want to avoid putting us in such a situation in the first\n> place. Giving \"checkout --orphan\" an extra check might be a simple\n> small thing we can do, i.e.\n> \n> $ git checkout master\n> $ git checkout --orphan master/1\n> fatal: 'master' branch exists, cannot create 'master/1'\n> \n> But I suspect it would not protect us from different avenues that\n> can cause this kind of thing; e.g. to prevent this:\n> \n> $ git branch -D next\n> $ git checkout --orphan next/1\n> $ git fetch origin master:refs/heads/next\n> \n> creation of a ref \"refs/heads/next\" here must notice HEAD points\n> at \"refs/heads/next/1\" (that does not yet exist) and do something\n> intelligent about it.\n\nRight. You'd have to teach the is_refname_available() check to always\ncheck what HEAD points to, and consider it as \"taken\", even if the ref\nitself doesn't exist. But what about other symbolic refs? The\n\"refs/remotes/origin/HEAD\" symref may point to\n\"refs/remotes/origin/master\" even though \"refs/remotes/origin/master/1\"\nexists. I doubt that will cause real problems in practice, but it points\nout that special cases like \"the value of HEAD is magic and reserved\"\nwill later end up being insufficient as the code is extended.\n\nI think I'd be willing to simply punt on the whole thing as being too\nrare to come up in practice.\n\n-Peff\n"},{"id":"266675","messageId":"xmqqk2tqpxvw.fsf@gitster.dls.corp.google.com","threadId":"39914","inReplyTo":"20150723055817.GA26794@peff.net","subject":"Re: [PATCH] receive-pack: crash when checking with non-exist HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-23T17:49:39Z","receivedAt":"2015-07-23T17:49:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jul 22, 2015 at 01:30:00PM -0700, Junio C Hamano wrote:\n>\n>> For example, in order to \"git commit\" from such a state to create\n>> the root commit on that branch, existing unrelated branches whose\n>> names collide with the branch must be removed, which would mean one\n>> of two things, either (1) you end up losing many unrelated work, or\n>> (2) the command refuses to work, not letting you to record the\n>> commit. Neither is satisfactory, but we seem to choose (2), which\n>> is at least the safer of the two:\n>> \n>> $ git checkout master\n>> $ git checkout --orphan master/1\n>> $ git commit -m foo\n>> fatal: cannot lock ref 'HEAD': 'refs/heads/master' exists;\n>> cannot create 'refs/heads/master/1'\n>\n> Yeah, that seems sensible. I think the \"way out\" for the user here would\n> presumably be:\n>\n> git symbolic-ref HEAD refs/heads/something-else\n>\n> though of course they could also rename the other ref.\n\nI would have expected you to say\n\n git checkout --orphan something-else\n\nwhich should work and would be more intuitive ;-)\n\n> Right. You'd have to teach the is_refname_available() check to always\n> check what HEAD points to, and consider it as \"taken\", even if the ref\n> itself doesn't exist. But what about other symbolic refs? The\n> \"refs/remotes/origin/HEAD\" symref may point to\n> \"refs/remotes/origin/master\" even though \"refs/remotes/origin/master/1\"\n> exists. I doubt that will cause real problems in practice, but it points\n> out that special cases like \"the value of HEAD is magic and reserved\"\n> will later end up being insufficient as the code is extended.\n\nYes, we do not have a handy cache of all symrefs, and it is dubious\nif this issue is grave enough to warrant adding one.\n\n> I think I'd be willing to simply punt on the whole thing as being too\n> rare to come up in practice.\n\nI tend to agree.\n"}]}