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>

diff --git a/sideband.c b/sideband.c
index 1c6bb0e25b7b7813dbe3bfec704ed8d96f632842..368647acf8c04decee14ceea2f03c535a206ad18 100644 (file)
--- 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 eb1b8aa05df01b4d729949a8851867cbf2ceaa19..f81b6813c03090fe932852f86ce4b3517c914d7a 100755 (executable)
--- 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 &&