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

Subsystems: the rest

STALE3481d

7 messages, 3 authors, 2017-02-14 · open the first message on its own page

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

From: Christian Couder <hidden>
Date: 2017-02-14 15:48:56

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 [off-list ref]
Signed-off-by: Christian Couder <redacted>
---
 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 0000000000..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

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

From: Jeff King <hidden>
Date: 2017-02-14 17:25:48

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

quoted hunk

@@ -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 1/3] show-branch: drop head_len variable

From: Jeff King <hidden>
Date: 2017-02-14 17:26:08

We copy the result of resolving HEAD into a buffer and keep
track of its length.  But we never actually use the length
for anything besides the copy. Let's stop passing it around.

Signed-off-by: Jeff King <redacted>
---
 builtin/show-branch.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 974f3403a..e4c488b8c 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c

@@ -470,7 +470,7 @@ static void snarf_refs(int head, int remotes)
 	}
 }
 
-static int rev_is_head(char *head, int headlen, char *name,
+static int rev_is_head(char *head, char *name,
 		       unsigned char *head_sha1, unsigned char *sha1)
 {
 	if ((!head[0]) ||

@@ -622,7 +622,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 	enum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;
 	char head[128];
 	const char *head_p;
-	int head_len;
 	struct object_id head_oid;
 	int merge_base = 0;
 	int independent = 0;

@@ -790,11 +789,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 	head_p = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
 				    head_oid.hash, NULL);
 	if (head_p) {
-		head_len = strlen(head_p);
+		size_t head_len = strlen(head_p);
 		memcpy(head, head_p, head_len + 1);
 	}
 	else {
-		head_len = 0;
 		head[0] = 0;
 	}

@@ -805,7 +803,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			 * HEAD points at.
 			 */
 			if (rev_is_head(head,
-					head_len,
 					ref_name[i],
 					head_oid.hash, NULL))
 				has_head++;

@@ -866,7 +863,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		for (i = 0; i < num_rev; i++) {
 			int j;
 			int is_head = rev_is_head(head,
-						  head_len,
 						  ref_name[i],
 						  head_oid.hash,
 						  rev[i]->object.oid.hash);

-- 
2.12.0.rc1.479.g59880b11e

[PATCH 2/3] show-branch: store resolved head in heap buffer

From: Jeff King <hidden>
Date: 2017-02-14 17:27:52

We resolve HEAD and copy the result to a fixed-size buffer
with memcpy, never checking that it actually fits. This bug
dates back to 8098a178b (Add git-symbolic-ref, 2005-09-30).
Before that we used readlink(), which took a maximum buffer
size.

We can fix this by using resolve_refdup(), which duplicates
the buffer on the heap. That also lets us just check
for a NULL pointer to see if we have resolved HEAD, and
drop the extra head_p variable.

Signed-off-by: Jeff King <redacted>
---
 builtin/show-branch.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index e4c488b8c..404c4d09a 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c

@@ -473,8 +473,7 @@ static void snarf_refs(int head, int remotes)
 static int rev_is_head(char *head, char *name,
 		       unsigned char *head_sha1, unsigned char *sha1)
 {
-	if ((!head[0]) ||
-	    (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
+	if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
 		return 0;
 	if (starts_with(head, "refs/heads/"))
 		head += 11;

@@ -620,8 +619,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];
-	const char *head_p;
+	char *head;
 	struct object_id head_oid;
 	int merge_base = 0;
 	int independent = 0;

@@ -786,17 +784,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			snarf_refs(all_heads, all_remotes);
 	}
 
-	head_p = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
-				    head_oid.hash, NULL);
-	if (head_p) {
-		size_t head_len = strlen(head_p);
-		memcpy(head, head_p, head_len + 1);
-	}
-	else {
-		head[0] = 0;
-	}
+	head = resolve_refdup("HEAD", RESOLVE_REF_READING,
+			      head_oid.hash, NULL);
 
-	if (with_current_branch && head_p) {
+	if (with_current_branch && head) {
 		int has_head = 0;
 		for (i = 0; !has_head && i < ref_name_cnt; i++) {
 			/* We are only interested in adding the branch

-- 
2.12.0.rc1.479.g59880b11e

[PATCH 3/3] show-branch: use skip_prefix to drop magic numbers

From: Jeff King <hidden>
Date: 2017-02-14 17:28:35

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 <redacted>
---
 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);
 		}
 	}

-- 
2.12.0.rc1.479.g59880b11e

Re: [PATCH 3/3] show-branch: use skip_prefix to drop magic numbers

From: Pranit Bauva <hidden>
Date: 2017-02-14 18:53:58

Hey Peff,

On Tue, Feb 14, 2017 at 10:58 PM, Jeff King [off-list ref] wrote:

quoted hunk

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 <redacted>
---
 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;

If not, you can squash this patch attached. Sorry, couldn't send it in
mail because of proxy issues.

Regards,
Pranit Bauva

Re: [PATCH 3/3] show-branch: use skip_prefix to drop magic numbers

From: Jeff King <hidden>
Date: 2017-02-14 19:53:17

On Wed, Feb 15, 2017 at 12:23:52AM +0530, Pranit Bauva wrote:

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;

Not purposely. I was just fixing up the bits that I had noticed in the
earlier patches.

quoted hunk

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

Yeah, I agree this the same type of fix and is worth including. Thanks!

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help