Re: [PATCH] show-branch: fix crash with long ref name

[Jeff King]

On Thu, Feb 16, 2017 at 01:40:00PM +0100, Christian Couder wrote:

> > 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.

Right, I think there's a tension there. Too short and it is not
interesting, and too long and things start to fail for uninteresting
reasons (e.g., your filesystem can't handle the name).

> 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]`.

But that doesn't catch `char stuff[512]`. I think you'd really want a
range of sizes, and to test all of them. Worse, though, is it's not
clear how you decide when a test has failed. Obviously smashing the
stack is a bad outcome, but part of the goal would presumably be to
flush out unnecessary length limitations elsewhere.

I got about that far in my thinking before saying "wow, this is getting
to be complicated for not much gain".

> > 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.

Yes, I'd agree that a fuzzer is probably a better choice. I played with
AFL a bit back when it came out, but I never got it to turn up anything
useful.

I am disappointed that this obvious memory problem survived for so long.
I did quite a bit of auditing for such problems a year or two ago, but I
focused on problematic functions like strcpy, sprintf, etc.

It's easy to use memcpy() wrong, too, but it's hard to audit. There are
a lot of calls, and you have to match up the copied length with a value
computed elsewhere. I traded a lot of them out back then for safer
variants (like the FLEX_ALLOC stuff), but many calls just fundamentally
need something unsafe like memcpy to get their job done.

-Peff

Re: [PATCH] show-branch: fix crash with long ref name

[Christian Couder]

On Wed, Feb 15, 2017 at 10:40 PM, Jeff King  wrote:
> 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 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]`.

Implementing those tests could have started with something like the
test case I sent, but as it would in the end be about many different
commands, one can see it as part of a different topic.

> But then you run into all sorts of portability annoyances with pathname
> restrictions (you can hack around creation by writing the refname
> directly into packed-refs, but most manipulations will want to take the
> .lock in the filesystem).

Yeah, but if a crash doesn't happen because we die() as the ref is too
long for the file system, we could detect that and make the test
succeed.

> 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.

> So I punted, at least for now.

Ok, no problem.

Re: [PATCH] show-branch: fix crash with long ref name

[Jeff King]

On Wed, Feb 15, 2017 at 01:50:07PM -0800, Junio C Hamano wrote:

> Jeff King  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.

Oh, indeed. I should have actually checked what you queued. Thanks.

-Peff

Re: [PATCH] show-branch: fix crash with long ref name

[Junio C Hamano]

Jeff King  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.

> -- >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 
> Signed-off-by: Jeff King 
> ---
>  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_str = pretty.buf;
>   }
> - if (starts_with(pretty_str, "[PATCH] "))
> - pretty_str += 8;
> + skip_prefix(pretty_str, "[PATCH] ", _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/", );
> + if (!skip_prefix(name, "refs/heads/", ))
> + skip_prefix(name, "heads/", );
>   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/", );
> + append_one_rev(name);
>   }
>   }

Re: [PATCH] show-branch: fix crash with long ref name

[Jeff King]

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.

But then you run into all sorts of portability annoyances with pathname
restrictions (you can hack around creation by writing the refname
directly into packed-refs, but most manipulations will want to take the
.lock in the filesystem). 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.

So I punted, at least for now.

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).

-- >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 
Signed-off-by: Jeff King 
---
 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_str = pretty.buf;
}
-   if (starts_with(pretty_str, "[PATCH] "))
-   pretty_str += 8;
+   skip_prefix(pretty_str, "[PATCH] ", _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/", );
+   if (!skip_prefix(name, "refs/heads/", ))
+   skip_prefix(name, "heads/", );
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/", );
+   append_one_rev(name);
}
}
 
-- 
2.12.0.rc1.541.g3e32dea89

Re: [PATCH] show-branch: fix crash with long ref name

[Christian Couder]

On Tue, Feb 14, 2017 at 8:55 PM, Jeff King  wrote:
> On Tue, Feb 14, 2017 at 11:35:48AM -0800, Junio C Hamano wrote:
>
>> Jeff King  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

Yeah, I noticed there were a number of things that could be improved
in the area, but I didn't want to spend too much time on this, so
thanks for this series.

>> 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.

Yeah, I didn't bother either.

> 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.

Re: [PATCH] show-branch: fix crash with long ref name

[Jeff King]

On Tue, Feb 14, 2017 at 11:35:48AM -0800, Junio C Hamano wrote:

> Jeff King  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.

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).

-Peff

Re: [PATCH] show-branch: fix crash with long ref name

[Junio C Hamano]

Jeff King  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.

Thanks.

Re: [PATCH] show-branch: fix crash with long ref name

[Jeff King]

On Tue, Feb 14, 2017 at 04:48:16PM +0100, Christian Couder wrote:

> @@ -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("");
>   }

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(-)

-Peff

[PATCH] show-branch: fix crash with long ref name

[Christian Couder]

This is a minimum fix for a crash with a long ref name.

Note that if the refname is too long (for example if you
use 100 instead of 50 in the test script) you could get
an error like:

fatal: cannot lock ref 'refs/heads/1_2_3_4_ ... _98_99_100_':
Unable to create '... /.git/refs/heads/1_2_3_4_ ... _98_99_100_.lock':
File name too long

when creating the ref instead of a crash when using
show-branch and that would be ok.

So a simpler fix could have been just something like:

-   char head[128];
+   char head[1024];

But if the filesystem ever allows filenames longer than 1024
characters then the crash could appear again.

Reported by: Maxim Kuvyrkov 
Signed-off-by: Christian Couder 
---
 builtin/show-branch.c  | 14 +++---
 t/t3204-show-branch-refname.sh | 19 +++
 2 files changed, 26 insertions(+), 7 deletions(-)
 create mode 100755 t/t3204-show-branch-refname.sh

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 974f3403ab..3c0fe55eec 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -620,7 +620,7 @@ int cmd_show_branch(int ac, const char **av, const char 
*prefix)
int all_heads = 0, all_remotes = 0;
int all_mask, all_revs;
enum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;
-   char head[128];
+   char *head_cpy;
const char *head_p;
int head_len;
struct object_id head_oid;
@@ -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("");
}
 
if (with_current_branch && head_p) {
@@ -804,15 +804,15 @@ int cmd_show_branch(int ac, const char **av, const char 
*prefix)
/* We are only interested in adding the branch
 * HEAD points at.
 */
-   if (rev_is_head(head,
+   if (rev_is_head(head_cpy,
head_len,
ref_name[i],
head_oid.hash, NULL))
has_head++;
}
if (!has_head) {
-   int offset = starts_with(head, "refs/heads/") ? 11 : 0;
-   append_one_rev(head + offset);
+   int offset = starts_with(head_cpy, "refs/heads/") ? 11 
: 0;
+   append_one_rev(head_cpy + offset);
}
}
 
@@ -865,7 +865,7 @@ int cmd_show_branch(int ac, const char **av, const char 
*prefix)
if (1 < num_rev || extra < 0) {
for (i = 0; i < num_rev; i++) {
int j;
-   int is_head = rev_is_head(head,
+   int is_head = rev_is_head(head_cpy,
  head_len,
  ref_name[i],
  head_oid.hash,
diff --git a/t/t3204-show-branch-refname.sh b/t/t3204-show-branch-refname.sh
new file mode 100755
index 00..83b64e3032
--- /dev/null
+++ b/t/t3204-show-branch-refname.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='test show-branch with long refname'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+   test_commit first &&
+   long_refname=$(printf "%s_" $(seq 1 50)) &&
+   git checkout -b "$long_refname"
+'
+
+test_expect_success 'show-branch with long refname' '
+
+   git show-branch first
+'
+
+test_done
-- 
2.12.0.rc0