git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Han-Wen Nienhuys <hanwen@google.com>,
	sunshine@sunshineco.com, git@vger.kernel.org
Subject: Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output
Date: Sat, 18 Aug 2018 09:16:28 -0700	[thread overview]
Message-ID: <xmqqa7pjabvn.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <xmqqin47acir.fsf@gitster-ct.c.googlers.com> (Junio C. Hamano's message of "Sat, 18 Aug 2018 09:02:36 -0700")

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>>> -	strbuf_add(dest, src, n);
>>>> +	if (0 < n)
>>>> +		strbuf_add(dest, src, n);
>>>
>>> This check seems unnecessary.  strbuf_add can cope fine with !n.
>>
>> I was primarily interested in catching negatives, and !n was a mere
>> optimization, but you are correct to point out that negative n at
>> this point in the codeflow is a BUG().
>
> Actually, let's just lose the conditional.  strbuf_add() would catch
> and issue an error message when it notices that we fed negative
> count anyway ;-).

So I'll have this applied on top of the original topic to prevent a
buggy version from escaping the lab.

-- >8 --
Subject: [PATCH] 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) made number of strncasecmp() calls to see if the remaining
     bytes in src[] matched keywords, reading beyond the end of the
     array (this actually happens even if n does not go negative),
     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.

Fix both issues by making sure we do not go beyond &src[n].

In the longer term we may want to accept size_t as parameter for
clarity (even though we know that a sideband message we are painting
typically would fit on a line on a terminal and int is sufficient).
Write it down as a NEEDSWORK comment.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sideband.c                          |  8 ++++++--
 t/t5409-colorize-remote-messages.sh | 14 ++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/sideband.c b/sideband.c
index 1c6bb0e25b..368647acf8 100644
--- a/sideband.c
+++ b/sideband.c
@@ -65,6 +65,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
  * Optionally highlight one keyword in remote output if it appears at the start
  * of the line. This should be called for a single line only, which is
  * passed as the first N characters of the SRC array.
+ *
+ * NEEDSWORK: use "size_t n" instead for clarity.
  */
 static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 {
@@ -75,7 +77,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 		return;
 	}
 
-	while (isspace(*src)) {
+	while (0 < n && isspace(*src)) {
 		strbuf_addch(dest, *src);
 		src++;
 		n--;
@@ -84,6 +86,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
 		struct keyword_entry *p = keywords + i;
 		int len = strlen(p->keyword);
+
+		if (n <= len)
+			continue;
 		/*
 		 * Match case insensitively, so we colorize output from existing
 		 * servers regardless of the case that they use for their
@@ -101,7 +106,6 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 	}
 
 	strbuf_add(dest, src, n);
-
 }
 
 
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index eb1b8aa05d..f81b6813c0 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-748-gfa03cdc39b


  reply	other threads:[~2018-08-18 16:16 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-07 12:51 [PATCH v7 0/1] sideband: highlight keywords in remote sideband output Han-Wen Nienhuys
2018-08-07 12:51 ` [PATCH v7 1/1] " Han-Wen Nienhuys
2018-08-17 18:33   ` Junio C Hamano
2018-08-17 18:44     ` Re* " Junio C Hamano
2018-08-18  6:09       ` Jonathan Nieder
2018-08-18 14:40         ` Junio C Hamano
2018-08-18 16:02           ` Junio C Hamano
2018-08-18 16:16             ` Junio C Hamano [this message]
2018-08-18 23:22               ` Jeff King
2018-08-20 14:21                 ` Junio C Hamano
2018-08-20 12:21               ` Han-Wen Nienhuys
2018-08-20 12:21                 ` Han-Wen Nienhuys
2018-08-20 14:32                 ` Junio C Hamano
2018-08-18  6:35       ` Jonathan Nieder
2018-08-18 16:06         ` Junio C Hamano
2018-08-07 21:01 ` [PATCH v7 0/1] " Junio C Hamano
2018-08-08 13:12   ` Han-Wen Nienhuys

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqa7pjabvn.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=hanwen@google.com \
    --cc=jrnieder@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).