# [PATCH] Avoid segfault with 'git branch' when the HEAD is detached 12 messages from 2009-02-18 to 2009-02-19. Participants: Johannes Schindelin, Jay Soffian, Jeff King. Thread: https://gitlist.dev/t/17878 ## Johannes Schindelin, 2009-02-18 18:14 Subject: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached Message-ID: URL: https://gitlist.dev/e/ad680bce413ddea084d84b3fcd7c4cc356c3cb0e.1234980819u.git.johannes.schindelin%40gmx.de In-Reply-To: ``` 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 --- 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 ``` ## Jay Soffian, 2009-02-18 20:36 Subject: Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached Message-ID: <76718490902181236r1058ed6dr3ae881dfb820f068@mail.gmail.com> URL: https://gitlist.dev/e/76718490902181236r1058ed6dr3ae881dfb820f068%40mail.gmail.com In-Reply-To: ``` On Wed, Feb 18, 2009 at 1:14 PM, 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 ACK. Thanks, j. ``` ## Jeff King, 2009-02-19 00:45 Subject: Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached Message-ID: <20090219004504.GB25808@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090219004504.GB25808%40coredump.intra.peff.net In-Reply-To: ``` 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 > --- > > 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... > --- 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; 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, 2009-02-19 01:15 Subject: Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902190212290.10279%40pacific.mpi-cbg.de In-Reply-To: <20090219004504.GB25808@coredump.intra.peff.net> ``` Hi, On Wed, 18 Feb 2009, Jeff King wrote: > 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 > > --- > > > > 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. > > --- 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, 2009-02-19 03:24 Subject: Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached Message-ID: <20090219032420.GA26488@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090219032420.GA26488%40coredump.intra.peff.net In-Reply-To: ``` On Thu, Feb 19, 2009 at 02:15:00AM +0100, Johannes Schindelin wrote: > > 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, 2009-02-19 03:33 Subject: Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached Message-ID: <20090219033329.GA13666@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090219033329.GA13666%40coredump.intra.peff.net In-Reply-To: <20090219032420.GA26488@coredump.intra.peff.net> ``` 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, 2009-02-19 03:34 Subject: [PATCH 1/2] add basic branch display tests Message-ID: <20090219033444.GA18970@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090219033444.GA18970%40coredump.intra.peff.net In-Reply-To: <20090219033329.GA13666@coredump.intra.peff.net> ``` 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 --- 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}" 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, 2009-02-19 03:35 Subject: [PATCH 2/2] branch: clean up repeated strlen Message-ID: <20090219033545.GB18970@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090219033545.GB18970%40coredump.intra.peff.net In-Reply-To: <20090219033329.GA13666@coredump.intra.peff.net> ``` 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 --- 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 ``` ## Jeff King, 2009-02-19 03:45 Subject: Re: [PATCH 1/2] add basic branch display tests Message-ID: <20090219034504.GA19862@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090219034504.GA19862%40coredump.intra.peff.net In-Reply-To: <20090219033444.GA18970@coredump.intra.peff.net> ``` 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. / -> / > +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 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"; / would work, but wouldn't. -Peff ``` ## Jay Soffian, 2009-02-19 03:51 Subject: Re: [PATCH 1/2] add basic branch display tests Message-ID: <76718490902181951p54dc222ai5ac6397f3471fc44@mail.gmail.com> URL: https://gitlist.dev/e/76718490902181951p54dc222ai5ac6397f3471fc44%40mail.gmail.com In-Reply-To: <20090219034504.GA19862@coredump.intra.peff.net> ``` On Wed, Feb 18, 2009 at 10:45 PM, Jeff King 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 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"; > / would work, but 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, 2009-02-19 03:53 Subject: Re: [PATCH 1/2] add basic branch display tests Message-ID: <20090219035332.GC19862@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090219035332.GC19862%40coredump.intra.peff.net In-Reply-To: <76718490902181951p54dc222ai5ac6397f3471fc44@mail.gmail.com> ``` On Wed, Feb 18, 2009 at 10:51:27PM -0500, Jay Soffian wrote: > On Wed, Feb 18, 2009 at 10:45 PM, Jeff King 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 ``` ## Johannes Schindelin, 2009-02-19 11:31 Subject: Re: [PATCH 2/2] branch: clean up repeated strlen Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902191231050.6223%40intel-tinevez-2-302 In-Reply-To: <20090219033545.GB18970@coredump.intra.peff.net> ``` 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 > --- > 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 ```