Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached
From: Jeff King <hidden>
Date: 2016-06-15 22:46:14
Subsystem:
the rest · Maintainer:
Linus Torvalds
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...
quoted hunk ↗ jump to hunk
--- 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