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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 19, 2009, 01:15 UTC
Message-ID: <alpine.DEB.1.00.0902190212290.10279@pacific.mpi-cbg.de>
In-Reply-To: <20090219004504.GB25808@coredump.intra.peff.net>

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