wt-status: fix an invalid memory read, clean up
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
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
@@ -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
PATCH 2/5 wt-status: exit early using goto in wt_shortstatus_print_tracking()
In reply to René ScharfeDeduplicate 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
@@ -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
PATCH 3/5 wt-status: avoid building bogus branch name with detached HEAD
In reply to René ScharfeIf 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
@@ -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
@@ -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
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 tobranch_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.
Signed-off-by Rene Scharfe <[email protected]>--- t/t7060-wtstatus.sh | 2 +- wt-status.c | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-)t/t7060-wtstatus.sh +1
-1@@ -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@@ -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 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 tobranch_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_namevariable that is not involved in the actual computation (i.e. call tobranch_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), letsbranch_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é
PATCH 4/5 wt-status: don't skip a magical number of characters blindly
In reply to René ScharfeUse 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
@@ -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
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.Signed-off-by Rene Scharfe <[email protected]>--- wt-status.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)wt-status.c +0
-0
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
-0color_fprintf (s->fp, branch_color_local,"%s" , branch_name);
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@@ -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->branchis the refname that is l10n independent;branch_nameis 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.
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
@@ -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