On Sat, Aug 18, 2018 at 6:16 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> > 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@xxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
> 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;
I would suggest
if (n < len) continue;
..
if (!strncasecmp(p->keyword, src, len) && (n == len || !isalnum(src[len]))) {
so we colorize a single line that looks like "warning" as well
Other than that, LGTM.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
