Re: [PATCH 3/5] wt-status: avoid building bogus branch name with detached HEAD
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 1, 2015, 17:50 UTC
- Message-ID
- <xmqqbnbdip6y.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <5634FC23.2090703@web.de>
René Scharfe <l.s.r@web.de> 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.
Show 54 quoted lines
> 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 <l.s.r@web.de>
> ---
> 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);