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

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

[Petter Urkedal]

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

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

[Jeff King]

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

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

[Petter Urkedal]

[-- Attachment #1: Type: text/plain, Size: 1505 bytes --]

On 2009-10-04, Jeff King wrote:
> 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.

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:
> 
> > +			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).

[-- Attachment #2: showbranch-argv0.txt --]
[-- Type: text/plain, Size: 5885 bytes --]

1552 eideticdew /tmp/test-repo$ gdb /home/urkedal/proj-ext/git/git show-branch
GNU gdb 6.8
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-pc-linux-gnu"...
/tmp/test-repo/show-branch: No such file or directory.
(gdb) run show-branch
[Thread debugging using libthread_db enabled]
*** glibc detected *** /home/urkedal/proj-ext/git/git: corrupted double-linked list: 0x0000000000734340 ***
[New Thread 0x7f937d4c36f0 (LWP 15739)]
======= Backtrace: =========
/lib/libc.so.6[0x7f937c84e1f2]
/lib/libc.so.6[0x7f937c84e4a4]
/lib/libc.so.6[0x7f937c84f7fc]
/lib/libc.so.6(cfree+0x75)[0x7f937c84fb13]
/lib/libc.so.6[0x7f937c8407bf]
/home/urkedal/proj-ext/git/git[0x48c921]
/home/urkedal/proj-ext/git/git[0x48e617]
/home/urkedal/proj-ext/git/git[0x44e1d2]
/home/urkedal/proj-ext/git/git[0x4042a3]
/home/urkedal/proj-ext/git/git[0x40444d]
/lib/libc.so.6(__libc_start_main+0xe6)[0x7f937c7fd56e]
/home/urkedal/proj-ext/git/git[0x403d59]
======= Memory map: ========
00400000-004e8000 r-xp 00000000 fd:04 12700046                           /home/urkedal/proj-ext/git/git
006e7000-006e8000 r--p 000e7000 fd:04 12700046                           /home/urkedal/proj-ext/git/git
006e8000-006ed000 rw-p 000e8000 fd:04 12700046                           /home/urkedal/proj-ext/git/git
006ed000-00755000 rw-p 00000000 00:00 0                                  [heap]
7f9378000000-7f9378021000 rw-p 00000000 00:00 0 
7f9378021000-7f937c000000 ---p 00000000 00:00 0 
7f937c3c5000-7f937c3da000 r-xp 00000000 08:01 32689                      /lib64/libgcc_s.so.1
7f937c3da000-7f937c5d9000 ---p 00015000 08:01 32689                      /lib64/libgcc_s.so.1
7f937c5d9000-7f937c5da000 r--p 00014000 08:01 32689                      /lib64/libgcc_s.so.1
7f937c5da000-7f937c5db000 rw-p 00015000 08:01 32689                      /lib64/libgcc_s.so.1
7f937c5db000-7f937c5dd000 r-xp 00000000 08:01 33045                      /lib64/libdl-2.9.so
7f937c5dd000-7f937c7dd000 ---p 00002000 08:01 33045                      /lib64/libdl-2.9.so
7f937c7dd000-7f937c7de000 r--p 00002000 08:01 33045                      /lib64/libdl-2.9.so
7f937c7de000-7f937c7df000 rw-p 00003000 08:01 33045                      /lib64/libdl-2.9.so
7f937c7df000-7f937c91e000 r-xp 00000000 08:01 32884                      /lib64/libc-2.9.so
7f937c91e000-7f937cb1e000 ---p 0013f000 08:01 32884                      /lib64/libc-2.9.so
7f937cb1e000-7f937cb22000 r--p 0013f000 08:01 32884                      /lib64/libc-2.9.so
7f937cb22000-7f937cb23000 rw-p 00143000 08:01 32884                      /lib64/libc-2.9.so
7f937cb23000-7f937cb28000 rw-p 00000000 00:00 0 
7f937cb28000-7f937cb3d000 r-xp 00000000 08:01 32902                      /lib64/libpthread-2.9.so
7f937cb3d000-7f937cd3d000 ---p 00015000 08:01 32902                      /lib64/libpthread-2.9.so
7f937cd3d000-7f937cd3e000 r--p 00015000 08:01 32902                      /lib64/libpthread-2.9.so
7f937cd3e000-7f937cd3f000 rw-p 00016000 08:01 32902                      /lib64/libpthread-2.9.so
7f937cd3f000-7f937cd43000 rw-p 00000000 00:00 0 
7f937cd43000-7f937ce95000 r-xp 00000000 fd:00 1181123                    /usr/lib64/libcrypto.so.0.9.8
7f937ce95000-7f937d095000 ---p 00152000 fd:00 1181123                    /usr/lib64/libcrypto.so.0.9.8
7f937d095000-7f937d0a3000 r--p 00152000 fd:00 1181123                    /usr/lib64/libcrypto.so.0.9.8
7f937d0a3000-7f937d0bb000 rw-p 00160000 fd:00 1181123                    /usr/lib64/libcrypto.so.0.9.8
7f937d0bb000-7f937d0bf000 rw-p 00000000 00:00 0 
7f937d0bf000-7f937d0d3000 r-xp 00000000 08:01 32655                      /lib64/libz.so.1.2.3
7f937d0d3000-7f937d2d2000 ---p 00014000 08:01 32655                      /lib64/libz.so.1.2.3
7f937d2d2000-7f937d2d3000 r--p 00013000 08:01 32655                      /lib64/libz.so.1.2.3
7f937d2d3000-7f937d2d4000 rw-p 00014000 08:01 32655                      /lib64/libz.so.1.2.3
7f937d2d4000-7f937d2f0000 r-xp 00000000 08:01 33042                      /lib64/ld-2.9.so
7f937d4c3000-7f937d4c6000 rw-p 00000000 00:00 0 
7f937d4ed000-7f937d4ef000 rw-p 00000000 00:00 0 
7f937d4ef000-7f937d4f0000 r--p 0001b000 08:01 33042                      /lib64/ld-2.9.so
7f937d4f0000-7f937d4f1000 rw-p 0001c000 08:01 33042                      /lib64/ld-2.9.so
7fff94a38000-7fff94a4e000 rw-p 00000000 00:00 0                          [stack]
7fff94bff000-7fff94c00000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]

Program received signal SIGABRT, Aborted.
[Switching to Thread 0x7f937d4c36f0 (LWP 15739)]
0x00007f937c810536 in raise () from /lib/libc.so.6
(gdb) bt
#0  0x00007f937c810536 in raise () from /lib/libc.so.6
#1  0x00007f937c811723 in abort () from /lib/libc.so.6
#2  0x00007f937c84953f in ?? () from /lib/libc.so.6
#3  0x00007f937c84e1f2 in ?? () from /lib/libc.so.6
#4  0x00007f937c84e4a4 in ?? () from /lib/libc.so.6
#5  0x00007f937c84f7fc in ?? () from /lib/libc.so.6
#6  0x00007f937c84fb13 in free () from /lib/libc.so.6
#7  0x00007f937c8407bf in ?? () from /lib/libc.so.6
#8  0x000000000048c921 in get_packed_refs () at refs.c:234
#9  0x000000000048e617 in do_for_each_ref (base=0x3d7b <Address 0x3d7b out of bounds>, fn=0x3d7b, 
    trim=6, flags=-1, cb_data=0x7f937c8e71e0) at refs.c:598
#10 0x000000000044e1d2 in cmd_show_branch (ac=<value optimized out>, av=0x734348, prefix=0x0)
    at builtin-show-branch.c:478
#11 0x00000000004042a3 in handle_internal_command (argc=1, argv=0x7fff94a4b1e0) at git.c:249
#12 0x000000000040444d in main (argc=1, argv=0x7fff94a4b1e0) at git.c:436
(gdb) q

[-- Attachment #3: 0001-Reserve-a-slot-for-argv-0-in-default_arg.patch --]
[-- Type: text/plain, Size: 1512 bytes --]

From a6bea57b5f9e3ebca38afce7829922ccb8f7d24f Mon Sep 17 00:00:00 2001
From: Petter Urkedal <urkedal@nbi.dk>
Date: Sat, 3 Oct 2009 14:52:41 +0200
Subject: [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 |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 3510a86..81c477c 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -568,6 +568,10 @@ 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 +696,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

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

[Jeff King]

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

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

[Stephen Boyd]

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

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

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

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

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.

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

[Petter Urkedal]

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

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

[Jeff King]

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

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

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

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

[Petter Urkedal]

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

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

[Petter Urkedal]

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;