# [PATCH 0/5] wt-status: fix an invalid memory read, clean up 10 messages from 2015-10-31 to 2015-11-01. Participants: René Scharfe, Junio C Hamano. Thread: https://gitlist.dev/t/40688 ## René Scharfe, 2015-10-31 17:33 Subject: [PATCH 0/5] wt-status: fix an invalid memory read, clean up Message-ID: <5634FB59.1000506@web.de> URL: https://gitlist.dev/e/5634FB59.1000506%40web.de ``` Memory is accessed out-of-bounds when try to show --branch info in a repo with a detached HEAD. Add a test in patch 1 and a fix in patch 3, as well as some cleanups. Rene Scharfe (5): t7060: add test for status --branch on a detached HEAD wt-status: exit early using goto in wt_shortstatus_print_tracking() wt-status: avoid building bogus branch name with detached HEAD wt-status: don't skip a magical number of characters blindly wt-status: use skip_prefix() to get rid of magic string length constants t/t7060-wtstatus.sh | 14 ++++++++++++ wt-status.c | 64 ++++++++++++++++++++++++----------------------------- 2 files changed, 43 insertions(+), 35 deletions(-) -- 2.6.2 ``` ## René Scharfe, 2015-10-31 17:35 Subject: [PATCH 1/5] t7060: add test for status --branch on a detached HEAD Message-ID: <5634FBE4.9030406@web.de> URL: https://gitlist.dev/e/5634FBE4.9030406%40web.de In-Reply-To: <5634FB59.1000506@web.de> ``` This test fails when run under Valgrind because branch_get() gets passed a bogus branch name pointer: ==62831== Invalid read of size 1 ==62831== at 0x4F76AE: branch_get (remote.c:1650) ==62831== by 0x53499E: wt_shortstatus_print_tracking (wt-status.c:1654) ==62831== by 0x53499E: wt_shortstatus_print (wt-status.c:1706) ==62831== by 0x428D29: cmd_status (commit.c:1384) ==62831== by 0x405D6D: run_builtin (git.c:350) ==62831== by 0x405D6D: handle_builtin (git.c:536) ==62831== by 0x404F10: run_argv (git.c:582) ==62831== by 0x404F10: main (git.c:690) ==62831== Address 0x5e89b0b is 6 bytes after a block of size 5 alloc'd ==62831== at 0x4C28C4F: malloc (vg_replace_malloc.c:299) ==62831== by 0x59579E9: strdup (strdup.c:42) ==62831== by 0x52E108: xstrdup (wrapper.c:43) ==62831== by 0x5322A6: wt_status_prepare (wt-status.c:130) ==62831== by 0x4276E0: status_init_config (commit.c:184) ==62831== by 0x428BB8: cmd_status (commit.c:1350) ==62831== by 0x405D6D: run_builtin (git.c:350) ==62831== by 0x405D6D: handle_builtin (git.c:536) ==62831== by 0x404F10: run_argv (git.c:582) ==62831== by 0x404F10: main (git.c:690) Signed-off-by: Rene Scharfe --- t/t7060-wtstatus.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh index 32d8176..e6af772 100755 --- a/t/t7060-wtstatus.sh +++ b/t/t7060-wtstatus.sh @@ -213,5 +213,19 @@ EOF git checkout master ' +test_expect_failure 'status --branch with detached HEAD' ' + git reset --hard && + git checkout master^0 && + git status --branch --porcelain >actual && + cat >expected <<-EOF && + ## HEAD (no branch) + ?? .gitconfig + ?? actual + ?? expect + ?? expected + ?? mdconflict/ + EOF + test_i18ncmp expected actual +' test_done -- 2.6.2 ``` ## René Scharfe, 2015-10-31 17:36 Subject: [PATCH 2/5] wt-status: exit early using goto in wt_shortstatus_print_tracking() Message-ID: <5634FC01.8050601@web.de> URL: https://gitlist.dev/e/5634FC01.8050601%40web.de In-Reply-To: <5634FB59.1000506@web.de> ``` Deduplicate printing the line terminator by jumping to the end of the function. Signed-off-by: Rene Scharfe --- wt-status.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/wt-status.c b/wt-status.c index 3e3b8c0..083328f 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1658,10 +1658,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) color_fprintf(s->fp, branch_color_local, "%s", branch_name); if (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) { - if (!base) { - fputc(s->null_termination ? '\0' : '\n', s->fp); - return; - } + if (!base) + goto conclude; upstream_is_gone = 1; } @@ -1671,10 +1669,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) color_fprintf(s->fp, branch_color_remote, "%s", base); free((char *)base); - if (!upstream_is_gone && !num_ours && !num_theirs) { - fputc(s->null_termination ? '\0' : '\n', s->fp); - return; - } + if (!upstream_is_gone && !num_ours && !num_theirs) + goto conclude; #define LABEL(string) (s->no_gettext ? (string) : _(string)) @@ -1695,6 +1691,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) } color_fprintf(s->fp, header_color, "]"); + conclude: fputc(s->null_termination ? '\0' : '\n', s->fp); } -- 2.6.2 ``` ## René Scharfe, 2015-10-31 17:36 Subject: [PATCH 3/5] wt-status: avoid building bogus branch name with detached HEAD Message-ID: <5634FC23.2090703@web.de> URL: https://gitlist.dev/e/5634FC23.2090703%40web.de In-Reply-To: <5634FB59.1000506@web.de> ``` If we're on a detached HEAD then wt_shortstatus_print_tracking() takes the string "HEAD (no branch)", translates it, skips the first eleven characters and passes the result to branch_get(), which returns a bogus result and accesses memory out of bounds in order to produce it. Somehow stat_tracking_info(), which is passed that result, does the right thing anyway, i.e. it finds that there is no base. Avoid the bogus results and memory accesses by checking for HEAD first and exiting early in that case. This fixes t7060 with --valgrind. Signed-off-by: Rene Scharfe --- t/t7060-wtstatus.sh | 2 +- wt-status.c | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh index e6af772..44bf1d8 100755 --- a/t/t7060-wtstatus.sh +++ b/t/t7060-wtstatus.sh @@ -213,7 +213,7 @@ EOF git checkout master ' -test_expect_failure 'status --branch with detached HEAD' ' +test_expect_success 'status --branch with detached HEAD' ' git reset --hard && git checkout master^0 && git status --branch --porcelain >actual && diff --git a/wt-status.c b/wt-status.c index 083328f..e206cc9 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1644,16 +1644,19 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) return; branch_name = s->branch; + if (s->is_initial) + color_fprintf(s->fp, header_color, _("Initial commit on ")); + + if (!strcmp(s->branch, "HEAD")) { + color_fprintf(s->fp, color(WT_STATUS_NOBRANCH, s), "%s", + _("HEAD (no branch)")); + goto conclude; + } + if (starts_with(branch_name, "refs/heads/")) branch_name += 11; - else if (!strcmp(branch_name, "HEAD")) { - branch_name = _("HEAD (no branch)"); - branch_color_local = color(WT_STATUS_NOBRANCH, s); - } branch = branch_get(s->branch + 11); - if (s->is_initial) - color_fprintf(s->fp, header_color, _("Initial commit on ")); color_fprintf(s->fp, branch_color_local, "%s", branch_name); -- 2.6.2 ``` ## René Scharfe, 2015-10-31 17:37 Subject: [PATCH 4/5] wt-status: don't skip a magical number of characters blindly Message-ID: <5634FC48.7070609@web.de> URL: https://gitlist.dev/e/5634FC48.7070609%40web.de In-Reply-To: <5634FB59.1000506@web.de> ``` Use the variable branch_name, which already has "refs/heads/" removed, instead of blindly advancing in the ->branch string by 11 bytes. This is safer and less magical. Signed-off-by: Rene Scharfe --- wt-status.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wt-status.c b/wt-status.c index e206cc9..42ea15e 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1656,7 +1656,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) if (starts_with(branch_name, "refs/heads/")) branch_name += 11; - branch = branch_get(s->branch + 11); + branch = branch_get(branch_name); color_fprintf(s->fp, branch_color_local, "%s", branch_name); -- 2.6.2 ``` ## René Scharfe, 2015-10-31 17:37 Subject: [PATCH 5/5] wt-status: use skip_prefix() to get rid of magic string length constants Message-ID: <5634FC67.20001@web.de> URL: https://gitlist.dev/e/5634FC67.20001%40web.de In-Reply-To: <5634FB59.1000506@web.de> ``` Signed-off-by: Rene Scharfe --- wt-status.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/wt-status.c b/wt-status.c index 42ea15e..435fc28 100644 --- a/wt-status.c +++ b/wt-status.c @@ -897,15 +897,15 @@ static void wt_status_print_verbose(struct wt_status *s) static void wt_status_print_tracking(struct wt_status *s) { struct strbuf sb = STRBUF_INIT; - const char *cp, *ep; + const char *cp, *ep, *branch_name; struct branch *branch; char comment_line_string[3]; int i; assert(s->branch && !s->is_initial); - if (!starts_with(s->branch, "refs/heads/")) + if (!skip_prefix(s->branch, "refs/heads/", &branch_name)) return; - branch = branch_get(s->branch + 11); + branch = branch_get(branch_name); if (!format_tracking_info(branch, &sb)) return; @@ -1268,6 +1268,7 @@ static char *read_and_strip_branch(const char *path) { struct strbuf sb = STRBUF_INIT; unsigned char sha1[20]; + const char *branch_name; if (strbuf_read_file(&sb, git_path("%s", path), 0) <= 0) goto got_nothing; @@ -1276,8 +1277,8 @@ static char *read_and_strip_branch(const char *path) strbuf_setlen(&sb, sb.len - 1); if (!sb.len) goto got_nothing; - if (starts_with(sb.buf, "refs/heads/")) - strbuf_remove(&sb,0, strlen("refs/heads/")); + if (skip_prefix(sb.buf, "refs/heads/", &branch_name)) + strbuf_remove(&sb, 0, branch_name - sb.buf); else if (starts_with(sb.buf, "refs/")) ; else if (!get_sha1_hex(sb.buf, sha1)) { @@ -1308,9 +1309,8 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, struct grab_1st_switch_cbdata *cb = cb_data; const char *target = NULL, *end; - if (!starts_with(message, "checkout: moving from ")) + if (!skip_prefix(message, "checkout: moving from ", &message)) return 0; - message += strlen("checkout: moving from "); target = strstr(message, " to "); if (!target) return 0; @@ -1348,14 +1348,10 @@ static void wt_status_get_detached_from(struct wt_status_state *state) /* perhaps sha1 is a tag, try to dereference to a commit */ ((commit = lookup_commit_reference_gently(sha1, 1)) != NULL && !hashcmp(cb.nsha1, commit->object.sha1)))) { - int ofs; - if (starts_with(ref, "refs/tags/")) - ofs = strlen("refs/tags/"); - else if (starts_with(ref, "refs/remotes/")) - ofs = strlen("refs/remotes/"); - else - ofs = 0; - state->detached_from = xstrdup(ref + ofs); + const char *from = ref; + if (!skip_prefix(from, "refs/tags/", &from)) + skip_prefix(from, "refs/remotes/", &from); + state->detached_from = xstrdup(from); } else state->detached_from = xstrdup(find_unique_abbrev(cb.nsha1, DEFAULT_ABBREV)); @@ -1442,9 +1438,7 @@ void wt_status_print(struct wt_status *s) if (s->branch) { const char *on_what = _("On branch "); const char *branch_name = s->branch; - if (starts_with(branch_name, "refs/heads/")) - branch_name += 11; - else if (!strcmp(branch_name, "HEAD")) { + if (!strcmp(branch_name, "HEAD")) { branch_status_color = color(WT_STATUS_NOBRANCH, s); if (state.rebase_in_progress || state.rebase_interactive_in_progress) { if (state.rebase_interactive_in_progress) @@ -1462,7 +1456,8 @@ void wt_status_print(struct wt_status *s) branch_name = ""; on_what = _("Not currently on any branch."); } - } + } else + skip_prefix(branch_name, "refs/heads/", &branch_name); status_printf(s, color(WT_STATUS_HEADER, s), "%s", ""); status_printf_more(s, branch_status_color, "%s", on_what); status_printf_more(s, branch_color, "%s\n", branch_name); @@ -1653,8 +1648,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) goto conclude; } - if (starts_with(branch_name, "refs/heads/")) - branch_name += 11; + skip_prefix(branch_name, "refs/heads/", &branch_name); branch = branch_get(branch_name); -- 2.6.2 ``` ## Junio C Hamano, 2015-11-01 17:50 Subject: Re: [PATCH 3/5] wt-status: avoid building bogus branch name with detached HEAD Message-ID: URL: https://gitlist.dev/e/xmqqbnbdip6y.fsf%40gitster.mtv.corp.google.com In-Reply-To: <5634FC23.2090703@web.de> ``` René Scharfe writes: > If we're on a detached HEAD then wt_shortstatus_print_tracking() takes > the string "HEAD (no branch)", translates it, skips the first eleven > characters and passes the result to branch_get(), which returns a bogus > result and accesses memory out of bounds in order to produce it. The fix is correct, but the above explanation looks "not quite" to me. That "HEAD (no branch)" thing is in a separate branch_name variable that is not involved in the actual computation (i.e. call to branch_get()). The function gets "HEAD" in s->branch, uses that and skips the first eleven characters (i.e. beyond the end of that string), lets branch_get() to return a garbage and likely missing branch, finds that nobody tracks that, and does the right thing anyway. If the garbage past the end of the "HEAD" happens to have a name of an existing branch, we would get an incorrect result. Thanks. > Somehow stat_tracking_info(), which is passed that result, does the > right thing anyway, i.e. it finds that there is no base. > > Avoid the bogus results and memory accesses by checking for HEAD first > and exiting early in that case. This fixes t7060 with --valgrind. > > Signed-off-by: Rene Scharfe > --- > t/t7060-wtstatus.sh | 2 +- > wt-status.c | 15 +++++++++------ > 2 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh > index e6af772..44bf1d8 100755 > --- a/t/t7060-wtstatus.sh > +++ b/t/t7060-wtstatus.sh > @@ -213,7 +213,7 @@ EOF > git checkout master > ' > > -test_expect_failure 'status --branch with detached HEAD' ' > +test_expect_success 'status --branch with detached HEAD' ' > git reset --hard && > git checkout master^0 && > git status --branch --porcelain >actual && > diff --git a/wt-status.c b/wt-status.c > index 083328f..e206cc9 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -1644,16 +1644,19 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) > return; > branch_name = s->branch; > > + if (s->is_initial) > + color_fprintf(s->fp, header_color, _("Initial commit on ")); > + > + if (!strcmp(s->branch, "HEAD")) { > + color_fprintf(s->fp, color(WT_STATUS_NOBRANCH, s), "%s", > + _("HEAD (no branch)")); > + goto conclude; > + } > + > if (starts_with(branch_name, "refs/heads/")) > branch_name += 11; > - else if (!strcmp(branch_name, "HEAD")) { > - branch_name = _("HEAD (no branch)"); > - branch_color_local = color(WT_STATUS_NOBRANCH, s); > - } > > branch = branch_get(s->branch + 11); > - if (s->is_initial) > - color_fprintf(s->fp, header_color, _("Initial commit on ")); > > color_fprintf(s->fp, branch_color_local, "%s", branch_name); ``` ## Junio C Hamano, 2015-11-01 17:51 Subject: Re: [PATCH 4/5] wt-status: don't skip a magical number of characters blindly Message-ID: URL: https://gitlist.dev/e/xmqq7fm1ip51.fsf%40gitster.mtv.corp.google.com In-Reply-To: <5634FC48.7070609@web.de> ``` René Scharfe writes: > Use the variable branch_name, which already has "refs/heads/" removed, > instead of blindly advancing in the ->branch string by 11 bytes. This > is safer and less magical. > > Signed-off-by: Rene Scharfe > --- > wt-status.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/wt-status.c b/wt-status.c > index e206cc9..42ea15e 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -1656,7 +1656,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) > if (starts_with(branch_name, "refs/heads/")) > branch_name += 11; > > - branch = branch_get(s->branch + 11); > + branch = branch_get(branch_name); Is this correct? s->branch is the refname that is l10n independent; branch_name is the localized variant for human consumption. > color_fprintf(s->fp, branch_color_local, "%s", branch_name); ``` ## Junio C Hamano, 2015-11-01 17:55 Subject: Re: [PATCH 4/5] wt-status: don't skip a magical number of characters blindly Message-ID: URL: https://gitlist.dev/e/xmqq37wpioyh.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: >> diff --git a/wt-status.c b/wt-status.c >> index e206cc9..42ea15e 100644 >> --- a/wt-status.c >> +++ b/wt-status.c >> @@ -1656,7 +1656,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) >> if (starts_with(branch_name, "refs/heads/")) >> branch_name += 11; >> >> - branch = branch_get(s->branch + 11); >> + branch = branch_get(branch_name); > > Is this correct? s->branch is the refname that is l10n independent; > branch_name is the localized variant for human consumption. Ahh, and that convention has been changed at patch 3/5. Now the only localizable string "HEAD (no branch)" never goes into that variable thanks to the code reorganization in 3/5, this variable is used only to give us a shortened refname. OK, I misread the code. The result is correct. ``` ## René Scharfe, 2015-11-01 18:11 Subject: Re: [PATCH 3/5] wt-status: avoid building bogus branch name with detached HEAD Message-ID: <563655BA.906@web.de> URL: https://gitlist.dev/e/563655BA.906%40web.de In-Reply-To: ``` Am 01.11.2015 um 18:50 schrieb Junio C Hamano: > René Scharfe writes: > >> If we're on a detached HEAD then wt_shortstatus_print_tracking() takes >> the string "HEAD (no branch)", translates it, skips the first eleven >> characters and passes the result to branch_get(), which returns a bogus >> result and accesses memory out of bounds in order to produce it. > > The fix is correct, but the above explanation looks "not quite" to > me. > > That "HEAD (no branch)" thing is in a separate branch_name variable > that is not involved in the actual computation (i.e. call to > branch_get()). > > The function gets "HEAD" in s->branch, uses that and skips the first > eleven characters (i.e. beyond the end of that string), lets > branch_get() to return a garbage and likely missing branch, finds > that nobody tracks that, and does the right thing anyway. If the > garbage past the end of the "HEAD" happens to have a name of an > existing branch, we would get an incorrect result. Ah, yes. This came from an earlier round which had patch 3 and 4 reversed, causing the translated string to be passed to branch_get(). Thanks for catching the commit message inconsistency! René ```