threads / patch / 17878

patchAvoid segfault with 'git branch' when the HEAD is detached

Subject: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached

## tl;dr

12 messages between Feb 18, 2009 and Feb 19, 2009. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Johannes Schindelin· Feb 18, 2009, 18:14 UTC · lore

A recent addition to the ref_item struct was not taken care of, leading to a segmentation fault when accessing the (uninitialized) "dest" member.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	Unfortunately not found by valgrind.
 builtin-branch.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Show changes to builtin-branch.c +2 −0
diff --git a/builtin-branch.c b/builtin-branch.c
index 8c37c78..ded0a82 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -441,7 +441,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
 	    is_descendant_of(head_commit, with_commit)) {
 		struct ref_item item;
 		item.name = xstrdup("(no branch)");
+		item.len = strlen(item.name);
 		item.kind = REF_LOCAL_BRANCH;
+		item.dest = NULL;
 		item.commit = head_commit;
 		if (strlen(item.name) > ref_list.maxwidth)
 			ref_list.maxwidth = strlen(item.name);

-- 
1.6.2.rc1.349.g70b801
Jay Soffian· Feb 18, 2009, 20:36 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached

On Wed, Feb 18, 2009 at 1:14 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

> A recent addition to the ref_item struct was not taken care of, leading
> to a segmentation fault when accessing the (uninitialized) "dest" member.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
ACK.
Thanks,
j.
Jeff King· Feb 19, 2009, 00:45 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached

On Wed, Feb 18, 2009 at 07:14:59PM +0100, Johannes Schindelin wrote:
Show 7 quoted lines
> A recent addition to the ref_item struct was not taken care of, leading
> to a segmentation fault when accessing the (uninitialized) "dest" member.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	Unfortunately not found by valgrind.

Meaning that the bug was created after your valgrind testing (which takes a painfully long time to run, and so only happens occasionally), and therefore you found it by hand? Or meaning that even running the test suite with valgrind did not reveal the problem?

If the latter, isn't that an indication that this code path was not being exercised by the test suite and it should be?

Now if only we had a way of measuring test coverage...
Show 12 quoted lines
> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> @@ -441,7 +441,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
>  	    is_descendant_of(head_commit, with_commit)) {
>  		struct ref_item item;
>  		item.name = xstrdup("(no branch)");
> +		item.len = strlen(item.name);
>  		item.kind = REF_LOCAL_BRANCH;
> +		item.dest = NULL;
>  		item.commit = head_commit;
>  		if (strlen(item.name) > ref_list.maxwidth)
>  			ref_list.maxwidth = strlen(item.name);
Maybe replace the repeated strlens below with item.len? I.e., squash in
Show changes to builtin-branch.c +2 −3
diff --git a/builtin-branch.c b/builtin-branch.c
index 13e4de8..14d4b91 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -443,8 +443,8 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
 		item.kind = REF_LOCAL_BRANCH;
 		item.dest = NULL;
 		item.commit = head_commit;
-		if (strlen(item.name) > ref_list.maxwidth)
-			ref_list.maxwidth = strlen(item.name);
+		if (item.len > ref_list.maxwidth)
+			ref_list.maxwidth = item.len;
 		print_ref_item(&item, ref_list.maxwidth, verbose, abbrev, 1, "");
 		free(item.name);
 	}

Other than that, patch looks obviously correct (and I did a quick scan
to see that there were no other locations).

-Peff
Johannes Schindelin· Feb 19, 2009, 01:15 UTC · re: Jeff King · lore

Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached

Hi,
On Wed, 18 Feb 2009, Jeff King wrote:
Show 14 quoted lines
> On Wed, Feb 18, 2009 at 07:14:59PM +0100, Johannes Schindelin wrote:
> 
> > A recent addition to the ref_item struct was not taken care of, leading
> > to a segmentation fault when accessing the (uninitialized) "dest" member.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > 
> > 	Unfortunately not found by valgrind.
> 
> Meaning that the bug was created after your valgrind testing (which 
> takes a painfully long time to run, and so only happens occasionally), 
> and therefore you found it by hand? Or meaning that even running the 
> test suite with valgrind did not reveal the problem?
It bit me.
IOW I had to fix it before I could finish up the work for the day.
> If the latter, isn't that an indication that this code path was not 
> being exercised by the test suite and it should be?

Like I said, I had to finish up some work for the day, that's why I did not have time to add a test.

> Now if only we had a way of measuring test coverage...

Yes, I also want the gcov series. Patience, grass hopper, patience: after 1.6.2.

Show 27 quoted lines
> > --- a/builtin-branch.c
> > +++ b/builtin-branch.c
> > @@ -441,7 +441,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
> >  	    is_descendant_of(head_commit, with_commit)) {
> >  		struct ref_item item;
> >  		item.name = xstrdup("(no branch)");
> > +		item.len = strlen(item.name);
> >  		item.kind = REF_LOCAL_BRANCH;
> > +		item.dest = NULL;
> >  		item.commit = head_commit;
> >  		if (strlen(item.name) > ref_list.maxwidth)
> >  			ref_list.maxwidth = strlen(item.name);
> 
> Maybe replace the repeated strlens below with item.len? I.e., squash in
> 
> diff --git a/builtin-branch.c b/builtin-branch.c
> index 13e4de8..14d4b91 100644
> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> @@ -443,8 +443,8 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
>  		item.kind = REF_LOCAL_BRANCH;
>  		item.dest = NULL;
>  		item.commit = head_commit;
> -		if (strlen(item.name) > ref_list.maxwidth)
> -			ref_list.maxwidth = strlen(item.name);
> +		if (item.len > ref_list.maxwidth)
> +			ref_list.maxwidth = item.len;

Yeah, I did not think of that. I checked that there are no other instances where a member of ref_item was uninitialized, and that took already more time than I had.

Ciao, Dscho

Jeff King· Feb 19, 2009, 03:24 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached

On Thu, Feb 19, 2009 at 02:15:00AM +0100, Johannes Schindelin wrote:
Show 8 quoted lines
> > Meaning that the bug was created after your valgrind testing (which 
> > takes a painfully long time to run, and so only happens occasionally), 
> > and therefore you found it by hand? Or meaning that even running the 
> > test suite with valgrind did not reveal the problem?
> 
> It bit me.
> 
> IOW I had to fix it before I could finish up the work for the day.

OK. I wasn't sure if it was "valgrind didn't find" or "valgrind wouldn't find". And your answer is "didn't", but as it turns out, it also "wouldn't.

Updated series with tests to follow.
-Peff
Jeff King· Feb 19, 2009, 03:33 UTC · re: Jeff King · lore

Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached

On Wed, Feb 18, 2009 at 10:24:20PM -0500, Jeff King wrote:
> Updated series with tests to follow.

Ah, it looks like Junio picked up your original patch. But I still think it is worth doing these on top:

  1/2: add basic branch display tests
  2/2: branch: clean up repeated strlen
-Peff
Jeff King· Feb 19, 2009, 03:34 UTC · re: Jeff King · lore

[PATCH 1/2] add basic branch display tests

We were not testing the output of "git branch" anywhere. Not only does this not protect us against regressions in the output, but we are not exercising code paths which may have bugs (such as the one fixed by 45e2b61).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3203-branch-output.sh |   81 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletions(-)
 create mode 100755 t/t3203-branch-output.sh
Show changes to t/t3203-branch-output.sh +81 −0
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
new file mode 100755
index 0000000..809d1c4
--- /dev/null
+++ b/t/t3203-branch-output.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+
+test_description='git branch display tests'
+. ./test-lib.sh
+
+test_expect_success 'make commits' '
+	echo content >file &&
+	git add file &&
+	git commit -m one &&
+	echo content >>file &&
+	git commit -a -m two
+'
+
+test_expect_success 'make branches' '
+	git branch branch-one
+	git branch branch-two HEAD^
+'
+
+test_expect_success 'make remote branches' '
+	git update-ref refs/remotes/origin/branch-one branch-one
+	git update-ref refs/remotes/origin/branch-two branch-two
+	git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/branch-one
+'
+
+cat >expect <<'EOF'
+  branch-one
+  branch-two
+* master
+EOF
+test_expect_success 'git branch shows local branches' '
+	git branch >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+  origin/HEAD -> origin/branch-one
+  origin/branch-one
+  origin/branch-two
+EOF
+test_expect_success 'git branch -r shows remote branches' '
+	git branch -r >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+  branch-one
+  branch-two
+* master
+  remotes/origin/HEAD -> origin/branch-one
+  remotes/origin/branch-one
+  remotes/origin/branch-two
+EOF
+test_expect_success 'git branch -a shows local and remote branches' '
+	git branch -a >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+two
+one
+two
+EOF
+test_expect_success 'git branch -v shows branch summaries' '
+	git branch -v >tmp &&
+	awk "{print \$NF}" <tmp >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+* (no branch)
+  branch-one
+  branch-two
+  master
+EOF
+test_expect_success 'git branch shows detached HEAD properly' '
+	git checkout HEAD^0 &&
+	git branch >actual &&
+	test_cmp expect actual
+'
+
+test_done

-- 
1.6.2.rc1.210.g1210c
Jeff King· Feb 19, 2009, 03:45 UTC · re: Jeff King · lore

Re: [PATCH 1/2] add basic branch display tests

On Wed, Feb 18, 2009 at 10:34:44PM -0500, Jeff King wrote:
> We were not testing the output of "git branch" anywhere.

There is one thing that occurred to me while writing these tests that I wanted to mention.

When we show a remote symref with "git branch -r", it looks like this:
Show 9 quoted lines
> +cat >expect <<'EOF'
> +  origin/HEAD -> origin/branch-one
> +  origin/branch-one
> +  origin/branch-two
> +EOF
> +test_expect_success 'git branch -r shows remote branches' '
> +	git branch -r >actual &&
> +	test_cmp expect actual
> +'
which makes sense. <remote>/<symref> -> <remote>/<branch>
Show 12 quoted lines
> +cat >expect <<'EOF'
> +  branch-one
> +  branch-two
> +* master
> +  remotes/origin/HEAD -> origin/branch-one
> +  remotes/origin/branch-one
> +  remotes/origin/branch-two
> +EOF
> +test_expect_success 'git branch -a shows local and remote branches' '
> +	git branch -a >actual &&
> +	test_cmp expect actual
> +'

But here we stick the "remotes/" head on, since we are showing both types. But the right hand side of the symref doesn't get the same treatment.

I don't think it's a big deal, but I wasn't sure if it was intentional, a bug, or simply that nobody cares (and since I have now codified it in a test script, it seems like we should make sure it is intentional).

I also had a brief thought that reprinting the <remote> is pointless. That is, printing

  origin/HEAD -> master

shows what is happening with less text due to the context (i.e., we already know we are talking about remote "origin" -- and if it isn't in origin, we already show more). But that is probably a bad idea; that context is missing if you were to try to do something like "git show"; <remote>/<branch> would work, but <branch> wouldn't.

-Peff
Jay Soffian· Feb 19, 2009, 03:51 UTC · re: Jeff King · lore

Re: [PATCH 1/2] add basic branch display tests

On Wed, Feb 18, 2009 at 10:45 PM, Jeff King <peff@peff.net> wrote:
Show 5 quoted lines
> But here we stick the "remotes/" head on, since we are showing both
> types. But the right hand side of the symref doesn't get the same
> treatment.
>
> I don't think it's a big deal, but I wasn't sure if it was intentional,

It was intentional on my part. I thought appending it a second time was redundant, and the remotes/ prefix on the LHS is just their to distinguish remote branches from local.

Show 10 quoted lines
> I also had a brief thought that reprinting the <remote> is pointless.
> That is, printing
>
>  origin/HEAD -> master
>
> shows what is happening with less text due to the context (i.e., we
> already know we are talking about remote "origin" -- and if it isn't in
> origin, we already show more). But that is probably a bad idea; that
> context is missing if you were to try to do something like "git show";
> <remote>/<branch> would work, but <branch> wouldn't.
Exactly. :-)

Thanks for the tests. If I had added them myself I wouldn't have you publicly questioning my intent. ;-)

j.
Jeff King· Feb 19, 2009, 03:53 UTC · re: Jay Soffian · lore

Re: [PATCH 1/2] add basic branch display tests

On Wed, Feb 18, 2009 at 10:51:27PM -0500, Jay Soffian wrote:
Show 10 quoted lines
> On Wed, Feb 18, 2009 at 10:45 PM, Jeff King <peff@peff.net> wrote:
> > But here we stick the "remotes/" head on, since we are showing both
> > types. But the right hand side of the symref doesn't get the same
> > treatment.
> >
> > I don't think it's a big deal, but I wasn't sure if it was intentional,
> 
> It was intentional on my part. I thought appending it a second time
> was redundant, and the remotes/ prefix on the LHS is just their to
> distinguish remote branches from local.

OK. The more I think about it, the more I think what is currently there is best.

> Thanks for the tests. If I had added them myself I wouldn't have you
> publicly questioning my intent. ;-)
You're welcome. :)
-Peff
Jeff King· Feb 19, 2009, 03:35 UTC · re: Jeff King · lore

[PATCH 2/2] branch: clean up repeated strlen

Commit 45e2b61 fixed the initialization of a "len" struct parameter via strlen. We can use that to clean up what is now 3 strlens in a 6-line sequence.

Signed-off-by: Jeff King <peff@peff.net>
---
I guess a good compiler could optimize these out, but I think it
actually reads a little bit nicer.
 builtin-branch.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin-branch.c +2 −2
diff --git a/builtin-branch.c b/builtin-branch.c
index 13e4de8..14d4b91 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -443,8 +443,8 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
 		item.kind = REF_LOCAL_BRANCH;
 		item.dest = NULL;
 		item.commit = head_commit;
-		if (strlen(item.name) > ref_list.maxwidth)
-			ref_list.maxwidth = strlen(item.name);
+		if (item.len > ref_list.maxwidth)
+			ref_list.maxwidth = item.len;
 		print_ref_item(&item, ref_list.maxwidth, verbose, abbrev, 1, "");
 		free(item.name);
 	}

-- 
1.6.2.rc1.210.g1210c
Johannes Schindelin· Feb 19, 2009, 11:31 UTC · re: Jeff King · lore

Re: [PATCH 2/2] branch: clean up repeated strlen

Hi,
On Wed, 18 Feb 2009, Jeff King wrote:
Show 8 quoted lines
> Commit 45e2b61 fixed the initialization of a "len" struct
> parameter via strlen. We can use that to clean up what is
> now 3 strlens in a 6-line sequence.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I guess a good compiler could optimize these out, but I think it
> actually reads a little bit nicer.
Definitely.  And thanks for the tests!

Ciao, Dscho

← back to recent threads