From 7ca8c18950c3f843cedba897b44f9c79b5ab44eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 31 Oct 2015 18:35:32 +0100 Subject: t7060: add test for status --branch on a detached HEAD 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 Signed-off-by: Junio C Hamano diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh index 741ec08..879d0c1 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 -- cgit v0.10.2-6-g49f6 From bcf8cc25acb3378bf62f2cfc27c28302585841c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 31 Oct 2015 18:36:01 +0100 Subject: wt-status: exit early using goto in wt_shortstatus_print_tracking() Deduplicate printing the line terminator by jumping to the end of the function. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano diff --git a/wt-status.c b/wt-status.c index e8c39ef..ac05b9b 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1535,10 +1535,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; } @@ -1548,10 +1546,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)) @@ -1572,6 +1568,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); } -- cgit v0.10.2-6-g49f6 From baf0a3e47d807b63e9fc5628caa455d1da91dd6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 31 Oct 2015 18:36:35 +0100 Subject: wt-status: avoid building bogus branch name with detached HEAD 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 Signed-off-by: Junio C Hamano diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh index 879d0c1..58df3f3 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 ac05b9b..0e4a04e 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1521,16 +1521,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); -- cgit v0.10.2-6-g49f6 From 8d8325f8ee3ef187a22743ccbedb978e52e3d095 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 31 Oct 2015 18:37:12 +0100 Subject: wt-status: don't skip a magical number of characters blindly 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 Signed-off-by: Junio C Hamano diff --git a/wt-status.c b/wt-status.c index 0e4a04e..ca68111 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1533,7 +1533,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); -- cgit v0.10.2-6-g49f6 From c72b49dfab8381abfd947db53c1c9e2da9593ab6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 31 Oct 2015 18:37:43 +0100 Subject: wt-status: use skip_prefix() to get rid of magic string length constants Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano diff --git a/wt-status.c b/wt-status.c index ca68111..62383a2 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; @@ -1154,6 +1154,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; @@ -1162,8 +1163,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)) { @@ -1194,9 +1195,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; @@ -1228,14 +1228,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)); @@ -1322,9 +1318,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) { on_what = _("rebase in progress; onto "); @@ -1339,7 +1333,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); @@ -1530,8 +1525,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); -- cgit v0.10.2-6-g49f6