threads / discuss / 22795

Re: git-push segfault

Subject: Re: git-push segfault

## tl;dr

4 messages between Feb 24, 2010 and Feb 24, 2010.

replies: 3people: 4as markdown or json

ddrowley3@juno.com· Feb 24, 2010, 16:27 UTC · lore

> 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=

Jeff King· Feb 24, 2010, 17:08 UTC · re: ddrowley3@juno.com · lore

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

Show 10 quoted lines

> ./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

Daniel Barkalow· Feb 24, 2010, 18:05 UTC · re: Jeff King · lore

On Wed, 24 Feb 2010, Jeff King wrote:

Show 25 quoted lines

> 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.

Show 34 quoted lines

> -- >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
> 
>

Junio C Hamano· Feb 24, 2010, 19:26 UTC · re: Jeff King · lore

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.