Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Aug 18, 2018, 06:35 UTC
- Message-ID
- <20180818063534.GD241538@aiede.svl.corp.google.com>
- In-Reply-To
- <xmqq8t54bzo4.fsf_-_@gitster-ct.c.googlers.com>
Junio C Hamano wrote:
Show 15 quoted lines
> Subject: sideband: do not read beyond the end of input > > The caller of maybe_colorize_sideband() gives a counted buffer > <src,n>, but the callee checked *src as if it were a NUL terminated > buffer. If src[] had all isspace() bytes in it, we would have made > n negative, and then (1) called number of strncasecmp() to see if > the remaining bytes in src[] matched keywords, reading beyond the > end of the array, and/or (2) called strbuf_add() with negative > count, most likely triggering the "you want to use way too much > memory" error due to unsigned integer overflow. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > sideband.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-)
And here are some tests to squash in.
A sideband line consisting entirely of spaces causes us to overflow our buffer and end up with a negative number of remaining characters, ultimately resulting in the message
fatal: you want to use way too much memory
when parsing it in order to add color.
We also forget to limit a strncasecmp and isalnum looking for keywords to color in to the memory region passed in. Fortunately all callers put a delimiter character (\0, \r, or \n) after the end of the region so this does not cause trouble in practice. Add a test for futureproofing to protect the new bounds checking code in case that ever changes.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Thanks again, Jonathan
t/t5409-colorize-remote-messages.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index eb1b8aa05df..f81b6813c03 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -15,6 +15,8 @@ test_expect_success 'setup' ' echo warning: warning echo prefixerror: error echo " " "error: leading space" + echo " " + echo Err exit 0 EOF echo 1 >file && @@ -44,6 +46,12 @@ test_expect_success 'whole words at line start' ' grep "prefixerror: error" decoded ' +test_expect_success 'short line' ' + git -C child -c color.remote=always push -f origin HEAD:short-line 2>output && + test_decode_color <output >decoded && + grep "remote: Err" decoded +' + test_expect_success 'case-insensitive' ' git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output && cat output && @@ -58,6 +66,12 @@ test_expect_success 'leading space' ' grep " <BOLD;RED>error<RESET>: leading space" decoded ' +test_expect_success 'spaces only' ' + git -C child -c color.remote=always push -f origin HEAD:only-space 2>output && + test_decode_color <output >decoded && + grep "remote: " decoded +' + test_expect_success 'no coloring for redirected output' ' git --git-dir child/.git push -f origin HEAD:refs/heads/redirected-output 2>output && test_decode_color <output >decoded &&
-- 2.18.0.1017.ga543ac7ca45