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

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

	Unfortunately not found by valgrind.

 builtin-branch.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

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

On Wed, Feb 18, 2009 at 1:14 PM, Johannes Schindelin
[off-list ref] 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 <redacted>

ACK.

Thanks,

j.

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

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

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

Hi,

On Wed, 18 Feb 2009, Jeff King wrote:

On Wed, Feb 18, 2009 at 07:14:59PM +0100, Johannes Schindelin wrote:

quoted

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

	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.

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

On Thu, Feb 19, 2009 at 02:15:00AM +0100, Johannes Schindelin wrote:

quoted

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

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

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 <redacted>
---
 t/t3203-branch-output.sh |   81 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletions(-)
 create mode 100755 t/t3203-branch-output.sh

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

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 <redacted>
---
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(-)

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

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:

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

+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

On Wed, Feb 18, 2009 at 10:45 PM, Jeff King [off-list ref] 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.

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.

On Wed, Feb 18, 2009 at 10:51:27PM -0500, Jay Soffian wrote:

On Wed, Feb 18, 2009 at 10:45 PM, Jeff King [off-list ref] wrote:

quoted

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

Hi,

On Wed, 18 Feb 2009, Jeff King wrote:

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