git-push segfault

git-push segfault

[Dale Rowley]

Steps to reproduce:

git clone /path/to/some/repo
cd repo
git config --unset branch.master.remote
git push
(seg fault)

I'm using version 1.7.0.

Dale

Re: git-push segfault

[Dale Rowley]

Dale Rowley <ddrowley3 <at> juno.com> writes:

> git config --unset branch.master.remote
> git push

Yes, those 2 commands don't make much sense. I should have mentioned that this
was just the simplest way to reproduce the problem. My original and more
realistic scenario:

git clone /path/to/some/repo
cd repo
git remote add remote1 <url>
git remote add remote2 <url>
git config --unset branch.master.remote
git push remote1
(seg fault)

The reason for unsetting branch.master.remote was to force git to complain and
abort if I accidentally typed 'git push' instead of 'git push <remote>' (I
wanted to avoid accidentally pushing to the wrong remote repo).

Dale

Re: git-push segfault

[Jeff King]

On Wed, Feb 24, 2010 at 03:46:37AM +0000, Dale Rowley wrote:

> Steps to reproduce:
> 
> git clone /path/to/some/repo
> cd repo
> git config --unset branch.master.remote
> git push
> (seg fault)
> 
> I'm using version 1.7.0.

I can't reproduce here, using the following script:

-- >8 --
#!/bin/sh

rm -rf parent child

mkdir parent && (
  cd parent &&
  git init
) &&
git clone parent child &&
cd child &&
git config --unset branch.master.remote &&
git push
-- 8< --

Does that trigger the problem for you? If not, can you show what is
different between your earlier reproduction recipe and this one?

-Peff

Re: git-push segfault

[Andreas Krey]

On Wed, 24 Feb 2010 01:36:06 +0000, Jeff King wrote:
...
> #!/bin/sh
...
> git clone parent child &&
> cd child &&

How about 'set -xe' instead of all the &&?

Andreas

Re: git-push segfault

[Jeff King]

On Wed, Feb 24, 2010 at 09:22:14AM +0100, Andreas Krey wrote:

> On Wed, 24 Feb 2010 01:36:06 +0000, Jeff King wrote:
> ...
> > #!/bin/sh
> ...
> > git clone parent child &&
> > cd child &&
> 
> How about 'set -xe' instead of all the &&?

I long ago stopped trying to remember all of the places where "-e" works
and where it doesn't, and just use explicit "&&" (or "|| exit 1"). For
example, the subshell in my recipe needs an explicit error check under
dash, but will exit correctly using bash. I seem to recall there are
some other gotchas, but it has been a long time since I looked.

At any rate, this is not even code for inclusion in git. It is "please
cut and paste this and tell me if it breaks". So I am not too worried
about style.

-Peff

Re: git-push segfault

[Andreas Krey]

On Wed, 24 Feb 2010 04:23:57 +0000, Jeff King wrote:
> On Wed, Feb 24, 2010 at 09:22:14AM +0100, Andreas Krey wrote:
...
> > How about 'set -xe' instead of all the &&?
...
> At any rate, this is not even code for inclusion in git.

That was why I was asking. In throwaway scripts -e as well as -x is
rather practical (esp. the latter); I wouldn't use that in 'real'
scripts. Probably it's better not to set a bad example in posted
throwaway scripts, though. :-)

Andreas

Re: git-push segfault

[ddrowley3]

> Does that trigger the problem for you? If not, can you show what is
> different between your earlier reproduction recipe and this one?

Yes, same problem. Here's the output:

Initialized empty Git repository in /tmp/parent/.git/
Initialized empty Git repository in /tmp/child/.git/
warning: You appear to have cloned an empty repository.
./test.sh: line 12:  3686 Segmentation fault      (core dumped) git push

I reproduced this consistently on 2 different machines with 2 different versions of git, so it must have something to do with my ~/.gitconfig. Yep - if I remove the following from my .gitconfig, then the seg fault goes away:

[push]
    default = tracking


Dale


____________________________________________________________
Criminal Lawyer
Criminal Lawyers - Click here.
http://thirdpartyoffers.juno.com/TGL2131/c?cp=NyvAFq3GMltQMHm1-EJV8wAAJz1C6gppc1sGnWt7Fqwc41TnAAYAAAAAAAAAAAAAAAAAAADNAAAAAAAAAAAAAAAAAAAiFgAAAAA=

Re: git-push segfault

[Jeff King]

On Wed, Feb 24, 2010 at 04:27:28PM +0000, ddrowley3@juno.com wrote:

> ./test.sh: line 12:  3686 Segmentation fault      (core dumped) git
> push
> 
> I reproduced this consistently on 2 different machines with 2
> different versions of git, so it must have something to do with my
> ~/.gitconfig. Yep - if I remove the following from my .gitconfig, then
> the seg fault goes away:
> 
> [push]
>     default = tracking

Thanks, I can see it now. The patch below should fix it.

Note, however, that you will still get a failure for "git push remote",
as your config is set up to push tracking branches by default, and you
don't have one here.

I'm a little unsure of the patch. Arguably branch_get should not be
setting branch->merge_nr to 1, as there is nothing in branch->merge. On
the other hand, branch->merge_name _does_ have one element, so perhaps
it is an error in the caller to assume that branch->merge_nr and
branch->merge necessarily correspond. Daniel, this looks like your code.
Comments?

-- >8 --
Subject: [PATCH] push: fix segfault for odd config

If you have a branch.$X.merge config option, but no
branch.$X.remote, and your configuration tries to push
tracking branches, git will segfault.

The problem is that even though branch->merge_nr is 1, you
don't actually have an upstream since there is no remote.
Other callsites generally check explicitly that
branch->merge is not NULL, so let's do that here, too.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-push.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 5633f0a..f7bc2b2 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -68,7 +68,7 @@ static void setup_push_tracking(void)
 	struct branch *branch = branch_get(NULL);
 	if (!branch)
 		die("You are not currently on a branch.");
-	if (!branch->merge_nr)
+	if (!branch->merge_nr || !branch->merge)
 		die("The current branch %s is not tracking anything.",
 		    branch->name);
 	if (branch->merge_nr != 1)
-- 
1.7.0.215.g2da3b.dirty

Re: git-push segfault

[Daniel Barkalow]

On Wed, 24 Feb 2010, Jeff King wrote:

> On Wed, Feb 24, 2010 at 04:27:28PM +0000, ddrowley3@juno.com wrote:
> 
> > ./test.sh: line 12:  3686 Segmentation fault      (core dumped) git
> > push
> > 
> > I reproduced this consistently on 2 different machines with 2
> > different versions of git, so it must have something to do with my
> > ~/.gitconfig. Yep - if I remove the following from my .gitconfig, then
> > the seg fault goes away:
> > 
> > [push]
> >     default = tracking
> 
> Thanks, I can see it now. The patch below should fix it.
> 
> Note, however, that you will still get a failure for "git push remote",
> as your config is set up to push tracking branches by default, and you
> don't have one here.
> 
> I'm a little unsure of the patch. Arguably branch_get should not be
> setting branch->merge_nr to 1, as there is nothing in branch->merge. On
> the other hand, branch->merge_name _does_ have one element, so perhaps
> it is an error in the caller to assume that branch->merge_nr and
> branch->merge necessarily correspond. Daniel, this looks like your code.
> Comments?

I think it would be worthwhile to have branch->merge, branch->merge_nr, 
and branch->merge_name all match, even if branch->remote is NULL, by 
discarding the merge_name if the remote isn't specified.

It might also be wise to check branch_has_merge_config() instead of the 
test below, in any case, to match how the other code paths determine 
whether there is tracking configured.

> -- >8 --
> Subject: [PATCH] push: fix segfault for odd config
> 
> If you have a branch.$X.merge config option, but no
> branch.$X.remote, and your configuration tries to push
> tracking branches, git will segfault.
> 
> The problem is that even though branch->merge_nr is 1, you
> don't actually have an upstream since there is no remote.
> Other callsites generally check explicitly that
> branch->merge is not NULL, so let's do that here, too.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin-push.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin-push.c b/builtin-push.c
> index 5633f0a..f7bc2b2 100644
> --- a/builtin-push.c
> +++ b/builtin-push.c
> @@ -68,7 +68,7 @@ static void setup_push_tracking(void)
>  	struct branch *branch = branch_get(NULL);
>  	if (!branch)
>  		die("You are not currently on a branch.");
> -	if (!branch->merge_nr)
> +	if (!branch->merge_nr || !branch->merge)
>  		die("The current branch %s is not tracking anything.",
>  		    branch->name);
>  	if (branch->merge_nr != 1)
> -- 
> 1.7.0.215.g2da3b.dirty
> 
> 

Re: git-push segfault

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... On
> the other hand, branch->merge_name _does_ have one element, so perhaps
> it is an error in the caller to assume that branch->merge_nr and
> branch->merge necessarily correspond.

I think this is a sensible thing to do.   branch_has_merge_config() could
be used here but I do not see a point, as the code already knows branch is
non NULL and there are many other places that checks !!branch->merge
themselves.

It may be worth adding a helper function that a caller can lazily sanity
check the set of configuration around a given branch, but that sanity
check will probably go way beyond what branch_has_merge_config() currently
does, so I would think that would be a separate patch after somebody
audits what current non-users of branch_has_merge_config() want.  For
example, builtin-branch.c not only makes sure branch->merge is non NULL
but also wants branch->merge[0] and branch->merge[0]->dst exists, and
encapsulating only the first two out of four checks it does by using
branch_has_merge_config() does not add much value to it..

Thanks.