[PATCH] Reserve a slot for argv[0] in default_arg.

Setting "av" to one slot before the allocated "default_arg" array causes
glibc abort with "free(): invalid next size (normal)" in some
configurations (Gentoo, glibc-2.9_p20081201-r2, gcc-5.3.2 with PIE).
---
 builtin-show-branch.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 3510a86..3ab72b7 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c

@@ -568,6 +568,9 @@ static int git_show_branch_config(const char *var, const char *value, void *cb)
 		if (default_alloc <= default_num + 1) {
 			default_alloc = default_alloc * 3 / 2 + 20;
 			default_arg = xrealloc(default_arg, sizeof *default_arg * default_alloc);
+			if (!default_num)
+			    /* One unused position for argv[0]. */
+			    default_arg[default_num++] = NULL;
 		}
 		default_arg[default_num++] = xstrdup(value);
 		default_arg[default_num] = NULL;

@@ -692,8 +695,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 	/* If nothing is specified, try the default first */
 	if (ac == 1 && default_num) {
-		ac = default_num + 1;
-		av = default_arg - 1; /* ick; we would not address av[0] */
+		ac = default_num;
+		av = default_arg;
 	}
 
 	ac = parse_options(ac, av, prefix, builtin_show_branch_options,

-- 
1.6.4.4

On Sun, Oct 04, 2009 at 04:13:55PM +0200, Petter Urkedal wrote:

I was wondering myself.  I tried to switch off optimisation, but that
had no effect.  I'm suspecting PIE, but it could be some other
configuration implied by the Gentoo "hardened" use-flag.

Nope, it's just a plain old git bug...

I can reproduce it on my machine with

    mkdir test-repo; cd test-repo
    /path/to/git init
    /path/to/git config showbranch.default --topo-order
    /path/to/git show-branch

Ah, thanks, for some reason I wasn't able to produce it before, but I
can easily replicate it here. I think it's a regression from converting
show-branch to use parse_options, which happened in May, but I didn't
actually bisect it. I'm not sure showbranch.default has worked at all
since then (which I guess goes to show how many people are actually
using it).

So your fix is definitely right, and the test case below (which can be
squashed in) fails reliably without it.

t3202 is maybe a bit of weird place to put it, but we don't seem to test
show-branch anywhere else. It could probably use a "check that
show-branch works at all" set of tests, but I am not volunteering to
write such a thing. I have always found its output to be one step above
line noise.

I also looked at putting it in t1200-tutorial.sh near the show-branch
call, but that script is an utter mess. Most of the tests don't actually
check the exit status of commands, and there is a random "test_done"
halfway through the script which skips all of the later tests (including
the show-branch test!). Removing that to enable the later tests reveals
that they are broken, with such obviously non-working crap as

  git merge -s "Merge upstream changes." master

which is clearly bogus. I wonder if we should just remove that script
altogether; at best it just seems redundant with other tests, and it is
full of obvious errors.

Comment's are treated as whitespace, but I'll adjust it for readability.
Maybe worse: I missed the 8-column indentation.  So, here is the patch
again (attached, I hope Git can extract it).

Thanks, that looks better (I actually didn't even notice the indent
problem the first time, but yes, it should be 8 columns).

Squashable test case is below.

-Peff

---

diff --git a/t/t3202-show-branch-octopus.sh b/t/t3202-show-branch-octopus.sh
index 7fe4a6e..0a5d5e6 100755
--- a/t/t3202-show-branch-octopus.sh
+++ b/t/t3202-show-branch-octopus.sh

@@ -56,4 +56,12 @@ test_expect_success 'show-branch with more than 8 branches' '
 
 '
 
+test_expect_success 'show-branch with showbranch.default' '
+	for i in $numbers; do
+		git config --add showbranch.default branch$i
+	done &&
+	git show-branch >out &&
+	test_cmp expect out
+'
+
 test_done

On Sun, 2009-10-04 at 14:27 -0400, Jeff King wrote:

Ah, thanks, for some reason I wasn't able to produce it before, but I
can easily replicate it here. I think it's a regression from converting
show-branch to use parse_options, which happened in May, but I didn't
actually bisect it. I'm not sure showbranch.default has worked at all
since then (which I guess goes to show how many people are actually
using it).

Correct. Junio sent a patch to fix this problem in June[1]. I guess he
must have dropped his own patch, or he wasn't satisfied with how parse
options clobbers things.

[1] http://article.gmane.org/gmane.comp.version-control.git/121142

Stephen Boyd [off-list ref] writes:

On Sun, 2009-10-04 at 14:27 -0400, Jeff King wrote:

quoted

Ah, thanks, for some reason I wasn't able to produce it before, but I
can easily replicate it here. I think it's a regression from converting
show-branch to use parse_options, which happened in May, but I didn't
actually bisect it. I'm not sure showbranch.default has worked at all
since then (which I guess goes to show how many people are actually
using it).

It is a command specific aliasing mechanism; not even I use the feature
these days, since "alias.*" is much easier to use.  But there is no strong
need to remove it either; it is not too much hassle to keep it for people
who do use it.  Perhaps deprecate it and remove it in the long run?

Correct. Junio sent a patch to fix this problem in June[1]. I guess he
must have dropped his own patch, or he wasn't satisfied with how parse
options clobbers things.

[1] http://article.gmane.org/gmane.comp.version-control.git/121142

I had it kept still in my Inbox; thanks for noticing.  Petter's patch does
essentially the same thing, but the old patch had a better log message
that described where in the history the fix should apply, so I'd probably
use that with your test squashed in.

Thanks.

On 2009-10-04, Junio C Hamano wrote:

It is a command specific aliasing mechanism; not even I use the feature
these days, since "alias.*" is much easier to use.  But there is no strong
need to remove it either; it is not too much hassle to keep it for people
who do use it.  Perhaps deprecate it and remove it in the long run?

I didn't know about alias.*.  Excellent.  I'll be using that.
 

quoted

Correct. Junio sent a patch to fix this problem in June[1]. I guess he
must have dropped his own patch, or he wasn't satisfied with how parse
options clobbers things.

[1] http://article.gmane.org/gmane.comp.version-control.git/121142

I had it kept still in my Inbox; thanks for noticing.  Petter's patch does
essentially the same thing, but the old patch had a better log message
that described where in the history the fix should apply, so I'd probably
use that with your test squashed in.

The code is slightly nicer to, I think, but you can probably drop "+ 20"
in the grow-case now.

On 2009-10-04, Jeff King wrote:

t3202 is maybe a bit of weird place to put it, but we don't seem to test
show-branch anywhere else. It could probably use a "check that
show-branch works at all" set of tests, but I am not volunteering to
write such a thing.

Looks good to me.  I'm not so familiar with the source code as I looked
at it first time when I submitted the patch.

I have always found its output to be one step above
line noise.

I agree show-branch is nosy, and the actual options I was adding was
"--topo-order master t/*" to show only topic branches.  After rebasing
them against master, show-branch with these arguments gives a nice
overview.  Thanks Junio's tip I now have

    alias.show-topics = show-branch --topo-order master t/*

On Mon, Oct 05, 2009 at 08:36:49AM +0200, Petter Urkedal wrote:

On 2009-10-04, Junio C Hamano wrote:

quoted

It is a command specific aliasing mechanism; not even I use the feature
these days, since "alias.*" is much easier to use.  But there is no strong
need to remove it either; it is not too much hassle to keep it for people
who do use it.  Perhaps deprecate it and remove it in the long run?

I didn't know about alias.*.  Excellent.  I'll be using that.

Yeah, showbranch.default really seems pointless now. Especially
confusing is the fact that it doesn't do whitespace-splitting, so you
can't do:

  git config showbranch.default "--topo-order branch1 branch2"

but instead have to set multiple config variables.

I think deprecation makes sense, but I am in no hurry to get rid of it.
I mainly just wouldn't want people to think it was a useful thing to
learn. :)

The code is slightly nicer to, I think, but you can probably drop "+ 20"
in the grow-case now.

I think it could actually just be switched to use ALLOC_GROW.

-Peff

On Sat, Oct 03, 2009 at 03:29:31PM +0200, Petter Urkedal wrote:

Setting "av" to one slot before the allocated "default_arg" array causes
glibc abort with "free(): invalid next size (normal)" in some
configurations (Gentoo, glibc-2.9_p20081201-r2, gcc-5.3.2 with PIE).

Thanks, your fix looks sane. But I am curious about whether we are
triggering some glibc pickiness that is in your setup, or if we are
somehow violating the assumption that we only ever look at
default_arg[1] and beyond.

What show-branch command did you issue to hit this? I was hoping to run
it under valgrind.

Also:

+			if (!default_num)
+			    /* One unused position for argv[0]. */
+			    default_arg[default_num++] = NULL;

I don't know if we have a style rule for comments on single line
conditionals, but I had to read this a few times to make sure it wasn't
missing braces.

-		ac = default_num + 1;
-		av = default_arg - 1; /* ick; we would not address av[0] */
+		ac = default_num;
+		av = default_arg;

Any time you can remove a comment with "ick" in it is probably a good
thing.  ;)

-Peff

On 2009-10-04, Jeff King wrote:

On Sat, Oct 03, 2009 at 03:29:31PM +0200, Petter Urkedal wrote:

quoted

Setting "av" to one slot before the allocated "default_arg" array causes
glibc abort with "free(): invalid next size (normal)" in some
configurations (Gentoo, glibc-2.9_p20081201-r2, gcc-5.3.2 with PIE).

Thanks, your fix looks sane. But I am curious about whether we are
triggering some glibc pickiness that is in your setup, or if we are
somehow violating the assumption that we only ever look at
default_arg[1] and beyond.

I was wondering myself.  I tried to switch off optimisation, but that
had no effect.  I'm suspecting PIE, but it could be some other
configuration implied by the Gentoo "hardened" use-flag.

What show-branch command did you issue to hit this? I was hoping to run
it under valgrind.

I can reproduce it on my machine with

    mkdir test-repo; cd test-repo
    /path/to/git init
    /path/to/git config showbranch.default --topo-order
    /path/to/git show-branch

Also:

quoted

+			if (!default_num)
+			    /* One unused position for argv[0]. */
+			    default_arg[default_num++] = NULL;

I don't know if we have a style rule for comments on single line
conditionals, but I had to read this a few times to make sure it wasn't
missing braces.

Comment's are treated as whitespace, but I'll adjust it for readability.
Maybe worse: I missed the 8-column indentation.  So, here is the patch
again (attached, I hope Git can extract it).

On 2009-10-04, Jeff King wrote:

Thanks, your fix looks sane. But I am curious about whether we are
triggering some glibc pickiness that is in your setup, or if we are
somehow violating the assumption that we only ever look at
default_arg[1] and beyond.

I had a look at parse-options.c.  In parse_options_start, argv is
assigned to ctx->out, which is overwritten from index 0 in
parse_options_end.  This will show the problem:

diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 3510a86..1c587ad 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c

@@ -691,6 +691,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
                showbranch_use_color = git_use_color_default;
 
        /* If nothing is specified, try the default first */
+       printf("default_arg = %p\n", default_arg);
        if (ac == 1 && default_num) {
                ac = default_num + 1;
                av = default_arg - 1; /* ick; we would not address av[0] */

diff --git a/parse-options.c b/parse-options.c
index f559411..267e752 100644
--- a/parse-options.c
+++ b/parse-options.c

@@ -435,6 +435,7 @@ unknown:
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
 {
+       printf("Assigning to %p\n", ctx->out + ctx->cpidx);
        memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
        ctx->out[ctx->cpidx + ctx->argc] = NULL;
        return ctx->cpidx + ctx->argc;