patchshow-branch: fix crash with long ref name

> @@ -791,11 +791,11 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  				    head_oid.hash, NULL);
>  	if (head_p) {
>  		head_len = strlen(head_p);
> -		memcpy(head, head_p, head_len + 1);
> +		head_cpy = xstrdup(head_p);
>  	}
>  	else {
>  		head_len = 0;
> -		head[0] = 0;
> +		head_cpy = xstrdup("");
>  	}

> We make several starts_with() calls, only to advance
> pointers. This is exactly what skip_prefix() is for, which
> lets us avoid manually-counted magic numbers.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/show-branch.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 404c4d09a..c03d3ec7c 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -470,17 +470,14 @@ static void snarf_refs(int head, int remotes)
>         }
>  }
>
> -static int rev_is_head(char *head, char *name,
> +static int rev_is_head(const char *head, const char *name,
>                        unsigned char *head_sha1, unsigned char *sha1)
>  {
>         if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
>                 return 0;
> -       if (starts_with(head, "refs/heads/"))
> -               head += 11;
> -       if (starts_with(name, "refs/heads/"))
> -               name += 11;
> -       else if (starts_with(name, "heads/"))
> -               name += 6;
> +       skip_prefix(head, "refs/heads/", &head);
> +       if (!skip_prefix(name, "refs/heads/", &name))
> +               skip_prefix(name, "heads/", &name);
>         return !strcmp(head, name);
>  }
>
> @@ -799,8 +796,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>                                 has_head++;
>                 }
>                 if (!has_head) {
> -                       int offset = starts_with(head, "refs/heads/") ? 11 : 0;
> -                       append_one_rev(head + offset);
> +                       const char *name = head;
> +                       skip_prefix(name, "refs/heads/", &name);
> +                       append_one_rev(name);
>                 }
>         }
>

> Did you purposely miss the one in line number 278 of
> builtin/show-branch.c because I think you only touched up the parts
> which were related to "refs/" but didn't explicitly mention it in the
> commit message?
> 
>     if (starts_with(pretty_str, "[PATCH] "))
>         pretty_str += 8;

> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index c03d3ec7c..19756595d 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)
>  		pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
>  		pretty_str = pretty.buf;
>  	}
> -	if (starts_with(pretty_str, "[PATCH] "))
> -		pretty_str += 8;
> +	skip_prefix(pretty_str, "[PATCH] ", &pretty_str);

> This fixes the problem, but I think we can simplify it quite a bit by
> using resolve_refdup(). Here's the patch series I ended up with:
>
>   [1/3]: show-branch: drop head_len variable
>   [2/3]: show-branch: store resolved head in heap buffer
>   [3/3]: show-branch: use skip_prefix to drop magic numbers
>
>  builtin/show-branch.c | 39 ++++++++++++---------------------------
>  1 file changed, 12 insertions(+), 27 deletions(-)

> Jeff King <peff@peff.net> writes:
> 
> > This fixes the problem, but I think we can simplify it quite a bit by
> > using resolve_refdup(). Here's the patch series I ended up with:
> >
> >   [1/3]: show-branch: drop head_len variable
> >   [2/3]: show-branch: store resolved head in heap buffer
> >   [3/3]: show-branch: use skip_prefix to drop magic numbers
> >
> >  builtin/show-branch.c | 39 ++++++++++++---------------------------
> >  1 file changed, 12 insertions(+), 27 deletions(-)
> 
> Yes, the whole thing is my fault ;-) and I agree with what these
> patches do.
> 
> The second one lacks free(head) but I think that is OK; it is
> something we allocate in cmd_*() and use pretty much thruout the
> rest of the program.

> On Tue, Feb 14, 2017 at 11:35:48AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > This fixes the problem, but I think we can simplify it quite a bit by
>> > using resolve_refdup(). Here's the patch series I ended up with:
>> >
>> >   [1/3]: show-branch: drop head_len variable
>> >   [2/3]: show-branch: store resolved head in heap buffer
>> >   [3/3]: show-branch: use skip_prefix to drop magic numbers

>> Yes, the whole thing is my fault ;-) and I agree with what these
>> patches do.
>>
>> The second one lacks free(head) but I think that is OK; it is
>> something we allocate in cmd_*() and use pretty much thruout the
>> rest of the program.
>
> Yes, I actually tested the whole thing under ASAN (which was necessary
> to notice the problem), which complained about the leak. I don't mind
> adding a free(head), but there are a bunch of similar "leaks" in that
> function, so I didn't bother.

> > I notice Christian's patch added a few tests. I don't know if we'd want
> > to squash them in (I didn't mean to override his patch at all; I was
> > about to send mine out when I noticed his, and I wondered if we wanted
> > to combine the two efforts).
> 
> I think it would be nice to have at least one test. Feel free to
> squash mine if you want.

> -- >8 --
> Subject: [PATCH] show-branch: use skip_prefix to drop magic numbers
>
> We make several starts_with() calls, only to advance
> pointers. This is exactly what skip_prefix() is for, which
> lets us avoid manually-counted magic numbers.
>
> Helped-by: Pranit Bauva <pranit.bauva@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/show-branch.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 404c4d09a..19756595d 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)
>  		pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
>  		pretty_str = pretty.buf;
>  	}
> -	if (starts_with(pretty_str, "[PATCH] "))
> -		pretty_str += 8;
> +	skip_prefix(pretty_str, "[PATCH] ", &pretty_str);
>  
>  	if (!no_name) {
>  		if (name && name->head_name) {
> @@ -470,17 +469,14 @@ static void snarf_refs(int head, int remotes)
>  	}
>  }
>  
> -static int rev_is_head(char *head, char *name,
> +static int rev_is_head(const char *head, const char *name,
>  		       unsigned char *head_sha1, unsigned char *sha1)
>  {
>  	if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
>  		return 0;
> -	if (starts_with(head, "refs/heads/"))
> -		head += 11;
> -	if (starts_with(name, "refs/heads/"))
> -		name += 11;
> -	else if (starts_with(name, "heads/"))
> -		name += 6;
> +	skip_prefix(head, "refs/heads/", &head);
> +	if (!skip_prefix(name, "refs/heads/", &name))
> +		skip_prefix(name, "heads/", &name);
>  	return !strcmp(head, name);
>  }
>  
> @@ -799,8 +795,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  				has_head++;
>  		}
>  		if (!has_head) {
> -			int offset = starts_with(head, "refs/heads/") ? 11 : 0;
> -			append_one_rev(head + offset);
> +			const char *name = head;
> +			skip_prefix(name, "refs/heads/", &name);
> +			append_one_rev(name);
>  		}
>  	}

> Jeff King <peff@peff.net> writes:
> 
> > I see the patches are marked for 'next' in the latest What's Cooking.
> > If it is not too late in today's integration cycle, here is a re-roll of
> > patch 3 that squashes in Pranit's suggestion (if it is too late, then
> > Pranit, you may want to re-send it as a squash on top).
> 
> Thanks.  
> 
> I think that matches what I queued last night, except for the
> Helped-by: line.  Will replace.

> On Tue, Feb 14, 2017 at 10:29:46PM +0100, Christian Couder wrote:
>
>> > I notice Christian's patch added a few tests. I don't know if we'd want
>> > to squash them in (I didn't mean to override his patch at all; I was
>> > about to send mine out when I noticed his, and I wondered if we wanted
>> > to combine the two efforts).
>>
>> I think it would be nice to have at least one test. Feel free to
>> squash mine if you want.
>
> I started to add some tests, but I had second thoughts. It _is_ nice
> to show off the fix, but as far as regressions go, this specific case is
> unlikely to come up again. What would be more valuable, I think, is a
> test script which set up a very long refname (not just 150 bytes or
> whatever) and ran it through a series of git commands.

> > I started to add some tests, but I had second thoughts. It _is_ nice
> > to show off the fix, but as far as regressions go, this specific case is
> > unlikely to come up again. What would be more valuable, I think, is a
> > test script which set up a very long refname (not just 150 bytes or
> > whatever) and ran it through a series of git commands.
> 
> I agree that a test script running through a series of command with
> long refnames would be great.
> 
> But I think the refname should not necesarily be too long. As I wrote
> in the commit message of my patch, if the ref name had been much
> longer the crash would not have happened because the ref could not
> have been created in the first place.

> So the best would be to run through a series of commands with a
> refname ranging from let's say 80 chars to 300 chars.
> 
> That would have a chance to catch crashes due to legacy code using for
> example things like `char stuff[128]` or `char stuff[256]`.

> > So I dunno. It seems like being thorough is a
> > lot of hassle for not much gain. Being not-thorough is easy, but is
> > mostly a token that is unlikely to find any real bugs.
> 
> Yeah, if we really care, it might be better to start using a fuzzer or
> a property based testing tool instead of bothering with these kind of
> tests by ourselves, which is also a different topic.