[PATCH 0/5] wt-status: fix an invalid memory read, clean up

[PATCH 0/5] wt-status: fix an invalid memory read, clean up

[René Scharfe]

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

[PATCH 1/5] t7060: add test for status --branch on a detached HEAD

[René Scharfe]

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 <l.s.r@web.de>
---
 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

[PATCH 2/5] wt-status: exit early using goto in wt_shortstatus_print_tracking()

[René Scharfe]

Deduplicate printing the line terminator by jumping to the end of the
function.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 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

[PATCH 3/5] wt-status: avoid building bogus branch name with detached HEAD

[René Scharfe]

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 <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);
 
-- 
2.6.2

[PATCH 4/5] wt-status: don't skip a magical number of characters blindly

[René Scharfe]

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 <l.s.r@web.de>
---
 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

[PATCH 5/5] wt-status: use skip_prefix() to get rid of magic string length constants

[René Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 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

Re: [PATCH 3/5] wt-status: avoid building bogus branch name with detached HEAD

[Junio C Hamano]

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.

> 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);

Re: [PATCH 4/5] wt-status: don't skip a magical number of characters blindly

[Junio C Hamano]

René Scharfe <l.s.r@web.de> 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 <l.s.r@web.de>
> ---
>  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);

Re: [PATCH 4/5] wt-status: don't skip a magical number of characters blindly

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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.

Re: [PATCH 3/5] wt-status: avoid building bogus branch name with detached HEAD

[René Scharfe]

Am 01.11.2015 um 18:50 schrieb Junio C Hamano:
> 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.

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é