Skip to contentSkip to conversation

wt-status: fix an invalid memory read, clean up

PATCH In review René Scharfe 2 participants 10 messages Jun 15, 2016

lore
Share & tools
lore
#0 lore raw

cover wt-status: fix an invalid memory read, clean up

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(-)
Signature · 1 line
2.6.2
#1 lore raw

PATCH 1/5 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)
---
 t/t7060-wtstatus.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
t/t7060-wtstatus.sh +14 -0
index 32d8176..e6af772 100755
@@ -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
Signature · 1 line
2.6.2
#2 lore raw

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

In reply to René Scharfe

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

---
 wt-status.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
wt-status.c +5 -8
index 3e3b8c0..083328f 100644
@@ -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);
 }
 
Signature · 1 line
2.6.2
#3 lore raw

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

In reply to 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.

---
 t/t7060-wtstatus.sh |  2 +-
 wt-status.c         | 15 +++++++++------
 2 files changed, 10 insertions(+), 7 deletions(-)
t/t7060-wtstatus.sh +1 -1
index e6af772..44bf1d8 100755
@@ -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 &&
wt-status.c +9 -6
index 083328f..e206cc9 100644
@@ -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);
 
Signature · 1 line
2.6.2
#4 lore raw

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

Quoted 4 lines · 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.

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.

Quoted 54 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.

---
 t/t7060-wtstatus.sh |  2 +-
 wt-status.c         | 15 +++++++++------
 2 files changed, 10 insertions(+), 7 deletions(-)
t/t7060-wtstatus.sh +1 -1
index e6af772..44bf1d8 100755
@@ -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 &&
wt-status.c +9 -6
index 083328f..e206cc9 100644
@@ -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);
#5 lore raw

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

Am 01.11.2015 um 18:50 schrieb Junio C Hamano:

Quoted 14 lines
Quoted 4 lines · 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.

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()).

Quoted 6 lines

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é

#6 lore raw

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

In reply to 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.

---
 wt-status.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
wt-status.c +1 -1
index e206cc9..42ea15e 100644
@@ -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);
 
Signature · 1 line
2.6.2
#7 lore raw

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

Quoted 14 lines · 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.

---
 wt-status.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
wt-status.c +0 -0
index e206cc9..42ea15e 100644

wt-status.c static void wt_shortstatus_print_tracking(struct wt_status *s) · line 1659

Quoted 5 lines
wt-status.c +1 -1
@@ -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.

Quoted 1 line
wt-status.c +0 -0
 	color_fprintf(s->fp, branch_color_local, "%s", branch_name);
#8 lore raw

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

Quoted 8 lines · Junio C Hamano
Quoted 8 lines
wt-status.c +0 -0
index e206cc9..42ea15e 100644
@@ -1656,7 +1656,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	if (starts_with(branch_name, "refs/heads/"))
 		branch_name += 11;
 
Quoted 5 lines

wt-status.c static void wt_shortstatus_print_tracking(struct wt_status *s) · line 1659

Quoted 2 lines
wt-status.c +1 -1
-	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.

#9 lore raw

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

In reply to René Scharfe
---
 wt-status.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)
wt-status.c +15 -21
index 42ea15e..435fc28 100644
@@ -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);
 
Signature · 1 line
2.6.2