{"thread":{"id":"45139","subject":"[PATCH] show-branch: fix crash with long ref name","startedAt":"2017-02-14T15:48:56Z","lastAt":"2017-02-17T05:03:47Z","messageCount":15,"participants":["Christian Couder","Jeff King","Pranit Bauva","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"311543","messageId":"20170214154816.12625-1-chriscool@tuxfamily.org","threadId":"45139","inReplyTo":null,"subject":"[PATCH] show-branch: fix crash with long ref name","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-02-14T15:48:16Z","receivedAt":"2017-02-14T15:48:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This is a minimum fix for a crash with a long ref name.\n\nNote that if the refname is too long (for example if you\nuse 100 instead of 50 in the test script) you could get\nan error like:\n\nfatal: cannot lock ref 'refs/heads/1_2_3_4_ ... _98_99_100_':\nUnable to create '... /.git/refs/heads/1_2_3_4_ ... _98_99_100_.lock':\nFile name too long\n\nwhen creating the ref instead of a crash when using\nshow-branch and that would be ok.\n\nSo a simpler fix could have been just something like:\n\n- char head[128];\n+ char head[1024];\n\nBut if the filesystem ever allows filenames longer than 1024\ncharacters then the crash could appear again.\n\nReported by: Maxim Kuvyrkov \nSigned-off-by: Christian Couder \n---\n builtin/show-branch.c | 14 +++++++-------\n t/t3204-show-branch-refname.sh | 19 +++++++++++++++++++\n 2 files changed, 26 insertions(+), 7 deletions(-)\n create mode 100755 t/t3204-show-branch-refname.sh\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 974f3403ab..3c0fe55eec 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -620,7 +620,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \tint all_heads = 0, all_remotes = 0;\n \tint all_mask, all_revs;\n \tenum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;\n-\tchar head[128];\n+\tchar *head_cpy;\n \tconst char *head_p;\n \tint head_len;\n \tstruct object_id head_oid;\n@@ -791,11 +791,11 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\t head_oid.hash, NULL);\n \tif (head_p) {\n \t\thead_len = strlen(head_p);\n-\t\tmemcpy(head, head_p, head_len + 1);\n+\t\thead_cpy = xstrdup(head_p);\n \t}\n \telse {\n \t\thead_len = 0;\n-\t\thead[0] = 0;\n+\t\thead_cpy = xstrdup(\"\");\n \t}\n \n \tif (with_current_branch && head_p) {\n@@ -804,15 +804,15 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t/* We are only interested in adding the branch\n \t\t\t * HEAD points at.\n \t\t\t */\n-\t\t\tif (rev_is_head(head,\n+\t\t\tif (rev_is_head(head_cpy,\n \t\t\t\t\thead_len,\n \t\t\t\t\tref_name[i],\n \t\t\t\t\thead_oid.hash, NULL))\n \t\t\t\thas_head++;\n \t\t}\n \t\tif (!has_head) {\n-\t\t\tint offset = starts_with(head, \"refs/heads/\") ? 11 : 0;\n-\t\t\tappend_one_rev(head + offset);\n+\t\t\tint offset = starts_with(head_cpy, \"refs/heads/\") ? 11 : 0;\n+\t\t\tappend_one_rev(head_cpy + offset);\n \t\t}\n \t}\n \n@@ -865,7 +865,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \tif (1 < num_rev || extra < 0) {\n \t\tfor (i = 0; i < num_rev; i++) {\n \t\t\tint j;\n-\t\t\tint is_head = rev_is_head(head,\n+\t\t\tint is_head = rev_is_head(head_cpy,\n \t\t\t\t\t\t head_len,\n \t\t\t\t\t\t ref_name[i],\n \t\t\t\t\t\t head_oid.hash,\ndiff --git a/t/t3204-show-branch-refname.sh b/t/t3204-show-branch-refname.sh\nnew file mode 100755\nindex 0000000000..83b64e3032\n--- /dev/null\n+++ b/t/t3204-show-branch-refname.sh\n@@ -0,0 +1,19 @@\n+#!/bin/sh\n+\n+test_description='test show-branch with long refname'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\n+\ttest_commit first &&\n+\tlong_refname=$(printf \"%s_\" $(seq 1 50)) &&\n+\tgit checkout -b \"$long_refname\"\n+'\n+\n+test_expect_success 'show-branch with long refname' '\n+\n+\tgit show-branch first\n+'\n+\n+test_done\n-- \n2.12.0.rc0\n\n"},{"id":"311546","messageId":"20170214172526.hzpm3d3ubd3vjnzr@sigill.intra.peff.net","threadId":"45139","inReplyTo":"20170214154816.12625-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] show-branch: fix crash with long ref name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T17:25:26Z","receivedAt":"2017-02-14T17:25:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2017 at 04:48:16PM +0100, Christian Couder wrote:\n\n> @@ -791,11 +791,11 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n> \t\t\t\t head_oid.hash, NULL);\n> \tif (head_p) {\n> \t\thead_len = strlen(head_p);\n> -\t\tmemcpy(head, head_p, head_len + 1);\n> +\t\thead_cpy = xstrdup(head_p);\n> \t}\n> \telse {\n> \t\thead_len = 0;\n> -\t\thead[0] = 0;\n> +\t\thead_cpy = xstrdup(\"\");\n> \t}\n\nThis fixes the problem, but I think we can simplify it quite a bit by\nusing resolve_refdup(). Here's the patch series I ended up with:\n\n [1/3]: show-branch: drop head_len variable\n [2/3]: show-branch: store resolved head in heap buffer\n [3/3]: show-branch: use skip_prefix to drop magic numbers\n\n builtin/show-branch.c | 39 ++++++++++++---------------------------\n 1 file changed, 12 insertions(+), 27 deletions(-)\n\n-Peff\n"},{"id":"311547","messageId":"20170214172601.3bt64h35i65vz4sj@sigill.intra.peff.net","threadId":"45139","inReplyTo":"20170214172526.hzpm3d3ubd3vjnzr@sigill.intra.peff.net","subject":"[PATCH 1/3] show-branch: drop head_len variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T17:26:01Z","receivedAt":"2017-02-14T17:26:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We copy the result of resolving HEAD into a buffer and keep\ntrack of its length. But we never actually use the length\nfor anything besides the copy. Let's stop passing it around.\n\nSigned-off-by: Jeff King \n---\n builtin/show-branch.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 974f3403a..e4c488b8c 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -470,7 +470,7 @@ static void snarf_refs(int head, int remotes)\n \t}\n }\n \n-static int rev_is_head(char *head, int headlen, char *name,\n+static int rev_is_head(char *head, char *name,\n \t\t unsigned char *head_sha1, unsigned char *sha1)\n {\n \tif ((!head[0]) ||\n@@ -622,7 +622,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \tenum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;\n \tchar head[128];\n \tconst char *head_p;\n-\tint head_len;\n \tstruct object_id head_oid;\n \tint merge_base = 0;\n \tint independent = 0;\n@@ -790,11 +789,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \thead_p = resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING,\n \t\t\t\t head_oid.hash, NULL);\n \tif (head_p) {\n-\t\thead_len = strlen(head_p);\n+\t\tsize_t head_len = strlen(head_p);\n \t\tmemcpy(head, head_p, head_len + 1);\n \t}\n \telse {\n-\t\thead_len = 0;\n \t\thead[0] = 0;\n \t}\n \n@@ -805,7 +803,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t * HEAD points at.\n \t\t\t */\n \t\t\tif (rev_is_head(head,\n-\t\t\t\t\thead_len,\n \t\t\t\t\tref_name[i],\n \t\t\t\t\thead_oid.hash, NULL))\n \t\t\t\thas_head++;\n@@ -866,7 +863,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\tfor (i = 0; i < num_rev; i++) {\n \t\t\tint j;\n \t\t\tint is_head = rev_is_head(head,\n-\t\t\t\t\t\t head_len,\n \t\t\t\t\t\t ref_name[i],\n \t\t\t\t\t\t head_oid.hash,\n \t\t\t\t\t\t rev[i]->object.oid.hash);\n-- \n2.12.0.rc1.479.g59880b11e\n\n"},{"id":"311548","messageId":"20170214172745.4tnnu6kv7xru54vy@sigill.intra.peff.net","threadId":"45139","inReplyTo":"20170214172526.hzpm3d3ubd3vjnzr@sigill.intra.peff.net","subject":"[PATCH 2/3] show-branch: store resolved head in heap buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T17:27:45Z","receivedAt":"2017-02-14T17:27:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We resolve HEAD and copy the result to a fixed-size buffer\nwith memcpy, never checking that it actually fits. This bug\ndates back to 8098a178b (Add git-symbolic-ref, 2005-09-30).\nBefore that we used readlink(), which took a maximum buffer\nsize.\n\nWe can fix this by using resolve_refdup(), which duplicates\nthe buffer on the heap. That also lets us just check\nfor a NULL pointer to see if we have resolved HEAD, and\ndrop the extra head_p variable.\n\nSigned-off-by: Jeff King \n---\n builtin/show-branch.c | 19 +++++--------------\n 1 file changed, 5 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex e4c488b8c..404c4d09a 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -473,8 +473,7 @@ static void snarf_refs(int head, int remotes)\n static int rev_is_head(char *head, char *name,\n \t\t unsigned char *head_sha1, unsigned char *sha1)\n {\n-\tif ((!head[0]) ||\n-\t (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))\n+\tif (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))\n \t\treturn 0;\n \tif (starts_with(head, \"refs/heads/\"))\n \t\thead += 11;\n@@ -620,8 +619,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \tint all_heads = 0, all_remotes = 0;\n \tint all_mask, all_revs;\n \tenum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;\n-\tchar head[128];\n-\tconst char *head_p;\n+\tchar *head;\n \tstruct object_id head_oid;\n \tint merge_base = 0;\n \tint independent = 0;\n@@ -786,17 +784,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\tsnarf_refs(all_heads, all_remotes);\n \t}\n \n-\thead_p = resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING,\n-\t\t\t\t head_oid.hash, NULL);\n-\tif (head_p) {\n-\t\tsize_t head_len = strlen(head_p);\n-\t\tmemcpy(head, head_p, head_len + 1);\n-\t}\n-\telse {\n-\t\thead[0] = 0;\n-\t}\n+\thead = resolve_refdup(\"HEAD\", RESOLVE_REF_READING,\n+\t\t\t head_oid.hash, NULL);\n \n-\tif (with_current_branch && head_p) {\n+\tif (with_current_branch && head) {\n \t\tint has_head = 0;\n \t\tfor (i = 0; !has_head && i < ref_name_cnt; i++) {\n \t\t\t/* We are only interested in adding the branch\n-- \n2.12.0.rc1.479.g59880b11e\n\n"},{"id":"311549","messageId":"20170214172829.6w3cnnqy6ozxl424@sigill.intra.peff.net","threadId":"45139","inReplyTo":"20170214172526.hzpm3d3ubd3vjnzr@sigill.intra.peff.net","subject":"[PATCH 3/3] show-branch: use skip_prefix to drop magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T17:28:29Z","receivedAt":"2017-02-14T17:28:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We make several starts_with() calls, only to advance\npointers. This is exactly what skip_prefix() is for, which\nlets us avoid manually-counted magic numbers.\n\nSigned-off-by: Jeff King \n---\n builtin/show-branch.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 404c4d09a..c03d3ec7c 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -470,17 +470,14 @@ static void snarf_refs(int head, int remotes)\n \t}\n }\n \n-static int rev_is_head(char *head, char *name,\n+static int rev_is_head(const char *head, const char *name,\n \t\t unsigned char *head_sha1, unsigned char *sha1)\n {\n \tif (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))\n \t\treturn 0;\n-\tif (starts_with(head, \"refs/heads/\"))\n-\t\thead += 11;\n-\tif (starts_with(name, \"refs/heads/\"))\n-\t\tname += 11;\n-\telse if (starts_with(name, \"heads/\"))\n-\t\tname += 6;\n+\tskip_prefix(head, \"refs/heads/\", &head);\n+\tif (!skip_prefix(name, \"refs/heads/\", &name))\n+\t\tskip_prefix(name, \"heads/\", &name);\n \treturn !strcmp(head, name);\n }\n \n@@ -799,8 +796,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\thas_head++;\n \t\t}\n \t\tif (!has_head) {\n-\t\t\tint offset = starts_with(head, \"refs/heads/\") ? 11 : 0;\n-\t\t\tappend_one_rev(head + offset);\n+\t\t\tconst char *name = head;\n+\t\t\tskip_prefix(name, \"refs/heads/\", &name);\n+\t\t\tappend_one_rev(name);\n \t\t}\n \t}\n \n-- \n2.12.0.rc1.479.g59880b11e\n"},{"id":"311561","messageId":"CAFZEwPOYGRGd5PWSfLRd6vMs35TT1ZzUSfr79fwRA4VzVjAWXA@mail.gmail.com","threadId":"45139","inReplyTo":"20170214172829.6w3cnnqy6ozxl424@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] show-branch: use skip_prefix to drop magic numbers","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2017-02-14T18:53:52Z","receivedAt":"2017-02-14T18:53:58Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Peff,\n\nOn Tue, Feb 14, 2017 at 10:58 PM, Jeff King wrote:\n> We make several starts_with() calls, only to advance\n> pointers. This is exactly what skip_prefix() is for, which\n> lets us avoid manually-counted magic numbers.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/show-branch.c | 16 +++++++---------\n> 1 file changed, 7 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index 404c4d09a..c03d3ec7c 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -470,17 +470,14 @@ static void snarf_refs(int head, int remotes)\n> }\n> }\n>\n> -static int rev_is_head(char *head, char *name,\n> +static int rev_is_head(const char *head, const char *name,\n> unsigned char *head_sha1, unsigned char *sha1)\n> {\n> if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))\n> return 0;\n> - if (starts_with(head, \"refs/heads/\"))\n> - head += 11;\n> - if (starts_with(name, \"refs/heads/\"))\n> - name += 11;\n> - else if (starts_with(name, \"heads/\"))\n> - name += 6;\n> + skip_prefix(head, \"refs/heads/\", &head);\n> + if (!skip_prefix(name, \"refs/heads/\", &name))\n> + skip_prefix(name, \"heads/\", &name);\n> return !strcmp(head, name);\n> }\n>\n> @@ -799,8 +796,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n> has_head++;\n> }\n> if (!has_head) {\n> - int offset = starts_with(head, \"refs/heads/\") ? 11 : 0;\n> - append_one_rev(head + offset);\n> + const char *name = head;\n> + skip_prefix(name, \"refs/heads/\", &name);\n> + append_one_rev(name);\n> }\n> }\n>\n\n\nDid you purposely miss the one in line number 278 of\nbuiltin/show-branch.c because I think you only touched up the parts\nwhich were related to \"refs/\" but didn't explicitly mention it in the\ncommit message?\n\n if (starts_with(pretty_str, \"[PATCH] \"))\n pretty_str += 8;\n\nIf not, you can squash this patch attached. Sorry, couldn't send it in\nmail because of proxy issues.\n\nRegards,\nPranit Bauva\n\n\nFrom 2e80d4458df659765011450621ee34459dc749f9 Mon Sep 17 00:00:00 2001\nFrom: Pranit Bauva \nDate: Tue, 14 Feb 2017 23:53:36 +0530\nSubject: [PATCH] !squash: show-branch: use skip_prefix to drop magic numbers\n\nSigned-off-by: Pranit Bauva \n---\n builtin/show-branch.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex c03d3ec7c..19756595d 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)\n \t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);\n \t\tpretty_str = pretty.buf;\n \t}\n-\tif (starts_with(pretty_str, \"[PATCH] \"))\n-\t\tpretty_str += 8;\n+\tskip_prefix(pretty_str, \"[PATCH] \", &pretty_str);\n \n \tif (!no_name) {\n \t\tif (name && name->head_name) {\n-- \n2.11.0\n\n"},{"id":"311566","messageId":"xmqqlgt88t0r.fsf@gitster.mtv.corp.google.com","threadId":"45139","inReplyTo":"20170214172526.hzpm3d3ubd3vjnzr@sigill.intra.peff.net","subject":"Re: [PATCH] show-branch: fix crash with long ref name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-14T19:35:48Z","receivedAt":"2017-02-14T19:36:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This fixes the problem, but I think we can simplify it quite a bit by\n> using resolve_refdup(). Here's the patch series I ended up with:\n>\n> [1/3]: show-branch: drop head_len variable\n> [2/3]: show-branch: store resolved head in heap buffer\n> [3/3]: show-branch: use skip_prefix to drop magic numbers\n>\n> builtin/show-branch.c | 39 ++++++++++++---------------------------\n> 1 file changed, 12 insertions(+), 27 deletions(-)\n\nYes, the whole thing is my fault ;-) and I agree with what these\npatches do.\n\nThe second one lacks free(head) but I think that is OK; it is\nsomething we allocate in cmd_*() and use pretty much thruout the\nrest of the program.\n\nThanks.\n"},{"id":"311568","messageId":"20170214195310.hlfke5vks6pnzmma@sigill.intra.peff.net","threadId":"45139","inReplyTo":"CAFZEwPOYGRGd5PWSfLRd6vMs35TT1ZzUSfr79fwRA4VzVjAWXA@mail.gmail.com","subject":"Re: [PATCH 3/3] show-branch: use skip_prefix to drop magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T19:53:10Z","receivedAt":"2017-02-14T19:53:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 15, 2017 at 12:23:52AM +0530, Pranit Bauva wrote:\n\n> Did you purposely miss the one in line number 278 of\n> builtin/show-branch.c because I think you only touched up the parts\n> which were related to \"refs/\" but didn't explicitly mention it in the\n> commit message?\n> \n> if (starts_with(pretty_str, \"[PATCH] \"))\n> pretty_str += 8;\n\nNot purposely. I was just fixing up the bits that I had noticed in the\nearlier patches.\n\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index c03d3ec7c..19756595d 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)\n> \t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);\n> \t\tpretty_str = pretty.buf;\n> \t}\n> -\tif (starts_with(pretty_str, \"[PATCH] \"))\n> -\t\tpretty_str += 8;\n> +\tskip_prefix(pretty_str, \"[PATCH] \", &pretty_str);\n\nYeah, I agree this the same type of fix and is worth including. Thanks!\n\n-Peff\n"},{"id":"311569","messageId":"20170214195513.7zae6x22advkrms6@sigill.intra.peff.net","threadId":"45139","inReplyTo":"xmqqlgt88t0r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] show-branch: fix crash with long ref name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T19:55:13Z","receivedAt":"2017-02-14T19:55:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2017 at 11:35:48AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > This fixes the problem, but I think we can simplify it quite a bit by\n> > using resolve_refdup(). Here's the patch series I ended up with:\n> >\n> > [1/3]: show-branch: drop head_len variable\n> > [2/3]: show-branch: store resolved head in heap buffer\n> > [3/3]: show-branch: use skip_prefix to drop magic numbers\n> >\n> > builtin/show-branch.c | 39 ++++++++++++---------------------------\n> > 1 file changed, 12 insertions(+), 27 deletions(-)\n> \n> Yes, the whole thing is my fault ;-) and I agree with what these\n> patches do.\n> \n> The second one lacks free(head) but I think that is OK; it is\n> something we allocate in cmd_*() and use pretty much thruout the\n> rest of the program.\n\nYes, I actually tested the whole thing under ASAN (which was necessary\nto notice the problem), which complained about the leak. I don't mind\nadding a free(head), but there are a bunch of similar \"leaks\" in that\nfunction, so I didn't bother.\n\nI notice Christian's patch added a few tests. I don't know if we'd want\nto squash them in (I didn't mean to override his patch at all; I was\nabout to send mine out when I noticed his, and I wondered if we wanted\nto combine the two efforts).\n\n-Peff\n"},{"id":"311583","messageId":"CAP8UFD0EfUgfmTB4dj-A+rw79F7SWKxYvatNfR+Nj-8ukWYAQA@mail.gmail.com","threadId":"45139","inReplyTo":"20170214195513.7zae6x22advkrms6@sigill.intra.peff.net","subject":"Re: [PATCH] show-branch: fix crash with long ref name","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-02-14T21:29:46Z","receivedAt":"2017-02-14T21:29:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Feb 14, 2017 at 8:55 PM, Jeff King wrote:\n> On Tue, Feb 14, 2017 at 11:35:48AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>>\n>> > This fixes the problem, but I think we can simplify it quite a bit by\n>> > using resolve_refdup(). Here's the patch series I ended up with:\n>> >\n>> > [1/3]: show-branch: drop head_len variable\n>> > [2/3]: show-branch: store resolved head in heap buffer\n>> > [3/3]: show-branch: use skip_prefix to drop magic numbers\n\nYeah, I noticed there were a number of things that could be improved\nin the area, but I didn't want to spend too much time on this, so\nthanks for this series.\n\n>> Yes, the whole thing is my fault ;-) and I agree with what these\n>> patches do.\n>>\n>> The second one lacks free(head) but I think that is OK; it is\n>> something we allocate in cmd_*() and use pretty much thruout the\n>> rest of the program.\n>\n> Yes, I actually tested the whole thing under ASAN (which was necessary\n> to notice the problem), which complained about the leak. I don't mind\n> adding a free(head), but there are a bunch of similar \"leaks\" in that\n> function, so I didn't bother.\n\nYeah, I didn't bother either.\n\n> I notice Christian's patch added a few tests. I don't know if we'd want\n> to squash them in (I didn't mean to override his patch at all; I was\n> about to send mine out when I noticed his, and I wondered if we wanted\n> to combine the two efforts).\n\nI think it would be nice to have at least one test. Feel free to\nsquash mine if you want.\n"},{"id":"311679","messageId":"20170215214052.5py4pxkcz4g2bmtk@sigill.intra.peff.net","threadId":"45139","inReplyTo":"CAP8UFD0EfUgfmTB4dj-A+rw79F7SWKxYvatNfR+Nj-8ukWYAQA@mail.gmail.com","subject":"Re: [PATCH] show-branch: fix crash with long ref name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-15T21:40:52Z","receivedAt":"2017-02-15T21:40:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2017 at 10:29:46PM +0100, Christian Couder wrote:\n\n> > I notice Christian's patch added a few tests. I don't know if we'd want\n> > to squash them in (I didn't mean to override his patch at all; I was\n> > about to send mine out when I noticed his, and I wondered if we wanted\n> > to combine the two efforts).\n> \n> I think it would be nice to have at least one test. Feel free to\n> squash mine if you want.\n\nI started to add some tests, but I had second thoughts. It _is_ nice\nto show off the fix, but as far as regressions go, this specific case is\nunlikely to come up again. What would be more valuable, I think, is a\ntest script which set up a very long refname (not just 150 bytes or\nwhatever) and ran it through a series of git commands.\n\nBut then you run into all sorts of portability annoyances with pathname\nrestrictions (you can hack around creation by writing the refname\ndirectly into packed-refs, but most manipulations will want to take the\n.lock in the filesystem). So I dunno. It seems like being thorough is a\nlot of hassle for not much gain. Being not-thorough is easy, but is\nmostly a token that is unlikely to find any real bugs.\n\nSo I punted, at least for now.\n\nI see the patches are marked for 'next' in the latest What's Cooking.\nIf it is not too late in today's integration cycle, here is a re-roll of\npatch 3 that squashes in Pranit's suggestion (if it is too late, then\nPranit, you may want to re-send it as a squash on top).\n\n-- >8 --\nSubject: [PATCH] show-branch: use skip_prefix to drop magic numbers\n\nWe make several starts_with() calls, only to advance\npointers. This is exactly what skip_prefix() is for, which\nlets us avoid manually-counted magic numbers.\n\nHelped-by: Pranit Bauva \nSigned-off-by: Jeff King \n---\n builtin/show-branch.c | 19 ++++++++-----------\n 1 file changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 404c4d09a..19756595d 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)\n \t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);\n \t\tpretty_str = pretty.buf;\n \t}\n-\tif (starts_with(pretty_str, \"[PATCH] \"))\n-\t\tpretty_str += 8;\n+\tskip_prefix(pretty_str, \"[PATCH] \", &pretty_str);\n \n \tif (!no_name) {\n \t\tif (name && name->head_name) {\n@@ -470,17 +469,14 @@ static void snarf_refs(int head, int remotes)\n \t}\n }\n \n-static int rev_is_head(char *head, char *name,\n+static int rev_is_head(const char *head, const char *name,\n \t\t unsigned char *head_sha1, unsigned char *sha1)\n {\n \tif (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))\n \t\treturn 0;\n-\tif (starts_with(head, \"refs/heads/\"))\n-\t\thead += 11;\n-\tif (starts_with(name, \"refs/heads/\"))\n-\t\tname += 11;\n-\telse if (starts_with(name, \"heads/\"))\n-\t\tname += 6;\n+\tskip_prefix(head, \"refs/heads/\", &head);\n+\tif (!skip_prefix(name, \"refs/heads/\", &name))\n+\t\tskip_prefix(name, \"heads/\", &name);\n \treturn !strcmp(head, name);\n }\n \n@@ -799,8 +795,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\thas_head++;\n \t\t}\n \t\tif (!has_head) {\n-\t\t\tint offset = starts_with(head, \"refs/heads/\") ? 11 : 0;\n-\t\t\tappend_one_rev(head + offset);\n+\t\t\tconst char *name = head;\n+\t\t\tskip_prefix(name, \"refs/heads/\", &name);\n+\t\t\tappend_one_rev(name);\n \t\t}\n \t}\n \n-- \n2.12.0.rc1.541.g3e32dea89\n\n"},{"id":"311681","messageId":"xmqqfujf2kfk.fsf@gitster.mtv.corp.google.com","threadId":"45139","inReplyTo":"20170215214052.5py4pxkcz4g2bmtk@sigill.intra.peff.net","subject":"Re: [PATCH] show-branch: fix crash with long ref name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T21:50:07Z","receivedAt":"2017-02-15T21:50:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I see the patches are marked for 'next' in the latest What's Cooking.\n> If it is not too late in today's integration cycle, here is a re-roll of\n> patch 3 that squashes in Pranit's suggestion (if it is too late, then\n> Pranit, you may want to re-send it as a squash on top).\n\nThanks. \n\nI think that matches what I queued last night, except for the\nHelped-by: line. Will replace.\n\n> -- >8 --\n> Subject: [PATCH] show-branch: use skip_prefix to drop magic numbers\n>\n> We make several starts_with() calls, only to advance\n> pointers. This is exactly what skip_prefix() is for, which\n> lets us avoid manually-counted magic numbers.\n>\n> Helped-by: Pranit Bauva \n> Signed-off-by: Jeff King \n> ---\n> builtin/show-branch.c | 19 ++++++++-----------\n> 1 file changed, 8 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index 404c4d09a..19756595d 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)\n> \t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);\n> \t\tpretty_str = pretty.buf;\n> \t}\n> -\tif (starts_with(pretty_str, \"[PATCH] \"))\n> -\t\tpretty_str += 8;\n> +\tskip_prefix(pretty_str, \"[PATCH] \", &pretty_str);\n> \n> \tif (!no_name) {\n> \t\tif (name && name->head_name) {\n> @@ -470,17 +469,14 @@ static void snarf_refs(int head, int remotes)\n> \t}\n> }\n> \n> -static int rev_is_head(char *head, char *name,\n> +static int rev_is_head(const char *head, const char *name,\n> \t\t unsigned char *head_sha1, unsigned char *sha1)\n> {\n> \tif (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))\n> \t\treturn 0;\n> -\tif (starts_with(head, \"refs/heads/\"))\n> -\t\thead += 11;\n> -\tif (starts_with(name, \"refs/heads/\"))\n> -\t\tname += 11;\n> -\telse if (starts_with(name, \"heads/\"))\n> -\t\tname += 6;\n> +\tskip_prefix(head, \"refs/heads/\", &head);\n> +\tif (!skip_prefix(name, \"refs/heads/\", &name))\n> +\t\tskip_prefix(name, \"heads/\", &name);\n> \treturn !strcmp(head, name);\n> }\n> \n> @@ -799,8 +795,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n> \t\t\t\thas_head++;\n> \t\t}\n> \t\tif (!has_head) {\n> -\t\t\tint offset = starts_with(head, \"refs/heads/\") ? 11 : 0;\n> -\t\t\tappend_one_rev(head + offset);\n> +\t\t\tconst char *name = head;\n> +\t\t\tskip_prefix(name, \"refs/heads/\", &name);\n> +\t\t\tappend_one_rev(name);\n> \t\t}\n> \t}\n"},{"id":"311684","messageId":"20170215215256.crg3t5nrj6kjvw2i@sigill.intra.peff.net","threadId":"45139","inReplyTo":"xmqqfujf2kfk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] show-branch: fix crash with long ref name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-15T21:52:56Z","receivedAt":"2017-02-15T21:53:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 15, 2017 at 01:50:07PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I see the patches are marked for 'next' in the latest What's Cooking.\n> > If it is not too late in today's integration cycle, here is a re-roll of\n> > patch 3 that squashes in Pranit's suggestion (if it is too late, then\n> > Pranit, you may want to re-send it as a squash on top).\n> \n> Thanks. \n> \n> I think that matches what I queued last night, except for the\n> Helped-by: line. Will replace.\n\nOh, indeed. I should have actually checked what you queued. Thanks.\n\n-Peff\n"},{"id":"311761","messageId":"CAP8UFD20=zVy=1Tr4D1Rxf+a4yR_X2zmexNCTNKV5RSF9ueYrg@mail.gmail.com","threadId":"45139","inReplyTo":"20170215214052.5py4pxkcz4g2bmtk@sigill.intra.peff.net","subject":"Re: [PATCH] show-branch: fix crash with long ref name","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-02-16T12:40:00Z","receivedAt":"2017-02-16T12:40:08Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Feb 15, 2017 at 10:40 PM, Jeff King wrote:\n> On Tue, Feb 14, 2017 at 10:29:46PM +0100, Christian Couder wrote:\n>\n>> > I notice Christian's patch added a few tests. I don't know if we'd want\n>> > to squash them in (I didn't mean to override his patch at all; I was\n>> > about to send mine out when I noticed his, and I wondered if we wanted\n>> > to combine the two efforts).\n>>\n>> I think it would be nice to have at least one test. Feel free to\n>> squash mine if you want.\n>\n> I started to add some tests, but I had second thoughts. It _is_ nice\n> to show off the fix, but as far as regressions go, this specific case is\n> unlikely to come up again. What would be more valuable, I think, is a\n> test script which set up a very long refname (not just 150 bytes or\n> whatever) and ran it through a series of git commands.\n\nI agree that a test script running through a series of command with\nlong refnames would be great.\n\nBut I think the refname should not necesarily be too long. As I wrote\nin the commit message of my patch, if the ref name had been much\nlonger the crash would not have happened because the ref could not\nhave been created in the first place.\n\nSo the best would be to run through a series of commands with a\nrefname ranging from let's say 80 chars to 300 chars.\n\nThat would have a chance to catch crashes due to legacy code using for\nexample things like `char stuff[128]` or `char stuff[256]`.\n\nImplementing those tests could have started with something like the\ntest case I sent, but as it would in the end be about many different\ncommands, one can see it as part of a different topic.\n\n> But then you run into all sorts of portability annoyances with pathname\n> restrictions (you can hack around creation by writing the refname\n> directly into packed-refs, but most manipulations will want to take the\n> .lock in the filesystem).\n\nYeah, but if a crash doesn't happen because we die() as the ref is too\nlong for the file system, we could detect that and make the test\nsucceed.\n\n> So I dunno. It seems like being thorough is a\n> lot of hassle for not much gain. Being not-thorough is easy, but is\n> mostly a token that is unlikely to find any real bugs.\n\nYeah, if we really care, it might be better to start using a fuzzer or\na property based testing tool instead of bothering with these kind of\ntests by ourselves, which is also a different topic.\n\n> So I punted, at least for now.\n\nOk, no problem.\n"},{"id":"311840","messageId":"20170217050339.i4s2esabk3uvi5yb@sigill.intra.peff.net","threadId":"45139","inReplyTo":"CAP8UFD20=zVy=1Tr4D1Rxf+a4yR_X2zmexNCTNKV5RSF9ueYrg@mail.gmail.com","subject":"Re: [PATCH] show-branch: fix crash with long ref name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-17T05:03:39Z","receivedAt":"2017-02-17T05:03:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 16, 2017 at 01:40:00PM +0100, Christian Couder wrote:\n\n> > I started to add some tests, but I had second thoughts. It _is_ nice\n> > to show off the fix, but as far as regressions go, this specific case is\n> > unlikely to come up again. What would be more valuable, I think, is a\n> > test script which set up a very long refname (not just 150 bytes or\n> > whatever) and ran it through a series of git commands.\n> \n> I agree that a test script running through a series of command with\n> long refnames would be great.\n> \n> But I think the refname should not necesarily be too long. As I wrote\n> in the commit message of my patch, if the ref name had been much\n> longer the crash would not have happened because the ref could not\n> have been created in the first place.\n\nRight, I think there's a tension there. Too short and it is not\ninteresting, and too long and things start to fail for uninteresting\nreasons (e.g., your filesystem can't handle the name).\n\n> So the best would be to run through a series of commands with a\n> refname ranging from let's say 80 chars to 300 chars.\n> \n> That would have a chance to catch crashes due to legacy code using for\n> example things like `char stuff[128]` or `char stuff[256]`.\n\nBut that doesn't catch `char stuff[512]`. I think you'd really want a\nrange of sizes, and to test all of them. Worse, though, is it's not\nclear how you decide when a test has failed. Obviously smashing the\nstack is a bad outcome, but part of the goal would presumably be to\nflush out unnecessary length limitations elsewhere.\n\nI got about that far in my thinking before saying \"wow, this is getting\nto be complicated for not much gain\".\n\n> > So I dunno. It seems like being thorough is a\n> > lot of hassle for not much gain. Being not-thorough is easy, but is\n> > mostly a token that is unlikely to find any real bugs.\n> \n> Yeah, if we really care, it might be better to start using a fuzzer or\n> a property based testing tool instead of bothering with these kind of\n> tests by ourselves, which is also a different topic.\n\nYes, I'd agree that a fuzzer is probably a better choice. I played with\nAFL a bit back when it came out, but I never got it to turn up anything\nuseful.\n\nI am disappointed that this obvious memory problem survived for so long.\nI did quite a bit of auditing for such problems a year or two ago, but I\nfocused on problematic functions like strcpy, sprintf, etc.\n\nIt's easy to use memcpy() wrong, too, but it's hard to audit. There are\na lot of calls, and you have to match up the copied length with a value\ncomputed elsewhere. I traded a lot of them out back then for safer\nvariants (like the FLEX_ALLOC stuff), but many calls just fundamentally\nneed something unsafe like memcpy to get their job done.\n\n-Peff\n"}]}