Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached
- From
Jeff King <peff@peff.net>
- Date
- Feb 19, 2009, 00:45 UTC
- Message-ID
- <20090219004504.GB25808@coredump.intra.peff.net>
- In-Reply-To
- <ad680bce413ddea084d84b3fcd7c4cc356c3cb0e.1234980819u.git.johannes.schindelin@gmx.de>
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
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