* [PATCH v7 0/1] sideband: highlight keywords in remote sideband output @ 2018-08-07 12:51 Han-Wen Nienhuys 2018-08-07 12:51 ` [PATCH v7 1/1] " Han-Wen Nienhuys 2018-08-07 21:01 ` [PATCH v7 0/1] " Junio C Hamano 0 siblings, 2 replies; 17+ messages in thread From: Han-Wen Nienhuys @ 2018-08-07 12:51 UTC (permalink / raw) To: gitster, sunshine, jrn; +Cc: git, Han-Wen Nienhuys Fix nits; remove debug printf. Han-Wen Nienhuys (1): sideband: highlight keywords in remote sideband output Documentation/config.txt | 12 +++ help.c | 1 + help.h | 1 + sideband.c | 125 ++++++++++++++++++++++++++-- t/t5409-colorize-remote-messages.sh | 87 +++++++++++++++++++ 5 files changed, 217 insertions(+), 9 deletions(-) create mode 100755 t/t5409-colorize-remote-messages.sh -- 2.18.0.597.ga71716f1ad-goog ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-07 12:51 [PATCH v7 0/1] sideband: highlight keywords in remote sideband output Han-Wen Nienhuys @ 2018-08-07 12:51 ` Han-Wen Nienhuys 2018-08-17 18:33 ` Junio C Hamano 2018-08-07 21:01 ` [PATCH v7 0/1] " Junio C Hamano 1 sibling, 1 reply; 17+ messages in thread From: Han-Wen Nienhuys @ 2018-08-07 12:51 UTC (permalink / raw) To: gitster, sunshine, jrn; +Cc: git, Han-Wen Nienhuys The colorization is controlled with the config setting "color.remote". Supported keywords are "error", "warning", "hint" and "success". They are highlighted if they appear at the start of the line, which is common in error messages, eg. ERROR: commit is missing Change-Id The Git push process itself prints lots of non-actionable messages (eg. bandwidth statistics, object counters for different phases of the process). This obscures actionable error messages that servers may send back. Highlighting keywords in the sideband draws more attention to those messages. The background for this change is that Gerrit does server-side processing to create or update code reviews, and actionable error messages (eg. missing Change-Id) must be communicated back to the user during the push. User research has shown that new users have trouble seeing these messages. The highlighting is done on the client rather than server side, so servers don't have to grow capabilities to understand terminal escape codes and terminal state. It also consistent with the current state where Git is control of the local display (eg. prefixing messages with "remote: "). The highlighting can be configured using color.remote.<KEYWORD> configuration settings. Since the keys are matched case insensitively, we match the keywords case insensitively too. Finally, this solution is backwards compatible: many servers already prefix their messages with "error", and they will benefit from this change without requiring a server update. By contrast, a server-side solution would likely require plumbing the TERM variable through the git protocol, so it would require changes to both server and client. Helped-by: Duy Nguyen <pclouds@gmail.com> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> --- Documentation/config.txt | 12 +++ help.c | 1 + help.h | 1 + sideband.c | 125 ++++++++++++++++++++++++++-- t/t5409-colorize-remote-messages.sh | 87 +++++++++++++++++++ 5 files changed, 217 insertions(+), 9 deletions(-) create mode 100755 t/t5409-colorize-remote-messages.sh diff --git a/Documentation/config.txt b/Documentation/config.txt index 63365dcf3d..33bc1a3def 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1263,6 +1263,18 @@ color.push:: color.push.error:: Use customized color for push errors. +color.remote:: + If set, keywords at the start of the line are highlighted. The + keywords are "error", "warning", "hint" and "success", and are + matched case-insensitively. Maybe set to `always`, `false` (or + `never`) or `auto` (or `true`). If unset, then the value of + `color.ui` is used (`auto` by default). + +color.remote.<slot>:: + Use customized color for each remote keyword. `<slot>` may be + `hint`, `warning`, `success` or `error` which match the + corresponding keyword. + color.showBranch:: A boolean to enable/disable color in the output of linkgit:git-show-branch[1]. May be set to `always`, diff --git a/help.c b/help.c index 3ebf0568db..b6cafcfc0a 100644 --- a/help.c +++ b/help.c @@ -425,6 +425,7 @@ void list_config_help(int for_human) { "color.diff", "<slot>", list_config_color_diff_slots }, { "color.grep", "<slot>", list_config_color_grep_slots }, { "color.interactive", "<slot>", list_config_color_interactive_slots }, + { "color.remote", "<slot>", list_config_color_sideband_slots }, { "color.status", "<slot>", list_config_color_status_slots }, { "fsck", "<msg-id>", list_config_fsck_msg_ids }, { "receive.fsck", "<msg-id>", list_config_fsck_msg_ids }, diff --git a/help.h b/help.h index f8b15323a6..9eab6a3f89 100644 --- a/help.h +++ b/help.h @@ -83,6 +83,7 @@ void list_config_color_diff_slots(struct string_list *list, const char *prefix); void list_config_color_grep_slots(struct string_list *list, const char *prefix); void list_config_color_interactive_slots(struct string_list *list, const char *prefix); void list_config_color_status_slots(struct string_list *list, const char *prefix); +void list_config_color_sideband_slots(struct string_list *list, const char *prefix); void list_config_fsck_msg_ids(struct string_list *list, const char *prefix); #endif /* HELP_H */ diff --git a/sideband.c b/sideband.c index 325bf0e974..1c6bb0e25b 100644 --- a/sideband.c +++ b/sideband.c @@ -1,6 +1,109 @@ #include "cache.h" +#include "color.h" +#include "config.h" #include "pkt-line.h" #include "sideband.h" +#include "help.h" + +struct keyword_entry { + /* + * We use keyword as config key so it should be a single alphanumeric word. + */ + const char *keyword; + char color[COLOR_MAXLEN]; +}; + +static struct keyword_entry keywords[] = { + { "hint", GIT_COLOR_YELLOW }, + { "warning", GIT_COLOR_BOLD_YELLOW }, + { "success", GIT_COLOR_BOLD_GREEN }, + { "error", GIT_COLOR_BOLD_RED }, +}; + +/* Returns a color setting (GIT_COLOR_NEVER, etc). */ +static int use_sideband_colors(void) +{ + static int use_sideband_colors_cached = -1; + + const char *key = "color.remote"; + struct strbuf sb = STRBUF_INIT; + char *value; + int i; + + if (use_sideband_colors_cached >= 0) + return use_sideband_colors_cached; + + if (!git_config_get_string(key, &value)) { + use_sideband_colors_cached = git_config_colorbool(key, value); + } else if (!git_config_get_string("color.ui", &value)) { + use_sideband_colors_cached = git_config_colorbool("color.ui", value); + } else { + use_sideband_colors_cached = GIT_COLOR_AUTO; + } + + for (i = 0; i < ARRAY_SIZE(keywords); i++) { + strbuf_reset(&sb); + strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword); + if (git_config_get_string(sb.buf, &value)) + continue; + if (color_parse(value, keywords[i].color)) + continue; + } + strbuf_release(&sb); + return use_sideband_colors_cached; +} + +void list_config_color_sideband_slots(struct string_list *list, const char *prefix) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(keywords); i++) + list_config_item(list, prefix, keywords[i].keyword); +} + +/* + * 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. + */ +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) +{ + int i; + + if (!want_color_stderr(use_sideband_colors())) { + strbuf_add(dest, src, n); + return; + } + + while (isspace(*src)) { + strbuf_addch(dest, *src); + src++; + n--; + } + + for (i = 0; i < ARRAY_SIZE(keywords); i++) { + struct keyword_entry *p = keywords + i; + int len = strlen(p->keyword); + /* + * Match case insensitively, so we colorize output from existing + * servers regardless of the case that they use for their + * messages. We only highlight the word precisely, so + * "successful" stays uncolored. + */ + if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) { + strbuf_addstr(dest, p->color); + strbuf_add(dest, src, len); + strbuf_addstr(dest, GIT_COLOR_RESET); + n -= len; + src += len; + break; + } + } + + strbuf_add(dest, src, n); + +} + /* * Receive multiplexed output stream over git native protocol. @@ -48,8 +151,10 @@ int recv_sideband(const char *me, int in_stream, int out) len--; switch (band) { case 3: - strbuf_addf(&outbuf, "%s%s%s", outbuf.len ? "\n" : "", - DISPLAY_PREFIX, buf + 1); + strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "", + DISPLAY_PREFIX); + maybe_colorize_sideband(&outbuf, buf + 1, len); + retval = SIDEBAND_REMOTE_ERROR; break; case 2: @@ -69,20 +174,22 @@ int recv_sideband(const char *me, int in_stream, int out) if (!outbuf.len) strbuf_addstr(&outbuf, DISPLAY_PREFIX); if (linelen > 0) { - strbuf_addf(&outbuf, "%.*s%s%c", - linelen, b, suffix, *brk); - } else { - strbuf_addch(&outbuf, *brk); + maybe_colorize_sideband(&outbuf, b, linelen); + strbuf_addstr(&outbuf, suffix); } + + strbuf_addch(&outbuf, *brk); xwrite(2, outbuf.buf, outbuf.len); strbuf_reset(&outbuf); b = brk + 1; } - if (*b) - strbuf_addf(&outbuf, "%s%s", outbuf.len ? - "" : DISPLAY_PREFIX, b); + if (*b) { + strbuf_addstr(&outbuf, outbuf.len ? + "" : DISPLAY_PREFIX); + maybe_colorize_sideband(&outbuf, b, strlen(b)); + } break; case 1: write_or_die(out, buf + 1, len); diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh new file mode 100755 index 0000000000..7057277a5f --- /dev/null +++ b/t/t5409-colorize-remote-messages.sh @@ -0,0 +1,87 @@ +#!/bin/sh + +test_description='remote messages are colorized on the client' + +. ./test-lib.sh + +test_expect_success 'setup' ' + mkdir .git/hooks && + write_script .git/hooks/update <<-\EOF && + echo error: error + echo ERROR: also highlighted + echo hint: hint + echo hinting: not highlighted + echo success: success + echo warning: warning + echo prefixerror: error + echo " " "error: leading space" + exit 0 + EOF + echo 1 >file && + git add file && + git commit -m 1 && + git clone . child && + ( + cd child && + test_commit message2 file content2 + ) +' + +test_expect_success 'keywords' ' + git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/keywords 2>output && + test_decode_color <output >decoded && + grep "<BOLD;RED>error<RESET>: error" decoded && + grep "<YELLOW>hint<RESET>:" decoded && + grep "<BOLD;GREEN>success<RESET>:" decoded && + grep "<BOLD;YELLOW>warning<RESET>:" decoded +' + +test_expect_success 'whole words at line start' ' + git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/whole-words 2>output && + test_decode_color <output >decoded && + grep "<YELLOW>hint<RESET>:" decoded && + grep "hinting: not highlighted" decoded && + grep "prefixerror: error" 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 && + test_decode_color <output >decoded && + grep "<BOLD;RED>error<RESET>: error" decoded && + grep "<BOLD;RED>ERROR<RESET>: also highlighted" decoded +' + +test_expect_success 'leading space' ' + git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space 2>output && cat output && + test_decode_color <output >decoded && + grep " <BOLD;RED>error<RESET>: leading space" 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 && + grep "error: error" decoded +' + +test_expect_success 'push with customized color' ' + git --git-dir child/.git -c color.remote=always -c color.remote.error=blue push -f origin HEAD:refs/heads/customized-color 2>output && + test_decode_color <output >decoded && + grep "<BLUE>error<RESET>:" decoded && + grep "<BOLD;GREEN>success<RESET>:" decoded +' + + +test_expect_success 'error in customized color' ' + git --git-dir child/.git -c color.remote=always -c color.remote.error=i-am-not-a-color push -f origin HEAD:refs/heads/error-customized-color 2>output && + test_decode_color <output >decoded && + grep "<BOLD;GREEN>success<RESET>:" decoded +' + +test_expect_success 'fallback to color.ui' ' + git --git-dir child/.git -c color.ui=always push -f origin HEAD:refs/heads/fallback-color-ui 2>output && + test_decode_color <output >decoded && + grep "<BOLD;RED>error<RESET>: error" decoded +' + +test_done -- 2.18.0.597.ga71716f1ad-goog ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 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 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2018-08-17 18:33 UTC (permalink / raw) To: Han-Wen Nienhuys; +Cc: sunshine, jrn, git Han-Wen Nienhuys <hanwen@google.com> writes: > +/* > + * 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. > + */ > +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) > +{ > + int i; > + > + if (!want_color_stderr(use_sideband_colors())) { > + strbuf_add(dest, src, n); > + return; > + } > + > + while (isspace(*src)) { > + strbuf_addch(dest, *src); > + src++; > + n--; > + } This loop can run out of bytes in src in search of non-space before n gets to zero or negative, and when that happens ... > + for (i = 0; i < ARRAY_SIZE(keywords); i++) { > + struct keyword_entry *p = keywords + i; > + int len = strlen(p->keyword); > + /* > + * Match case insensitively, so we colorize output from existing > + * servers regardless of the case that they use for their > + * messages. We only highlight the word precisely, so > + * "successful" stays uncolored. > + */ > + if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) { ... these access src[] beyond the end of what the caller intended to show us, and also ... > + strbuf_addstr(dest, p->color); > + strbuf_add(dest, src, len); > + strbuf_addstr(dest, GIT_COLOR_RESET); > + n -= len; > + src += len; > + break; > + } > + } > + > + strbuf_add(dest, src, n); ... this will now try to add 0 or negative number of bytes. > + > +} > + Perhaps this will help (not really tested). The second hunk is an unrelated style clean-up. sideband.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sideband.c b/sideband.c index 1c6bb0e25b..d99a559a44 100644 --- a/sideband.c +++ b/sideband.c @@ -75,11 +75,13 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) return; } - while (isspace(*src)) { + while (isspace(*src) && n) { strbuf_addch(dest, *src); src++; n--; } + if (!n) + return; for (i = 0; i < ARRAY_SIZE(keywords); i++) { struct keyword_entry *p = keywords + i; @@ -101,7 +103,6 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) } strbuf_add(dest, src, n); - } ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-17 18:33 ` Junio C Hamano @ 2018-08-17 18:44 ` Junio C Hamano 2018-08-18 6:09 ` Jonathan Nieder 2018-08-18 6:35 ` Jonathan Nieder 0 siblings, 2 replies; 17+ messages in thread From: Junio C Hamano @ 2018-08-17 18:44 UTC (permalink / raw) To: Han-Wen Nienhuys; +Cc: sunshine, jrn, git Junio C Hamano <gitster@pobox.com> writes: > This loop can run out of bytes in src in search of non-space before > n gets to zero or negative, and when that happens ... > >> + for (i = 0; i < ARRAY_SIZE(keywords); i++) { >> + struct keyword_entry *p = keywords + i; >> + int len = strlen(p->keyword); >> + /* >> + * Match case insensitively, so we colorize output from existing >> + * servers regardless of the case that they use for their >> + * messages. We only highlight the word precisely, so >> + * "successful" stays uncolored. >> + */ >> + if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) { > > ... these access src[] beyond the end of what the caller intended to > show us, and also ... Actually, leaving when !n before this loop is insufficient. src[] may have 2 bytes "in" remaining, and we may be trying to see if it begins with "info", for example, and using strncasecmp() with len==4 would of course read beyond the end of src[]. -- >8 -- 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(-) diff --git a/sideband.c b/sideband.c index 1c6bb0e25b..372039247f 100644 --- a/sideband.c +++ b/sideband.c @@ -75,7 +75,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 +84,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 @@ -100,8 +103,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) } } - strbuf_add(dest, src, n); - + if (0 < n) + strbuf_add(dest, src, n); } ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 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 6:35 ` Jonathan Nieder 1 sibling, 1 reply; 17+ messages in thread From: Jonathan Nieder @ 2018-08-18 6:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Han-Wen Nienhuys, sunshine, git (-cc: my @google.com email) Hi, Junio C Hamano wrote: > 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(-) This indeed avoids the "you want to use way too much memory" error when I apply it. > --- a/sideband.c > +++ b/sideband.c > @@ -75,7 +75,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) Not about this patch: should the 'n' parameter be a size_t instead of an int? It doesn't matter in practice (since the caller has an int, it can never be more than INT_MAX) but it might make the intent clearer. Based on inspecting the caller, using an int seems fine. > return; > } > > - while (isspace(*src)) { > + while (0 < n && isspace(*src)) { Yes, we need to check 'n && isspace(*src)' to avoid overflowing the buffer if it consists entirely of spaces. > strbuf_addch(dest, *src); > src++; > n--; > @@ -84,6 +84,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; Using <= instead of < since we look at the character after the word as well. Good. > /* > * Match case insensitively, so we colorize output from existing > * servers regardless of the case that they use for their > * messages. We only highlight the word precisely, so > * "successful" stays uncolored. > */ > if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) { Not about this patch: should this check "&& src[len] == ':'" instead, as discussed upthread? > @@ -100,8 +103,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) > } > } > > - strbuf_add(dest, src, n); > + if (0 < n) > + strbuf_add(dest, src, n); This check seems unnecessary. strbuf_add can cope fine with !n. Should we put assert(n >= 0); or even if (n < 0) BUG(); instead, since the earlier part of the fix guarantees that n >= 0? Thanks for the careful work. With or without such a change, Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-18 6:09 ` Jonathan Nieder @ 2018-08-18 14:40 ` Junio C Hamano 2018-08-18 16:02 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2018-08-18 14:40 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Han-Wen Nienhuys, sunshine, git Jonathan Nieder <jrnieder@gmail.com> writes: >> --- a/sideband.c >> +++ b/sideband.c >> @@ -75,7 +75,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) > > Not about this patch: should the 'n' parameter be a size_t instead of > an int? It doesn't matter in practice (since the caller has an int, > it can never be more than INT_MAX) but it might make the intent > clearer. I tend to agree, but I think a separate "clean-up" patch to do so is more appropriate than rolling it into this fix. >> /* >> * Match case insensitively, so we colorize output from existing >> * servers regardless of the case that they use for their >> * messages. We only highlight the word precisely, so >> * "successful" stays uncolored. >> */ >> if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) { > > Not about this patch: should this check "&& src[len] == ':'" instead, > as discussed upthread? I originally was of an opinion that we should take only lowercase keyword followed by a colon, primarily because that is what we produce. Then "the real world need" told us that we are better off catching the keyword case-insensitively. Recalling that lesson, I am not sure I would support "let's limit to the colon, rejecting any other punctionation letter". In any case, we should make such a policy decision outside a patch like this one that is about fixing a behaviour which all users would consider as a bug regardless of the policy they support. >> @@ -100,8 +103,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) >> } >> } >> >> - 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(). ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-18 14:40 ` Junio C Hamano @ 2018-08-18 16:02 ` Junio C Hamano 2018-08-18 16:16 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2018-08-18 16:02 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Han-Wen Nienhuys, sunshine, git 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 ;-). ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-18 16:02 ` Junio C Hamano @ 2018-08-18 16:16 ` Junio C Hamano 2018-08-18 23:22 ` Jeff King 2018-08-20 12:21 ` Han-Wen Nienhuys 0 siblings, 2 replies; 17+ messages in thread From: Junio C Hamano @ 2018-08-18 16:16 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Han-Wen Nienhuys, sunshine, git 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-18 16:16 ` Junio C Hamano @ 2018-08-18 23:22 ` Jeff King 2018-08-20 14:21 ` Junio C Hamano 2018-08-20 12:21 ` Han-Wen Nienhuys 1 sibling, 1 reply; 17+ messages in thread From: Jeff King @ 2018-08-18 23:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Han-Wen Nienhuys, sunshine, git On Sat, Aug 18, 2018 at 09:16:28AM -0700, Junio C Hamano wrote: > -- >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]. Thanks. I've been sporadically seeing "fatal: you want to use way too much memory" the last few days while running 'next', and finally managed to catch a reproducible case. This patch definitely fixes it. > 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. This "typically" made me nervous about what happens in the non-typical case, but I think we can say something even stronger: the length comes from a pktline, so the maximum is less than 16 bits. I wondered if we might ever call this on the accumulated string from multiple sidebands, but it doesn't look like it. -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-18 23:22 ` Jeff King @ 2018-08-20 14:21 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2018-08-20 14:21 UTC (permalink / raw) To: Jeff King; +Cc: Jonathan Nieder, Han-Wen Nienhuys, sunshine, git Jeff King <peff@peff.net> writes: >> 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. > > This "typically" made me nervous about what happens in the non-typical > case, but I think we can say something even stronger: the length comes > from a pktline, so the maximum is less than 16 bits. I wondered if we > might ever call this on the accumulated string from multiple sidebands, > but it doesn't look like it. I think a sideband packet may be split on one but not the other codepath to result in multiple calls, but I do not think we coalesce them together, so I agree that int is sufficient. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-18 16:16 ` Junio C Hamano 2018-08-18 23:22 ` Jeff King @ 2018-08-20 12:21 ` Han-Wen Nienhuys 2018-08-20 12:21 ` Han-Wen Nienhuys 2018-08-20 14:32 ` Junio C Hamano 1 sibling, 2 replies; 17+ messages in thread From: Han-Wen Nienhuys @ 2018-08-20 12:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Eric Sunshine, git On Sat, Aug 18, 2018 at 6:16 PM Junio C Hamano <gitster@pobox.com> 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@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; 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-20 12:21 ` Han-Wen Nienhuys @ 2018-08-20 12:21 ` Han-Wen Nienhuys 2018-08-20 14:32 ` Junio C Hamano 1 sibling, 0 replies; 17+ messages in thread From: Han-Wen Nienhuys @ 2018-08-20 12:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Eric Sunshine, git and, thanks for cleaning up after me :) -- 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-20 12:21 ` Han-Wen Nienhuys 2018-08-20 12:21 ` Han-Wen Nienhuys @ 2018-08-20 14:32 ` Junio C Hamano 1 sibling, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2018-08-20 14:32 UTC (permalink / raw) To: Han-Wen Nienhuys; +Cc: Jonathan Nieder, Eric Sunshine, git Han-Wen Nienhuys <hanwen@google.com> writes: >> @@ -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 That's the kind of thing I would have mentioned in the initial review of the feature, and I do not think it is a bad idea. I do think it is a bad idea to roll it into this patch, though. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-17 18:44 ` Re* " Junio C Hamano 2018-08-18 6:09 ` Jonathan Nieder @ 2018-08-18 6:35 ` Jonathan Nieder 2018-08-18 16:06 ` Junio C Hamano 1 sibling, 1 reply; 17+ messages in thread From: Jonathan Nieder @ 2018-08-18 6:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Han-Wen Nienhuys, sunshine, jrn, git Junio C Hamano wrote: > 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output 2018-08-18 6:35 ` Jonathan Nieder @ 2018-08-18 16:06 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2018-08-18 16:06 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Han-Wen Nienhuys, sunshine, jrn, git Jonathan Nieder <jrnieder@gmail.com> writes: > And here are some tests to squash in. Thanks, will do. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7 0/1] sideband: highlight keywords in remote sideband output 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-07 21:01 ` Junio C Hamano 2018-08-08 13:12 ` Han-Wen Nienhuys 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2018-08-07 21:01 UTC (permalink / raw) To: Han-Wen Nienhuys; +Cc: sunshine, jrn, git Han-Wen Nienhuys <hanwen@google.com> writes: > Fix nits; remove debug printf. > > Han-Wen Nienhuys (1): > sideband: highlight keywords in remote sideband output > > Documentation/config.txt | 12 +++ > help.c | 1 + > help.h | 1 + > sideband.c | 125 ++++++++++++++++++++++++++-- > t/t5409-colorize-remote-messages.sh | 87 +++++++++++++++++++ > 5 files changed, 217 insertions(+), 9 deletions(-) > create mode 100755 t/t5409-colorize-remote-messages.sh > > -- > 2.18.0.597.ga71716f1ad-goog I'll squash in the following while queuing for <CAPig+cScb_7s4a_ZSVCsr+nBxAHGHZVMZOtnrOgbhZUi96-VFA@mail.gmail.com> Thanks for sticking to the topic. diff --git a/Documentation/config.txt b/Documentation/config.txt index 33bc1a3def..9a38dd2cbb 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1266,7 +1266,7 @@ color.push.error:: color.remote:: If set, keywords at the start of the line are highlighted. The keywords are "error", "warning", "hint" and "success", and are - matched case-insensitively. Maybe set to `always`, `false` (or + matched case-insensitively. May be set to `always`, `false` (or `never`) or `auto` (or `true`). If unset, then the value of `color.ui` is used (`auto` by default). ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v7 0/1] sideband: highlight keywords in remote sideband output 2018-08-07 21:01 ` [PATCH v7 0/1] " Junio C Hamano @ 2018-08-08 13:12 ` Han-Wen Nienhuys 0 siblings, 0 replies; 17+ messages in thread From: Han-Wen Nienhuys @ 2018-08-08 13:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Sunshine, Jonathan Nieder, git On Tue, Aug 7, 2018 at 11:01 PM Junio C Hamano <gitster@pobox.com> wrote: > > Han-Wen Nienhuys <hanwen@google.com> writes: > > > Fix nits; remove debug printf. > > > > Han-Wen Nienhuys (1): > > sideband: highlight keywords in remote sideband output > > > > Documentation/config.txt | 12 +++ > > help.c | 1 + > > help.h | 1 + > > sideband.c | 125 ++++++++++++++++++++++++++-- > > t/t5409-colorize-remote-messages.sh | 87 +++++++++++++++++++ > > 5 files changed, 217 insertions(+), 9 deletions(-) > > create mode 100755 t/t5409-colorize-remote-messages.sh > > > > -- > > 2.18.0.597.ga71716f1ad-goog > > I'll squash in the following while queuing for > > <CAPig+cScb_7s4a_ZSVCsr+nBxAHGHZVMZOtnrOgbhZUi96-VFA@mail.gmail.com> > > Thanks for sticking to the topic. Thanks, 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-08-20 14:32 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.