{"thread":{"id":"40688","subject":"[PATCH 0/5] wt-status: fix an invalid memory read, clean up","startedAt":"2015-10-31T17:33:13Z","lastAt":"2015-11-01T18:11:06Z","messageCount":10,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"272623","messageId":"5634FB59.1000506@web.de","threadId":"40688","inReplyTo":null,"subject":"[PATCH 0/5] wt-status: fix an invalid memory read, clean up","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-31T17:33:13Z","receivedAt":"2015-10-31T17:33:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Memory is accessed out-of-bounds when try to show --branch info in a\nrepo with a detached HEAD.  Add a test in patch 1 and a fix in patch\n3, as well as some cleanups.\n\nRene Scharfe (5):\n  t7060: add test for status --branch on a detached HEAD\n  wt-status: exit early using goto in wt_shortstatus_print_tracking()\n  wt-status: avoid building bogus branch name with detached HEAD\n  wt-status: don't skip a magical number of characters blindly\n  wt-status: use skip_prefix() to get rid of magic string length constants\n\n t/t7060-wtstatus.sh | 14 ++++++++++++\n wt-status.c         | 64 ++++++++++++++++++++++++-----------------------------\n 2 files changed, 43 insertions(+), 35 deletions(-)\n\n-- \n2.6.2\n"},{"id":"272625","messageId":"5634FBE4.9030406@web.de","threadId":"40688","inReplyTo":"5634FB59.1000506@web.de","subject":"[PATCH 1/5] t7060: add test for status --branch on a detached HEAD","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-31T17:35:32Z","receivedAt":"2015-10-31T17:35:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"This test fails when run under Valgrind because branch_get() gets passed\na bogus branch name pointer:\n\n==62831== Invalid read of size 1\n==62831==    at 0x4F76AE: branch_get (remote.c:1650)\n==62831==    by 0x53499E: wt_shortstatus_print_tracking (wt-status.c:1654)\n==62831==    by 0x53499E: wt_shortstatus_print (wt-status.c:1706)\n==62831==    by 0x428D29: cmd_status (commit.c:1384)\n==62831==    by 0x405D6D: run_builtin (git.c:350)\n==62831==    by 0x405D6D: handle_builtin (git.c:536)\n==62831==    by 0x404F10: run_argv (git.c:582)\n==62831==    by 0x404F10: main (git.c:690)\n==62831==  Address 0x5e89b0b is 6 bytes after a block of size 5 alloc'd\n==62831==    at 0x4C28C4F: malloc (vg_replace_malloc.c:299)\n==62831==    by 0x59579E9: strdup (strdup.c:42)\n==62831==    by 0x52E108: xstrdup (wrapper.c:43)\n==62831==    by 0x5322A6: wt_status_prepare (wt-status.c:130)\n==62831==    by 0x4276E0: status_init_config (commit.c:184)\n==62831==    by 0x428BB8: cmd_status (commit.c:1350)\n==62831==    by 0x405D6D: run_builtin (git.c:350)\n==62831==    by 0x405D6D: handle_builtin (git.c:536)\n==62831==    by 0x404F10: run_argv (git.c:582)\n==62831==    by 0x404F10: main (git.c:690)\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n t/t7060-wtstatus.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh\nindex 32d8176..e6af772 100755\n--- a/t/t7060-wtstatus.sh\n+++ b/t/t7060-wtstatus.sh\n@@ -213,5 +213,19 @@ EOF\n \tgit checkout master\n '\n \n+test_expect_failure 'status --branch with detached HEAD' '\n+\tgit reset --hard &&\n+\tgit checkout master^0 &&\n+\tgit status --branch --porcelain >actual &&\n+\tcat >expected <<-EOF &&\n+\t## HEAD (no branch)\n+\t?? .gitconfig\n+\t?? actual\n+\t?? expect\n+\t?? expected\n+\t?? mdconflict/\n+\tEOF\n+\ttest_i18ncmp expected actual\n+'\n \n test_done\n-- \n2.6.2\n"},{"id":"272626","messageId":"5634FC01.8050601@web.de","threadId":"40688","inReplyTo":"5634FB59.1000506@web.de","subject":"[PATCH 2/5] wt-status: exit early using goto in wt_shortstatus_print_tracking()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-31T17:36:01Z","receivedAt":"2015-10-31T17:36:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Deduplicate printing the line terminator by jumping to the end of the\nfunction.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n wt-status.c | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 3e3b8c0..083328f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1658,10 +1658,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n \tcolor_fprintf(s->fp, branch_color_local, \"%s\", branch_name);\n \n \tif (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) {\n-\t\tif (!base) {\n-\t\t\tfputc(s->null_termination ? '\\0' : '\\n', s->fp);\n-\t\t\treturn;\n-\t\t}\n+\t\tif (!base)\n+\t\t\tgoto conclude;\n \n \t\tupstream_is_gone = 1;\n \t}\n@@ -1671,10 +1669,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n \tcolor_fprintf(s->fp, branch_color_remote, \"%s\", base);\n \tfree((char *)base);\n \n-\tif (!upstream_is_gone && !num_ours && !num_theirs) {\n-\t\tfputc(s->null_termination ? '\\0' : '\\n', s->fp);\n-\t\treturn;\n-\t}\n+\tif (!upstream_is_gone && !num_ours && !num_theirs)\n+\t\tgoto conclude;\n \n #define LABEL(string) (s->no_gettext ? (string) : _(string))\n \n@@ -1695,6 +1691,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n \t}\n \n \tcolor_fprintf(s->fp, header_color, \"]\");\n+ conclude:\n \tfputc(s->null_termination ? '\\0' : '\\n', s->fp);\n }\n \n-- \n2.6.2\n"},{"id":"272627","messageId":"5634FC23.2090703@web.de","threadId":"40688","inReplyTo":"5634FB59.1000506@web.de","subject":"[PATCH 3/5] wt-status: avoid building bogus branch name with detached HEAD","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-31T17:36:35Z","receivedAt":"2015-10-31T17:36:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If we're on a detached HEAD then wt_shortstatus_print_tracking() takes\nthe string \"HEAD (no branch)\", translates it, skips the first eleven\ncharacters and passes the result to branch_get(), which returns a bogus\nresult and accesses memory out of bounds in order to produce it.\nSomehow stat_tracking_info(), which is passed that result, does the\nright thing anyway, i.e. it finds that there is no base.\n\nAvoid the bogus results and memory accesses by checking for HEAD first\nand exiting early in that case.  This fixes t7060 with --valgrind.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n t/t7060-wtstatus.sh |  2 +-\n wt-status.c         | 15 +++++++++------\n 2 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh\nindex e6af772..44bf1d8 100755\n--- a/t/t7060-wtstatus.sh\n+++ b/t/t7060-wtstatus.sh\n@@ -213,7 +213,7 @@ EOF\n \tgit checkout master\n '\n \n-test_expect_failure 'status --branch with detached HEAD' '\n+test_expect_success 'status --branch with detached HEAD' '\n \tgit reset --hard &&\n \tgit checkout master^0 &&\n \tgit status --branch --porcelain >actual &&\ndiff --git a/wt-status.c b/wt-status.c\nindex 083328f..e206cc9 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1644,16 +1644,19 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n \t\treturn;\n \tbranch_name = s->branch;\n \n+\tif (s->is_initial)\n+\t\tcolor_fprintf(s->fp, header_color, _(\"Initial commit on \"));\n+\n+\tif (!strcmp(s->branch, \"HEAD\")) {\n+\t\tcolor_fprintf(s->fp, color(WT_STATUS_NOBRANCH, s), \"%s\",\n+\t\t\t      _(\"HEAD (no branch)\"));\n+\t\tgoto conclude;\n+\t}\n+\n \tif (starts_with(branch_name, \"refs/heads/\"))\n \t\tbranch_name += 11;\n-\telse if (!strcmp(branch_name, \"HEAD\")) {\n-\t\tbranch_name = _(\"HEAD (no branch)\");\n-\t\tbranch_color_local = color(WT_STATUS_NOBRANCH, s);\n-\t}\n \n \tbranch = branch_get(s->branch + 11);\n-\tif (s->is_initial)\n-\t\tcolor_fprintf(s->fp, header_color, _(\"Initial commit on \"));\n \n \tcolor_fprintf(s->fp, branch_color_local, \"%s\", branch_name);\n \n-- \n2.6.2\n"},{"id":"272628","messageId":"5634FC48.7070609@web.de","threadId":"40688","inReplyTo":"5634FB59.1000506@web.de","subject":"[PATCH 4/5] wt-status: don't skip a magical number of characters blindly","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-31T17:37:12Z","receivedAt":"2015-10-31T17:37:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the variable branch_name, which already has \"refs/heads/\" removed,\ninstead of blindly advancing in the ->branch string by 11 bytes.  This\nis safer and less magical.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n wt-status.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex e206cc9..42ea15e 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1656,7 +1656,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n \tif (starts_with(branch_name, \"refs/heads/\"))\n \t\tbranch_name += 11;\n \n-\tbranch = branch_get(s->branch + 11);\n+\tbranch = branch_get(branch_name);\n \n \tcolor_fprintf(s->fp, branch_color_local, \"%s\", branch_name);\n \n-- \n2.6.2\n"},{"id":"272629","messageId":"5634FC67.20001@web.de","threadId":"40688","inReplyTo":"5634FB59.1000506@web.de","subject":"[PATCH 5/5] wt-status: use skip_prefix() to get rid of magic string length constants","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-31T17:37:43Z","receivedAt":"2015-10-31T17:37:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Signed-off-by: Rene Scharfe <l.s.r@web.de>\n---\n wt-status.c | 36 +++++++++++++++---------------------\n 1 file changed, 15 insertions(+), 21 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 42ea15e..435fc28 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -897,15 +897,15 @@ static void wt_status_print_verbose(struct wt_status *s)\n static void wt_status_print_tracking(struct wt_status *s)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *cp, *ep;\n+\tconst char *cp, *ep, *branch_name;\n \tstruct branch *branch;\n \tchar comment_line_string[3];\n \tint i;\n \n \tassert(s->branch && !s->is_initial);\n-\tif (!starts_with(s->branch, \"refs/heads/\"))\n+\tif (!skip_prefix(s->branch, \"refs/heads/\", &branch_name))\n \t\treturn;\n-\tbranch = branch_get(s->branch + 11);\n+\tbranch = branch_get(branch_name);\n \tif (!format_tracking_info(branch, &sb))\n \t\treturn;\n \n@@ -1268,6 +1268,7 @@ static char *read_and_strip_branch(const char *path)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tunsigned char sha1[20];\n+\tconst char *branch_name;\n \n \tif (strbuf_read_file(&sb, git_path(\"%s\", path), 0) <= 0)\n \t\tgoto got_nothing;\n@@ -1276,8 +1277,8 @@ static char *read_and_strip_branch(const char *path)\n \t\tstrbuf_setlen(&sb, sb.len - 1);\n \tif (!sb.len)\n \t\tgoto got_nothing;\n-\tif (starts_with(sb.buf, \"refs/heads/\"))\n-\t\tstrbuf_remove(&sb,0, strlen(\"refs/heads/\"));\n+\tif (skip_prefix(sb.buf, \"refs/heads/\", &branch_name))\n+\t\tstrbuf_remove(&sb, 0, branch_name - sb.buf);\n \telse if (starts_with(sb.buf, \"refs/\"))\n \t\t;\n \telse if (!get_sha1_hex(sb.buf, sha1)) {\n@@ -1308,9 +1309,8 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n \tstruct grab_1st_switch_cbdata *cb = cb_data;\n \tconst char *target = NULL, *end;\n \n-\tif (!starts_with(message, \"checkout: moving from \"))\n+\tif (!skip_prefix(message, \"checkout: moving from \", &message))\n \t\treturn 0;\n-\tmessage += strlen(\"checkout: moving from \");\n \ttarget = strstr(message, \" to \");\n \tif (!target)\n \t\treturn 0;\n@@ -1348,14 +1348,10 @@ static void wt_status_get_detached_from(struct wt_status_state *state)\n \t     /* perhaps sha1 is a tag, try to dereference to a commit */\n \t     ((commit = lookup_commit_reference_gently(sha1, 1)) != NULL &&\n \t      !hashcmp(cb.nsha1, commit->object.sha1)))) {\n-\t\tint ofs;\n-\t\tif (starts_with(ref, \"refs/tags/\"))\n-\t\t\tofs = strlen(\"refs/tags/\");\n-\t\telse if (starts_with(ref, \"refs/remotes/\"))\n-\t\t\tofs = strlen(\"refs/remotes/\");\n-\t\telse\n-\t\t\tofs = 0;\n-\t\tstate->detached_from = xstrdup(ref + ofs);\n+\t\tconst char *from = ref;\n+\t\tif (!skip_prefix(from, \"refs/tags/\", &from))\n+\t\t\tskip_prefix(from, \"refs/remotes/\", &from);\n+\t\tstate->detached_from = xstrdup(from);\n \t} else\n \t\tstate->detached_from =\n \t\t\txstrdup(find_unique_abbrev(cb.nsha1, DEFAULT_ABBREV));\n@@ -1442,9 +1438,7 @@ void wt_status_print(struct wt_status *s)\n \tif (s->branch) {\n \t\tconst char *on_what = _(\"On branch \");\n \t\tconst char *branch_name = s->branch;\n-\t\tif (starts_with(branch_name, \"refs/heads/\"))\n-\t\t\tbranch_name += 11;\n-\t\telse if (!strcmp(branch_name, \"HEAD\")) {\n+\t\tif (!strcmp(branch_name, \"HEAD\")) {\n \t\t\tbranch_status_color = color(WT_STATUS_NOBRANCH, s);\n \t\t\tif (state.rebase_in_progress || state.rebase_interactive_in_progress) {\n \t\t\t\tif (state.rebase_interactive_in_progress)\n@@ -1462,7 +1456,8 @@ void wt_status_print(struct wt_status *s)\n \t\t\t\tbranch_name = \"\";\n \t\t\t\ton_what = _(\"Not currently on any branch.\");\n \t\t\t}\n-\t\t}\n+\t\t} else\n+\t\t\tskip_prefix(branch_name, \"refs/heads/\", &branch_name);\n \t\tstatus_printf(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n \t\tstatus_printf_more(s, branch_status_color, \"%s\", on_what);\n \t\tstatus_printf_more(s, branch_color, \"%s\\n\", branch_name);\n@@ -1653,8 +1648,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n \t\tgoto conclude;\n \t}\n \n-\tif (starts_with(branch_name, \"refs/heads/\"))\n-\t\tbranch_name += 11;\n+\tskip_prefix(branch_name, \"refs/heads/\", &branch_name);\n \n \tbranch = branch_get(branch_name);\n \n-- \n2.6.2\n"},{"id":"272677","messageId":"xmqqbnbdip6y.fsf@gitster.mtv.corp.google.com","threadId":"40688","inReplyTo":"5634FC23.2090703@web.de","subject":"Re: [PATCH 3/5] wt-status: avoid building bogus branch name with detached HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-01T17:50:45Z","receivedAt":"2015-11-01T17:50:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> If we're on a detached HEAD then wt_shortstatus_print_tracking() takes\n> the string \"HEAD (no branch)\", translates it, skips the first eleven\n> characters and passes the result to branch_get(), which returns a bogus\n> result and accesses memory out of bounds in order to produce it.\n\nThe fix is correct, but the above explanation looks \"not quite\" to\nme.\n\nThat \"HEAD (no branch)\" thing is in a separate branch_name variable\nthat is not involved in the actual computation (i.e. call to\nbranch_get()).\n\nThe function gets \"HEAD\" in s->branch, uses that and skips the first\neleven characters (i.e. beyond the end of that string), lets\nbranch_get() to return a garbage and likely missing branch, finds\nthat nobody tracks that, and does the right thing anyway.  If the\ngarbage past the end of the \"HEAD\" happens to have a name of an\nexisting branch, we would get an incorrect result.\n\nThanks.\n\n> Somehow stat_tracking_info(), which is passed that result, does the\n> right thing anyway, i.e. it finds that there is no base.\n>\n> Avoid the bogus results and memory accesses by checking for HEAD first\n> and exiting early in that case.  This fixes t7060 with --valgrind.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  t/t7060-wtstatus.sh |  2 +-\n>  wt-status.c         | 15 +++++++++------\n>  2 files changed, 10 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh\n> index e6af772..44bf1d8 100755\n> --- a/t/t7060-wtstatus.sh\n> +++ b/t/t7060-wtstatus.sh\n> @@ -213,7 +213,7 @@ EOF\n>  \tgit checkout master\n>  '\n>  \n> -test_expect_failure 'status --branch with detached HEAD' '\n> +test_expect_success 'status --branch with detached HEAD' '\n>  \tgit reset --hard &&\n>  \tgit checkout master^0 &&\n>  \tgit status --branch --porcelain >actual &&\n> diff --git a/wt-status.c b/wt-status.c\n> index 083328f..e206cc9 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1644,16 +1644,19 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n>  \t\treturn;\n>  \tbranch_name = s->branch;\n>  \n> +\tif (s->is_initial)\n> +\t\tcolor_fprintf(s->fp, header_color, _(\"Initial commit on \"));\n> +\n> +\tif (!strcmp(s->branch, \"HEAD\")) {\n> +\t\tcolor_fprintf(s->fp, color(WT_STATUS_NOBRANCH, s), \"%s\",\n> +\t\t\t      _(\"HEAD (no branch)\"));\n> +\t\tgoto conclude;\n> +\t}\n> +\n>  \tif (starts_with(branch_name, \"refs/heads/\"))\n>  \t\tbranch_name += 11;\n> -\telse if (!strcmp(branch_name, \"HEAD\")) {\n> -\t\tbranch_name = _(\"HEAD (no branch)\");\n> -\t\tbranch_color_local = color(WT_STATUS_NOBRANCH, s);\n> -\t}\n>  \n>  \tbranch = branch_get(s->branch + 11);\n> -\tif (s->is_initial)\n> -\t\tcolor_fprintf(s->fp, header_color, _(\"Initial commit on \"));\n>  \n>  \tcolor_fprintf(s->fp, branch_color_local, \"%s\", branch_name);\n"},{"id":"272678","messageId":"xmqq7fm1ip51.fsf@gitster.mtv.corp.google.com","threadId":"40688","inReplyTo":"5634FC48.7070609@web.de","subject":"Re: [PATCH 4/5] wt-status: don't skip a magical number of characters blindly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-01T17:51:54Z","receivedAt":"2015-11-01T17:51:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Use the variable branch_name, which already has \"refs/heads/\" removed,\n> instead of blindly advancing in the ->branch string by 11 bytes.  This\n> is safer and less magical.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  wt-status.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index e206cc9..42ea15e 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1656,7 +1656,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n>  \tif (starts_with(branch_name, \"refs/heads/\"))\n>  \t\tbranch_name += 11;\n>  \n> -\tbranch = branch_get(s->branch + 11);\n> +\tbranch = branch_get(branch_name);\n\nIs this correct?  s->branch is the refname that is l10n independent;\nbranch_name is the localized variant for human consumption.\n\n>  \tcolor_fprintf(s->fp, branch_color_local, \"%s\", branch_name);\n"},{"id":"272680","messageId":"xmqq37wpioyh.fsf@gitster.mtv.corp.google.com","threadId":"40688","inReplyTo":"xmqq7fm1ip51.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/5] wt-status: don't skip a magical number of characters blindly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-01T17:55:50Z","receivedAt":"2015-11-01T17:55:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> diff --git a/wt-status.c b/wt-status.c\n>> index e206cc9..42ea15e 100644\n>> --- a/wt-status.c\n>> +++ b/wt-status.c\n>> @@ -1656,7 +1656,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n>>  \tif (starts_with(branch_name, \"refs/heads/\"))\n>>  \t\tbranch_name += 11;\n>>  \n>> -\tbranch = branch_get(s->branch + 11);\n>> +\tbranch = branch_get(branch_name);\n>\n> Is this correct?  s->branch is the refname that is l10n independent;\n> branch_name is the localized variant for human consumption.\n\nAhh, and that convention has been changed at patch 3/5.  Now the\nonly localizable string \"HEAD (no branch)\" never goes into that\nvariable thanks to the code reorganization in 3/5, this variable\nis used only to give us a shortened refname.\n\nOK, I misread the code.  The result is correct.\n"},{"id":"272683","messageId":"563655BA.906@web.de","threadId":"40688","inReplyTo":"xmqqbnbdip6y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/5] wt-status: avoid building bogus branch name with detached HEAD","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-01T18:11:06Z","receivedAt":"2015-11-01T18:11:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.11.2015 um 18:50 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> If we're on a detached HEAD then wt_shortstatus_print_tracking() takes\n>> the string \"HEAD (no branch)\", translates it, skips the first eleven\n>> characters and passes the result to branch_get(), which returns a bogus\n>> result and accesses memory out of bounds in order to produce it.\n>\n> The fix is correct, but the above explanation looks \"not quite\" to\n> me.\n>\n> That \"HEAD (no branch)\" thing is in a separate branch_name variable\n> that is not involved in the actual computation (i.e. call to\n> branch_get()).\n>\n> The function gets \"HEAD\" in s->branch, uses that and skips the first\n> eleven characters (i.e. beyond the end of that string), lets\n> branch_get() to return a garbage and likely missing branch, finds\n> that nobody tracks that, and does the right thing anyway.  If the\n> garbage past the end of the \"HEAD\" happens to have a name of an\n> existing branch, we would get an incorrect result.\n\nAh, yes.  This came from an earlier round which had patch 3 and 4 \nreversed, causing the translated string to be passed to branch_get(). \nThanks for catching the commit message inconsistency!\n\nRené\n"}]}