[PATCH] receive-pack: crash when checking with non-exist HEAD

[PATCH] receive-pack: crash when checking with non-exist HEAD

[Jiang Xin]

If HEAD of a repository points to a conflict reference, such as:

* There exist a reference named 'refs/heads/jx/feature1', but HEAD
  points to 'refs/heads/jx', or

* There exist a reference named 'refs/heads/feature', but HEAD points
  to 'refs/heads/feature/bad'.

When we push to delete a reference for this repo, such as:

        git push /path/to/bad-head-repo.git :some/good/reference

The git-receive-pack process will crash.

This is because if HEAD points to a conflict reference, the function
`resolve_refdup("HEAD", ...)` does not return a valid reference name,
but a null buffer.  Later matching the delete reference against the null
buffer will cause git-receive-pack crash.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
I'm not sure this email is well-formed for git-am. Because gmail changed
it's auth policy, I can not use git send-email command line to send mail.
You may know, in our China, we can not visit google/gmail directly, I
must access the outside world use VPN!

 builtin/receive-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 94d0571..04cb5a1 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -911,7 +911,7 @@ static const char *update(struct command *cmd,
struct shallow_info *si)
      return "deletion prohibited";
    }

-   if (!strcmp(namespaced_name, head_name)) {
+   if (head_name && !strcmp(namespaced_name, head_name)) {
      switch (deny_delete_current) {
      case DENY_IGNORE:
        break;
-- 
2.5.0.rc2.34.gfbdeabf.dirty

Re: [PATCH] receive-pack: crash when checking with non-exist HEAD

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> If HEAD of a repository points to a conflict reference, such as:
>
> * There exist a reference named 'refs/heads/jx/feature1', but HEAD
>   points to 'refs/heads/jx', or
>
> * There exist a reference named 'refs/heads/feature', but HEAD points
>   to 'refs/heads/feature/bad'.
>
> When we push to delete a reference for this repo, such as:
>
>         git push /path/to/bad-head-repo.git :some/good/reference
>
> The git-receive-pack process will crash.

I see a similar "if head_name is NULL, don't bother." check in
is_ref_checked_out() so in that sense this is a correct fix to the
immediate problem.  That check came from 986e8239 (receive-pack:
detect push to current branch of non-bare repo, 2008-11-08).

This is a tangent, but if HEAD points at an unborn branch that
cannot be created, wouldn't all other things break?  

For example, in order to "git commit" from such a state to create
the root commit on that branch, existing unrelated branches whose
names collide with the branch must be removed, which would mean one
of two things, either (1) you end up losing many unrelated work, or
(2) the command refuses to work, not letting you to record the
commit.  Neither is satisfactory, but we seem to choose (2), which
is at least the safer of the two:

    $ git checkout master
    $ git checkout --orphan master/1
    $ git commit -m foo
    fatal: cannot lock ref 'HEAD': 'refs/heads/master' exists;
    cannot create 'refs/heads/master/1'

We may want to avoid putting us in such a situation in the first
place.  Giving "checkout --orphan" an extra check might be a simple
small thing we can do, i.e.

    $ git checkout master
    $ git checkout --orphan master/1
    fatal: 'master' branch exists, cannot create 'master/1'

But I suspect it would not protect us from different avenues that
can cause this kind of thing; e.g. to prevent this:

    $ git branch -D next
    $ git checkout --orphan next/1
    $ git fetch origin master:refs/heads/next

creation of a ref "refs/heads/next" here must notice HEAD points
at "refs/heads/next/1" (that does not yet exist) and do something
intelligent about it.

>  builtin/receive-pack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 94d0571..04cb5a1 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -911,7 +911,7 @@ static const char *update(struct command *cmd,
> struct shallow_info *si)
>       return "deletion prohibited";
>     }
>
> -   if (!strcmp(namespaced_name, head_name)) {
> +   if (head_name && !strcmp(namespaced_name, head_name)) {
>       switch (deny_delete_current) {
>       case DENY_IGNORE:
>         break;

Re: [PATCH] receive-pack: crash when checking with non-exist HEAD

[Jeff King]

On Wed, Jul 22, 2015 at 01:30:00PM -0700, Junio C Hamano wrote:

> I see a similar "if head_name is NULL, don't bother." check in
> is_ref_checked_out() so in that sense this is a correct fix to the
> immediate problem.  That check came from 986e8239 (receive-pack:
> detect push to current branch of non-bare repo, 2008-11-08).

Yeah, I think this patch makes sense for the same reason as the check in
986e8239: if our HEAD ref does not point to a branch, we cannot possibly
be trying to delete it.

I do think the check is a little racy, though I'm not sure it is easy to
fix. E.g., imagine one client pushes to create refs/heads/master just as
somebody else is trying to delete it. I don't think this is much
different than the case where somebody redirects HEAD to
refs/heads/master as we are trying to delete it, though. The checks are
inherently racy because they are not done under locks (you would need to
lock both HEAD and refs/heads/master in that case). It's probably not a
big deal in practice.

> This is a tangent, but if HEAD points at an unborn branch that
> cannot be created, wouldn't all other things break?  
> 
> For example, in order to "git commit" from such a state to create
> the root commit on that branch, existing unrelated branches whose
> names collide with the branch must be removed, which would mean one
> of two things, either (1) you end up losing many unrelated work, or
> (2) the command refuses to work, not letting you to record the
> commit.  Neither is satisfactory, but we seem to choose (2), which
> is at least the safer of the two:
> 
>     $ git checkout master
>     $ git checkout --orphan master/1
>     $ git commit -m foo
>     fatal: cannot lock ref 'HEAD': 'refs/heads/master' exists;
>     cannot create 'refs/heads/master/1'

Yeah, that seems sensible. I think the "way out" for the user here would
presumably be:

  git symbolic-ref HEAD refs/heads/something-else

though of course they could also rename the other ref.

> We may want to avoid putting us in such a situation in the first
> place.  Giving "checkout --orphan" an extra check might be a simple
> small thing we can do, i.e.
> 
>     $ git checkout master
>     $ git checkout --orphan master/1
>     fatal: 'master' branch exists, cannot create 'master/1'
> 
> But I suspect it would not protect us from different avenues that
> can cause this kind of thing; e.g. to prevent this:
> 
>     $ git branch -D next
>     $ git checkout --orphan next/1
>     $ git fetch origin master:refs/heads/next
> 
> creation of a ref "refs/heads/next" here must notice HEAD points
> at "refs/heads/next/1" (that does not yet exist) and do something
> intelligent about it.

Right. You'd have to teach the is_refname_available() check to always
check what HEAD points to, and consider it as "taken", even if the ref
itself doesn't exist. But what about other symbolic refs? The
"refs/remotes/origin/HEAD" symref may point to
"refs/remotes/origin/master" even though "refs/remotes/origin/master/1"
exists. I doubt that will cause real problems in practice, but it points
out that special cases like "the value of HEAD is magic and reserved"
will later end up being insufficient as the code is extended.

I think I'd be willing to simply punt on the whole thing as being too
rare to come up in practice.

-Peff

Re: [PATCH] receive-pack: crash when checking with non-exist HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jul 22, 2015 at 01:30:00PM -0700, Junio C Hamano wrote:
>
>> For example, in order to "git commit" from such a state to create
>> the root commit on that branch, existing unrelated branches whose
>> names collide with the branch must be removed, which would mean one
>> of two things, either (1) you end up losing many unrelated work, or
>> (2) the command refuses to work, not letting you to record the
>> commit.  Neither is satisfactory, but we seem to choose (2), which
>> is at least the safer of the two:
>> 
>>     $ git checkout master
>>     $ git checkout --orphan master/1
>>     $ git commit -m foo
>>     fatal: cannot lock ref 'HEAD': 'refs/heads/master' exists;
>>     cannot create 'refs/heads/master/1'
>
> Yeah, that seems sensible. I think the "way out" for the user here would
> presumably be:
>
>   git symbolic-ref HEAD refs/heads/something-else
>
> though of course they could also rename the other ref.

I would have expected you to say

    git checkout --orphan something-else

which should work and would be more intuitive ;-)

> Right. You'd have to teach the is_refname_available() check to always
> check what HEAD points to, and consider it as "taken", even if the ref
> itself doesn't exist. But what about other symbolic refs? The
> "refs/remotes/origin/HEAD" symref may point to
> "refs/remotes/origin/master" even though "refs/remotes/origin/master/1"
> exists. I doubt that will cause real problems in practice, but it points
> out that special cases like "the value of HEAD is magic and reserved"
> will later end up being insufficient as the code is extended.

Yes, we do not have a handy cache of all symrefs, and it is dubious
if this issue is grave enough to warrant adding one.

> I think I'd be willing to simply punt on the whole thing as being too
> rare to come up in practice.

I tend to agree.