{"thread":{"id":"49060","subject":"[PATCH v7 0/1] sideband: highlight keywords in remote sideband output","startedAt":"2018-08-07T12:51:22Z","lastAt":"2018-08-20T14:32:12Z","messageCount":17,"participants":["Han-Wen Nienhuys","Junio C Hamano","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":7,"patchTotal":1},"messages":[{"id":"354728","messageId":"20180807125108.104293-1-hanwen@google.com","threadId":"49060","inReplyTo":null,"subject":"[PATCH v7 0/1] sideband: highlight keywords in remote sideband output","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-07T12:51:07Z","receivedAt":"2018-08-07T12:51:22Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"Fix nits; remove debug printf.\n\nHan-Wen Nienhuys (1):\n sideband: highlight keywords in remote sideband output\n\n Documentation/config.txt | 12 +++\n help.c | 1 +\n help.h | 1 +\n sideband.c | 125 ++++++++++++++++++++++++++--\n t/t5409-colorize-remote-messages.sh | 87 +++++++++++++++++++\n 5 files changed, 217 insertions(+), 9 deletions(-)\n create mode 100755 t/t5409-colorize-remote-messages.sh\n\n--\n2.18.0.597.ga71716f1ad-goog\n"},{"id":"354729","messageId":"20180807125108.104293-2-hanwen@google.com","threadId":"49060","inReplyTo":"20180807125108.104293-1-hanwen@google.com","subject":"[PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-07T12:51:08Z","receivedAt":"2018-08-07T12:51:28Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"The colorization is controlled with the config setting \"color.remote\".\n\nSupported keywords are \"error\", \"warning\", \"hint\" and \"success\". They\nare highlighted if they appear at the start of the line, which is\ncommon in error messages, eg.\n\n ERROR: commit is missing Change-Id\n\nThe Git push process itself prints lots of non-actionable messages\n(eg. bandwidth statistics, object counters for different phases of the\nprocess). This obscures actionable error messages that servers may\nsend back. Highlighting keywords in the sideband draws more attention\nto those messages.\n\nThe background for this change is that Gerrit does server-side\nprocessing to create or update code reviews, and actionable error\nmessages (eg. missing Change-Id) must be communicated back to the user\nduring the push. User research has shown that new users have trouble\nseeing these messages.\n\nThe highlighting is done on the client rather than server side, so\nservers don't have to grow capabilities to understand terminal escape\ncodes and terminal state. It also consistent with the current state\nwhere Git is control of the local display (eg. prefixing messages with\n\"remote: \").\n\nThe highlighting can be configured using color.remote.\nconfiguration settings. Since the keys are matched case insensitively,\nwe match the keywords case insensitively too.\n\nFinally, this solution is backwards compatible: many servers already\nprefix their messages with \"error\", and they will benefit from this\nchange without requiring a server update. By contrast, a server-side\nsolution would likely require plumbing the TERM variable through the\ngit protocol, so it would require changes to both server and client.\n\nHelped-by: Duy Nguyen \nSigned-off-by: Han-Wen Nienhuys \n---\n Documentation/config.txt | 12 +++\n help.c | 1 +\n help.h | 1 +\n sideband.c | 125 ++++++++++++++++++++++++++--\n t/t5409-colorize-remote-messages.sh | 87 +++++++++++++++++++\n 5 files changed, 217 insertions(+), 9 deletions(-)\n create mode 100755 t/t5409-colorize-remote-messages.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 63365dcf3d..33bc1a3def 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1263,6 +1263,18 @@ color.push::\n color.push.error::\n \tUse customized color for push errors.\n \n+color.remote::\n+\tIf set, keywords at the start of the line are highlighted. The\n+\tkeywords are \"error\", \"warning\", \"hint\" and \"success\", and are\n+\tmatched case-insensitively. Maybe set to `always`, `false` (or\n+\t`never`) or `auto` (or `true`). If unset, then the value of\n+\t`color.ui` is used (`auto` by default).\n+\n+color.remote.::\n+\tUse customized color for each remote keyword. `` may be\n+\t`hint`, `warning`, `success` or `error` which match the\n+\tcorresponding keyword.\n+\n color.showBranch::\n \tA boolean to enable/disable color in the output of\n \tlinkgit:git-show-branch[1]. May be set to `always`,\ndiff --git a/help.c b/help.c\nindex 3ebf0568db..b6cafcfc0a 100644\n--- a/help.c\n+++ b/help.c\n@@ -425,6 +425,7 @@ void list_config_help(int for_human)\n \t\t{ \"color.diff\", \"\", list_config_color_diff_slots },\n \t\t{ \"color.grep\", \"\", list_config_color_grep_slots },\n \t\t{ \"color.interactive\", \"\", list_config_color_interactive_slots },\n+\t\t{ \"color.remote\", \"\", list_config_color_sideband_slots },\n \t\t{ \"color.status\", \"\", list_config_color_status_slots },\n \t\t{ \"fsck\", \"\", list_config_fsck_msg_ids },\n \t\t{ \"receive.fsck\", \"\", list_config_fsck_msg_ids },\ndiff --git a/help.h b/help.h\nindex f8b15323a6..9eab6a3f89 100644\n--- a/help.h\n+++ b/help.h\n@@ -83,6 +83,7 @@ void list_config_color_diff_slots(struct string_list *list, const char *prefix);\n void list_config_color_grep_slots(struct string_list *list, const char *prefix);\n void list_config_color_interactive_slots(struct string_list *list, const char *prefix);\n void list_config_color_status_slots(struct string_list *list, const char *prefix);\n+void list_config_color_sideband_slots(struct string_list *list, const char *prefix);\n void list_config_fsck_msg_ids(struct string_list *list, const char *prefix);\n \n #endif /* HELP_H */\ndiff --git a/sideband.c b/sideband.c\nindex 325bf0e974..1c6bb0e25b 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -1,6 +1,109 @@\n #include \"cache.h\"\n+#include \"color.h\"\n+#include \"config.h\"\n #include \"pkt-line.h\"\n #include \"sideband.h\"\n+#include \"help.h\"\n+\n+struct keyword_entry {\n+\t/*\n+\t * We use keyword as config key so it should be a single alphanumeric word.\n+\t */\n+\tconst char *keyword;\n+\tchar color[COLOR_MAXLEN];\n+};\n+\n+static struct keyword_entry keywords[] = {\n+\t{ \"hint\",\tGIT_COLOR_YELLOW },\n+\t{ \"warning\",\tGIT_COLOR_BOLD_YELLOW },\n+\t{ \"success\",\tGIT_COLOR_BOLD_GREEN },\n+\t{ \"error\",\tGIT_COLOR_BOLD_RED },\n+};\n+\n+/* Returns a color setting (GIT_COLOR_NEVER, etc). */\n+static int use_sideband_colors(void)\n+{\n+\tstatic int use_sideband_colors_cached = -1;\n+\n+\tconst char *key = \"color.remote\";\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *value;\n+\tint i;\n+\n+\tif (use_sideband_colors_cached >= 0)\n+\t\treturn use_sideband_colors_cached;\n+\n+\tif (!git_config_get_string(key, &value)) {\n+\t\tuse_sideband_colors_cached = git_config_colorbool(key, value);\n+\t} else if (!git_config_get_string(\"color.ui\", &value)) {\n+\t\tuse_sideband_colors_cached = git_config_colorbool(\"color.ui\", value);\n+\t} else {\n+\t\tuse_sideband_colors_cached = GIT_COLOR_AUTO;\n+\t}\n+\n+\tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%s.%s\", key, keywords[i].keyword);\n+\t\tif (git_config_get_string(sb.buf, &value))\n+\t\t\tcontinue;\n+\t\tif (color_parse(value, keywords[i].color))\n+\t\t\tcontinue;\n+\t}\n+\tstrbuf_release(&sb);\n+\treturn use_sideband_colors_cached;\n+}\n+\n+void list_config_color_sideband_slots(struct string_list *list, const char *prefix)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(keywords); i++)\n+\t\tlist_config_item(list, prefix, keywords[i].keyword);\n+}\n+\n+/*\n+ * Optionally highlight one keyword in remote output if it appears at the start\n+ * of the line. This should be called for a single line only, which is\n+ * passed as the first N characters of the SRC array.\n+ */\n+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n+{\n+\tint i;\n+\n+\tif (!want_color_stderr(use_sideband_colors())) {\n+\t\tstrbuf_add(dest, src, n);\n+\t\treturn;\n+\t}\n+\n+\twhile (isspace(*src)) {\n+\t\tstrbuf_addch(dest, *src);\n+\t\tsrc++;\n+\t\tn--;\n+\t}\n+\n+\tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n+\t\tstruct keyword_entry *p = keywords + i;\n+\t\tint len = strlen(p->keyword);\n+\t\t/*\n+\t\t * Match case insensitively, so we colorize output from existing\n+\t\t * servers regardless of the case that they use for their\n+\t\t * messages. We only highlight the word precisely, so\n+\t\t * \"successful\" stays uncolored.\n+\t\t */\n+\t\tif (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n+\t\t\tstrbuf_addstr(dest, p->color);\n+\t\t\tstrbuf_add(dest, src, len);\n+\t\t\tstrbuf_addstr(dest, GIT_COLOR_RESET);\n+\t\t\tn -= len;\n+\t\t\tsrc += len;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tstrbuf_add(dest, src, n);\n+\n+}\n+\n \n /*\n * Receive multiplexed output stream over git native protocol.\n@@ -48,8 +151,10 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\tlen--;\n \t\tswitch (band) {\n \t\tcase 3:\n-\t\t\tstrbuf_addf(&outbuf, \"%s%s%s\", outbuf.len ? \"\\n\" : \"\",\n-\t\t\t\t DISPLAY_PREFIX, buf + 1);\n+\t\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n+\t\t\t\t DISPLAY_PREFIX);\n+\t\t\tmaybe_colorize_sideband(&outbuf, buf + 1, len);\n+\n \t\t\tretval = SIDEBAND_REMOTE_ERROR;\n \t\t\tbreak;\n \t\tcase 2:\n@@ -69,20 +174,22 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\t\t\tif (!outbuf.len)\n \t\t\t\t\tstrbuf_addstr(&outbuf, DISPLAY_PREFIX);\n \t\t\t\tif (linelen > 0) {\n-\t\t\t\t\tstrbuf_addf(&outbuf, \"%.*s%s%c\",\n-\t\t\t\t\t\t linelen, b, suffix, *brk);\n-\t\t\t\t} else {\n-\t\t\t\t\tstrbuf_addch(&outbuf, *brk);\n+\t\t\t\t\tmaybe_colorize_sideband(&outbuf, b, linelen);\n+\t\t\t\t\tstrbuf_addstr(&outbuf, suffix);\n \t\t\t\t}\n+\n+\t\t\t\tstrbuf_addch(&outbuf, *brk);\n \t\t\t\txwrite(2, outbuf.buf, outbuf.len);\n \t\t\t\tstrbuf_reset(&outbuf);\n \n \t\t\t\tb = brk + 1;\n \t\t\t}\n \n-\t\t\tif (*b)\n-\t\t\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ?\n-\t\t\t\t\t \"\" : DISPLAY_PREFIX, b);\n+\t\t\tif (*b) {\n+\t\t\t\tstrbuf_addstr(&outbuf, outbuf.len ?\n+\t\t\t\t\t \"\" : DISPLAY_PREFIX);\n+\t\t\t\tmaybe_colorize_sideband(&outbuf, b, strlen(b));\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 1:\n \t\t\twrite_or_die(out, buf + 1, len);\ndiff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\nnew file mode 100755\nindex 0000000000..7057277a5f\n--- /dev/null\n+++ b/t/t5409-colorize-remote-messages.sh\n@@ -0,0 +1,87 @@\n+#!/bin/sh\n+\n+test_description='remote messages are colorized on the client'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir .git/hooks &&\n+\twrite_script .git/hooks/update <<-\\EOF &&\n+\techo error: error\n+\techo ERROR: also highlighted\n+\techo hint: hint\n+\techo hinting: not highlighted\n+\techo success: success\n+\techo warning: warning\n+\techo prefixerror: error\n+\techo \" \" \"error: leading space\"\n+\texit 0\n+\tEOF\n+\techo 1 >file &&\n+\tgit add file &&\n+\tgit commit -m 1 &&\n+\tgit clone . child &&\n+\t(\n+\t\tcd child &&\n+\t\ttest_commit message2 file content2\n+\t)\n+'\n+\n+test_expect_success 'keywords' '\n+\tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/keywords 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error: error\" decoded &&\n+\tgrep \"hint:\" decoded &&\n+\tgrep \"success:\" decoded &&\n+\tgrep \"warning:\" decoded\n+'\n+\n+test_expect_success 'whole words at line start' '\n+\tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/whole-words 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"hint:\" decoded &&\n+\tgrep \"hinting: not highlighted\" decoded &&\n+\tgrep \"prefixerror: error\" decoded\n+'\n+\n+test_expect_success 'case-insensitive' '\n+\tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output &&\n+ cat output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error: error\" decoded &&\n+\tgrep \"ERROR: also highlighted\" decoded\n+'\n+\n+test_expect_success 'leading space' '\n+\tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space 2>output && cat output &&\n+\ttest_decode_color decoded &&\n+\tgrep \" error: leading space\" decoded\n+'\n+\n+test_expect_success 'no coloring for redirected output' '\n+\tgit --git-dir child/.git push -f origin HEAD:refs/heads/redirected-output 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error: error\" decoded\n+'\n+\n+test_expect_success 'push with customized color' '\n+\tgit --git-dir child/.git -c color.remote=always -c color.remote.error=blue push -f origin HEAD:refs/heads/customized-color 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error:\" decoded &&\n+\tgrep \"success:\" decoded\n+'\n+\n+\n+test_expect_success 'error in customized color' '\n+\tgit --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 &&\n+\ttest_decode_color decoded &&\n+\tgrep \"success:\" decoded\n+'\n+\n+test_expect_success 'fallback to color.ui' '\n+\tgit --git-dir child/.git -c color.ui=always push -f origin HEAD:refs/heads/fallback-color-ui 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error: error\" decoded\n+'\n+\n+test_done\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354771","messageId":"xmqqva8lki14.fsf@gitster-ct.c.googlers.com","threadId":"49060","inReplyTo":"20180807125108.104293-1-hanwen@google.com","subject":"Re: [PATCH v7 0/1] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-07T21:01:43Z","receivedAt":"2018-08-07T21:01:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> Fix nits; remove debug printf.\n>\n> Han-Wen Nienhuys (1):\n> sideband: highlight keywords in remote sideband output\n>\n> Documentation/config.txt | 12 +++\n> help.c | 1 +\n> help.h | 1 +\n> sideband.c | 125 ++++++++++++++++++++++++++--\n> t/t5409-colorize-remote-messages.sh | 87 +++++++++++++++++++\n> 5 files changed, 217 insertions(+), 9 deletions(-)\n> create mode 100755 t/t5409-colorize-remote-messages.sh\n>\n> --\n> 2.18.0.597.ga71716f1ad-goog\n\nI'll squash in the following while queuing for\n\n \n\nThanks for sticking to the topic. \n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 33bc1a3def..9a38dd2cbb 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1266,7 +1266,7 @@ color.push.error::\n color.remote::\n \tIf set, keywords at the start of the line are highlighted. The\n \tkeywords are \"error\", \"warning\", \"hint\" and \"success\", and are\n-\tmatched case-insensitively. Maybe set to `always`, `false` (or\n+\tmatched case-insensitively. May be set to `always`, `false` (or\n \t`never`) or `auto` (or `true`). If unset, then the value of\n \t`color.ui` is used (`auto` by default).\n \n"},{"id":"354800","messageId":"CAFQ2z_OiwVqxawfuVWurqCwpA8beC7Vn89tkhZkLkYcrApacEQ@mail.gmail.com","threadId":"49060","inReplyTo":"xmqqva8lki14.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v7 0/1] sideband: highlight keywords in remote sideband output","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-08T13:12:43Z","receivedAt":"2018-08-08T13:12:57Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Aug 7, 2018 at 11:01 PM Junio C Hamano wrote:\n>\n> Han-Wen Nienhuys writes:\n>\n> > Fix nits; remove debug printf.\n> >\n> > Han-Wen Nienhuys (1):\n> > sideband: highlight keywords in remote sideband output\n> >\n> > Documentation/config.txt | 12 +++\n> > help.c | 1 +\n> > help.h | 1 +\n> > sideband.c | 125 ++++++++++++++++++++++++++--\n> > t/t5409-colorize-remote-messages.sh | 87 +++++++++++++++++++\n> > 5 files changed, 217 insertions(+), 9 deletions(-)\n> > create mode 100755 t/t5409-colorize-remote-messages.sh\n> >\n> > --\n> > 2.18.0.597.ga71716f1ad-goog\n>\n> I'll squash in the following while queuing for\n>\n> \n>\n> Thanks for sticking to the topic.\n\nThanks, LGTM.\n\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"355930","messageId":"xmqqd0ugc07x.fsf@gitster-ct.c.googlers.com","threadId":"49060","inReplyTo":"20180807125108.104293-2-hanwen@google.com","subject":"Re: [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-17T18:33:06Z","receivedAt":"2018-08-17T18:33:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> +/*\n> + * Optionally highlight one keyword in remote output if it appears at the start\n> + * of the line. This should be called for a single line only, which is\n> + * passed as the first N characters of the SRC array.\n> + */\n> +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> +{\n> +\tint i;\n> +\n> +\tif (!want_color_stderr(use_sideband_colors())) {\n> +\t\tstrbuf_add(dest, src, n);\n> +\t\treturn;\n> +\t}\n> +\n> +\twhile (isspace(*src)) {\n> +\t\tstrbuf_addch(dest, *src);\n> +\t\tsrc++;\n> +\t\tn--;\n> +\t}\n\nThis loop can run out of bytes in src in search of non-space before\nn gets to zero or negative, and when that happens ...\n\n> +\tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n> +\t\tstruct keyword_entry *p = keywords + i;\n> +\t\tint len = strlen(p->keyword);\n> +\t\t/*\n> +\t\t * Match case insensitively, so we colorize output from existing\n> +\t\t * servers regardless of the case that they use for their\n> +\t\t * messages. We only highlight the word precisely, so\n> +\t\t * \"successful\" stays uncolored.\n> +\t\t */\n> +\t\tif (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n\n... these access src[] beyond the end of what the caller intended to\nshow us, and also ...\n\n> +\t\t\tstrbuf_addstr(dest, p->color);\n> +\t\t\tstrbuf_add(dest, src, len);\n> +\t\t\tstrbuf_addstr(dest, GIT_COLOR_RESET);\n> +\t\t\tn -= len;\n> +\t\t\tsrc += len;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tstrbuf_add(dest, src, n);\n\n... this will now try to add 0 or negative number of bytes.\n\n> +\n> +}\n> +\n\nPerhaps this will help (not really tested). The second hunk is an\nunrelated style clean-up.\n\n\n sideband.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 1c6bb0e25b..d99a559a44 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -75,11 +75,13 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \t\treturn;\n \t}\n \n-\twhile (isspace(*src)) {\n+\twhile (isspace(*src) && n) {\n \t\tstrbuf_addch(dest, *src);\n \t\tsrc++;\n \t\tn--;\n \t}\n+\tif (!n)\n+\t\treturn;\n \n \tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n \t\tstruct keyword_entry *p = keywords + i;\n@@ -101,7 +103,6 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \t}\n \n \tstrbuf_add(dest, src, n);\n-\n }\n \n \n"},{"id":"355931","messageId":"xmqq8t54bzo4.fsf_-_@gitster-ct.c.googlers.com","threadId":"49060","inReplyTo":"xmqqd0ugc07x.fsf@gitster-ct.c.googlers.com","subject":"Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-17T18:44:59Z","receivedAt":"2018-08-17T18:45:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This loop can run out of bytes in src in search of non-space before\n> n gets to zero or negative, and when that happens ...\n>\n>> +\tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n>> +\t\tstruct keyword_entry *p = keywords + i;\n>> +\t\tint len = strlen(p->keyword);\n>> +\t\t/*\n>> +\t\t * Match case insensitively, so we colorize output from existing\n>> +\t\t * servers regardless of the case that they use for their\n>> +\t\t * messages. We only highlight the word precisely, so\n>> +\t\t * \"successful\" stays uncolored.\n>> +\t\t */\n>> +\t\tif (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n>\n> ... these access src[] beyond the end of what the caller intended to\n> show us, and also ...\n\nActually, leaving when !n before this loop is insufficient. src[]\nmay have 2 bytes \"in\" remaining, and we may be trying to see if it\nbegins with \"info\", for example, and using strncasecmp() with len==4\nwould of course read beyond the end of src[].\n\n-- >8 --\nSubject: sideband: do not read beyond the end of input\n\nThe caller of maybe_colorize_sideband() gives a counted buffer\n, but the callee checked *src as if it were a NUL terminated\nbuffer. If src[] had all isspace() bytes in it, we would have made\nn negative, and then (1) called number of strncasecmp() to see if\nthe remaining bytes in src[] matched keywords, reading beyond the\nend of the array, and/or (2) called strbuf_add() with negative\ncount, most likely triggering the \"you want to use way too much\nmemory\" error due to unsigned integer overflow.\n\nSigned-off-by: Junio C Hamano \n---\n sideband.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 1c6bb0e25b..372039247f 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -75,7 +75,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \t\treturn;\n \t}\n \n-\twhile (isspace(*src)) {\n+\twhile (0 < n && isspace(*src)) {\n \t\tstrbuf_addch(dest, *src);\n \t\tsrc++;\n \t\tn--;\n@@ -84,6 +84,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n \t\tstruct keyword_entry *p = keywords + i;\n \t\tint len = strlen(p->keyword);\n+\n+\t\tif (n <= len)\n+\t\t\tcontinue;\n \t\t/*\n \t\t * Match case insensitively, so we colorize output from existing\n \t\t * servers regardless of the case that they use for their\n@@ -100,8 +103,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \t\t}\n \t}\n \n-\tstrbuf_add(dest, src, n);\n-\n+\tif (0 < n)\n+\t\tstrbuf_add(dest, src, n);\n }\n \n \n"},{"id":"355964","messageId":"20180818060911.GC241538@aiede.svl.corp.google.com","threadId":"49060","inReplyTo":"xmqq8t54bzo4.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-18T06:09:11Z","receivedAt":"2018-08-18T06:14:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(-cc: my @google.com email)\nHi,\n\nJunio C Hamano wrote:\n\n> Subject: sideband: do not read beyond the end of input\n>\n> The caller of maybe_colorize_sideband() gives a counted buffer\n> , but the callee checked *src as if it were a NUL terminated\n> buffer. If src[] had all isspace() bytes in it, we would have made\n> n negative, and then (1) called number of strncasecmp() to see if\n> the remaining bytes in src[] matched keywords, reading beyond the\n> end of the array, and/or (2) called strbuf_add() with negative\n> count, most likely triggering the \"you want to use way too much\n> memory\" error due to unsigned integer overflow.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> sideband.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n\nThis indeed avoids the \"you want to use way too much memory\" error\nwhen I apply it.\n\n> --- a/sideband.c\n> +++ b/sideband.c\n> @@ -75,7 +75,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n\nNot about this patch: should the 'n' parameter be a size_t instead of\nan int? It doesn't matter in practice (since the caller has an int,\nit can never be more than INT_MAX) but it might make the intent\nclearer.\n\nBased on inspecting the caller, using an int seems fine.\n\n> \t\treturn;\n> \t}\n> \n> -\twhile (isspace(*src)) {\n> +\twhile (0 < n && isspace(*src)) {\n\nYes, we need to check 'n && isspace(*src)' to avoid overflowing the\nbuffer if it consists entirely of spaces.\n\n> \t\tstrbuf_addch(dest, *src);\n> \t\tsrc++;\n> \t\tn--;\n> @@ -84,6 +84,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> \tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n> \t\tstruct keyword_entry *p = keywords + i;\n> \t\tint len = strlen(p->keyword);\n> +\n> +\t\tif (n <= len)\n> +\t\t\tcontinue;\n\nUsing <= instead of < since we look at the character after the word as\nwell. Good.\n\n> \t\t/*\n> \t\t * Match case insensitively, so we colorize output from existing\n> \t\t * servers regardless of the case that they use for their\n> \t\t * messages. We only highlight the word precisely, so\n> \t\t * \"successful\" stays uncolored.\n> \t\t */\n> \t\tif (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n\nNot about this patch: should this check \"&& src[len] == ':'\" instead,\nas discussed upthread?\n\n> @@ -100,8 +103,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> \t\t}\n> \t}\n> \n> -\tstrbuf_add(dest, src, n);\n> +\tif (0 < n)\n> +\t\tstrbuf_add(dest, src, n);\n\nThis check seems unnecessary. strbuf_add can cope fine with !n.\nShould we put\n\n\tassert(n >= 0);\n\nor even\n\n\tif (n < 0)\n\t\tBUG();\n\ninstead, since the earlier part of the fix guarantees that n >= 0?\n\nThanks for the careful work. With or without such a change,\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"355966","messageId":"20180818063534.GD241538@aiede.svl.corp.google.com","threadId":"49060","inReplyTo":"xmqq8t54bzo4.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-18T06:35:34Z","receivedAt":"2018-08-18T06:35:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Subject: sideband: do not read beyond the end of input\n>\n> The caller of maybe_colorize_sideband() gives a counted buffer\n> , but the callee checked *src as if it were a NUL terminated\n> buffer. If src[] had all isspace() bytes in it, we would have made\n> n negative, and then (1) called number of strncasecmp() to see if\n> the remaining bytes in src[] matched keywords, reading beyond the\n> end of the array, and/or (2) called strbuf_add() with negative\n> count, most likely triggering the \"you want to use way too much\n> memory\" error due to unsigned integer overflow.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> sideband.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n\nAnd here are some tests to squash in.\n\nA sideband line consisting entirely of spaces causes us to overflow\nour buffer and end up with a negative number of remaining characters,\nultimately resulting in the message\n\n\tfatal: you want to use way too much memory\n\nwhen parsing it in order to add color.\n\nWe also forget to limit a strncasecmp and isalnum looking for keywords\nto color in to the memory region passed in. Fortunately all callers\nput a delimiter character (\\0, \\r, or \\n) after the end of the region\nso this does not cause trouble in practice. Add a test for\nfutureproofing to protect the new bounds checking code in case that\never changes.\n\nSigned-off-by: Jonathan Nieder \n---\nThanks again,\nJonathan\n\n t/t5409-colorize-remote-messages.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\nindex eb1b8aa05df..f81b6813c03 100755\n--- a/t/t5409-colorize-remote-messages.sh\n+++ b/t/t5409-colorize-remote-messages.sh\n@@ -15,6 +15,8 @@ test_expect_success 'setup' '\n \techo warning: warning\n \techo prefixerror: error\n \techo \" \" \"error: leading space\"\n+\techo \" \"\n+\techo Err\n \texit 0\n \tEOF\n \techo 1 >file &&\n@@ -44,6 +46,12 @@ test_expect_success 'whole words at line start' '\n \tgrep \"prefixerror: error\" decoded\n '\n \n+test_expect_success 'short line' '\n+\tgit -C child -c color.remote=always push -f origin HEAD:short-line 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"remote: Err\" decoded\n+'\n+\n test_expect_success 'case-insensitive' '\n \tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output &&\n \tcat output &&\n@@ -58,6 +66,12 @@ test_expect_success 'leading space' '\n \tgrep \" error: leading space\" decoded\n '\n \n+test_expect_success 'spaces only' '\n+\tgit -C child -c color.remote=always push -f origin HEAD:only-space 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"remote: \" decoded\n+'\n+\n test_expect_success 'no coloring for redirected output' '\n \tgit --git-dir child/.git push -f origin HEAD:refs/heads/redirected-output 2>output &&\n \ttest_decode_color decoded &&\n-- \n2.18.0.1017.ga543ac7ca45\n\n"},{"id":"355977","messageId":"xmqqmutjagc7.fsf@gitster-ct.c.googlers.com","threadId":"49060","inReplyTo":"20180818060911.GC241538@aiede.svl.corp.google.com","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-18T14:40:08Z","receivedAt":"2018-08-18T14:40:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> --- a/sideband.c\n>> +++ b/sideband.c\n>> @@ -75,7 +75,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n>\n> Not about this patch: should the 'n' parameter be a size_t instead of\n> an int? It doesn't matter in practice (since the caller has an int,\n> it can never be more than INT_MAX) but it might make the intent\n> clearer.\n\nI tend to agree, but I think a separate \"clean-up\" patch to do so is\nmore appropriate than rolling it into this fix.\n\n>> \t\t/*\n>> \t\t * Match case insensitively, so we colorize output from existing\n>> \t\t * servers regardless of the case that they use for their\n>> \t\t * messages. We only highlight the word precisely, so\n>> \t\t * \"successful\" stays uncolored.\n>> \t\t */\n>> \t\tif (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n>\n> Not about this patch: should this check \"&& src[len] == ':'\" instead,\n> as discussed upthread?\n\nI originally was of an opinion that we should take only lowercase\nkeyword followed by a colon, primarily because that is what we\nproduce. Then \"the real world need\" told us that we are better off\ncatching the keyword case-insensitively. Recalling that lesson, I\nam not sure I would support \"let's limit to the colon, rejecting any\nother punctionation letter\".\n\nIn any case, we should make such a policy decision outside a patch\nlike this one that is about fixing a behaviour which all users would\nconsider as a bug regardless of the policy they support.\n\n>> @@ -100,8 +103,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n>> \t\t}\n>> \t}\n>> \n>> -\tstrbuf_add(dest, src, n);\n>> +\tif (0 < n)\n>> +\t\tstrbuf_add(dest, src, n);\n>\n> This check seems unnecessary. strbuf_add can cope fine with !n.\n\nI was primarily interested in catching negatives, and !n was a mere\noptimization, but you are correct to point out that negative n at\nthis point in the codeflow is a BUG().\n"},{"id":"355992","messageId":"xmqqin47acir.fsf@gitster-ct.c.googlers.com","threadId":"49060","inReplyTo":"xmqqmutjagc7.fsf@gitster-ct.c.googlers.com","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-18T16:02:36Z","receivedAt":"2018-08-18T16:02:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>>> -\tstrbuf_add(dest, src, n);\n>>> +\tif (0 < n)\n>>> +\t\tstrbuf_add(dest, src, n);\n>>\n>> This check seems unnecessary. strbuf_add can cope fine with !n.\n>\n> I was primarily interested in catching negatives, and !n was a mere\n> optimization, but you are correct to point out that negative n at\n> this point in the codeflow is a BUG().\n\nActually, let's just lose the conditional. strbuf_add() would catch\nand issue an error message when it notices that we fed negative\ncount anyway ;-).\n"},{"id":"355994","messageId":"xmqqefevacbr.fsf@gitster-ct.c.googlers.com","threadId":"49060","inReplyTo":"20180818063534.GD241538@aiede.svl.corp.google.com","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-18T16:06:48Z","receivedAt":"2018-08-18T16:06:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> And here are some tests to squash in.\n\nThanks, will do.\n"},{"id":"355998","messageId":"xmqqa7pjabvn.fsf@gitster-ct.c.googlers.com","threadId":"49060","inReplyTo":"xmqqin47acir.fsf@gitster-ct.c.googlers.com","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-18T16:16:28Z","receivedAt":"2018-08-18T16:16:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>>>> -\tstrbuf_add(dest, src, n);\n>>>> +\tif (0 < n)\n>>>> +\t\tstrbuf_add(dest, src, n);\n>>>\n>>> This check seems unnecessary. strbuf_add can cope fine with !n.\n>>\n>> I was primarily interested in catching negatives, and !n was a mere\n>> optimization, but you are correct to point out that negative n at\n>> this point in the codeflow is a BUG().\n>\n> Actually, let's just lose the conditional. strbuf_add() would catch\n> and issue an error message when it notices that we fed negative\n> count anyway ;-).\n\nSo I'll have this applied on top of the original topic to prevent a\nbuggy version from escaping the lab.\n\n-- >8 --\nSubject: [PATCH] sideband: do not read beyond the end of input\n\nThe caller of maybe_colorize_sideband() gives a counted buffer\n, but the callee checked src[] as if it were a NUL terminated\nbuffer. If src[] had all isspace() bytes in it, we would have made\nn negative, and then\n\n (1) made number of strncasecmp() calls to see if the remaining\n bytes in src[] matched keywords, reading beyond the end of the\n array (this actually happens even if n does not go negative),\n and/or\n\n (2) called strbuf_add() with negative count, most likely triggering\n the \"you want to use way too much memory\" error due to unsigned\n integer overflow.\n\nFix both issues by making sure we do not go beyond &src[n].\n\nIn the longer term we may want to accept size_t as parameter for\nclarity (even though we know that a sideband message we are painting\ntypically would fit on a line on a terminal and int is sufficient).\nWrite it down as a NEEDSWORK comment.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Junio C Hamano \n---\n sideband.c | 8 ++++++--\n t/t5409-colorize-remote-messages.sh | 14 ++++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 1c6bb0e25b..368647acf8 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -65,6 +65,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n * Optionally highlight one keyword in remote output if it appears at the start\n * of the line. This should be called for a single line only, which is\n * passed as the first N characters of the SRC array.\n+ *\n+ * NEEDSWORK: use \"size_t n\" instead for clarity.\n */\n static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n {\n@@ -75,7 +77,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \t\treturn;\n \t}\n \n-\twhile (isspace(*src)) {\n+\twhile (0 < n && isspace(*src)) {\n \t\tstrbuf_addch(dest, *src);\n \t\tsrc++;\n \t\tn--;\n@@ -84,6 +86,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n \t\tstruct keyword_entry *p = keywords + i;\n \t\tint len = strlen(p->keyword);\n+\n+\t\tif (n <= len)\n+\t\t\tcontinue;\n \t\t/*\n \t\t * Match case insensitively, so we colorize output from existing\n \t\t * servers regardless of the case that they use for their\n@@ -101,7 +106,6 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \t}\n \n \tstrbuf_add(dest, src, n);\n-\n }\n \n \ndiff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\nindex eb1b8aa05d..f81b6813c0 100755\n--- a/t/t5409-colorize-remote-messages.sh\n+++ b/t/t5409-colorize-remote-messages.sh\n@@ -15,6 +15,8 @@ test_expect_success 'setup' '\n \techo warning: warning\n \techo prefixerror: error\n \techo \" \" \"error: leading space\"\n+\techo \" \"\n+\techo Err\n \texit 0\n \tEOF\n \techo 1 >file &&\n@@ -44,6 +46,12 @@ test_expect_success 'whole words at line start' '\n \tgrep \"prefixerror: error\" decoded\n '\n \n+test_expect_success 'short line' '\n+\tgit -C child -c color.remote=always push -f origin HEAD:short-line 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"remote: Err\" decoded\n+'\n+\n test_expect_success 'case-insensitive' '\n \tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output &&\n \tcat output &&\n@@ -58,6 +66,12 @@ test_expect_success 'leading space' '\n \tgrep \" error: leading space\" decoded\n '\n \n+test_expect_success 'spaces only' '\n+\tgit -C child -c color.remote=always push -f origin HEAD:only-space 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"remote: \" decoded\n+'\n+\n test_expect_success 'no coloring for redirected output' '\n \tgit --git-dir child/.git push -f origin HEAD:refs/heads/redirected-output 2>output &&\n \ttest_decode_color decoded &&\n-- \n2.18.0-748-gfa03cdc39b\n\n"},{"id":"356018","messageId":"20180818232220.GA24533@sigill.intra.peff.net","threadId":"49060","inReplyTo":"xmqqa7pjabvn.fsf@gitster-ct.c.googlers.com","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-18T23:22:21Z","receivedAt":"2018-08-18T23:22:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 18, 2018 at 09:16:28AM -0700, Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: [PATCH] sideband: do not read beyond the end of input\n> \n> The caller of maybe_colorize_sideband() gives a counted buffer\n> , but the callee checked src[] as if it were a NUL terminated\n> buffer. If src[] had all isspace() bytes in it, we would have made\n> n negative, and then\n> \n> (1) made number of strncasecmp() calls to see if the remaining\n> bytes in src[] matched keywords, reading beyond the end of the\n> array (this actually happens even if n does not go negative),\n> and/or\n> \n> (2) called strbuf_add() with negative count, most likely triggering\n> the \"you want to use way too much memory\" error due to unsigned\n> integer overflow.\n> \n> Fix both issues by making sure we do not go beyond &src[n].\n\nThanks. I've been sporadically seeing \"fatal: you want to use way too\nmuch memory\" the last few days while running 'next', and finally managed\nto catch a reproducible case. This patch definitely fixes it.\n\n> In the longer term we may want to accept size_t as parameter for\n> clarity (even though we know that a sideband message we are painting\n> typically would fit on a line on a terminal and int is sufficient).\n> Write it down as a NEEDSWORK comment.\n\nThis \"typically\" made me nervous about what happens in the non-typical\ncase, but I think we can say something even stronger: the length comes\nfrom a pktline, so the maximum is less than 16 bits. I wondered if we\nmight ever call this on the accumulated string from multiple sidebands,\nbut it doesn't look like it.\n\n-Peff\n"},{"id":"356081","messageId":"CAFQ2z_NEBeiHWeC8BLKeBhTq_zA2iN2cTrENfPTdfykyM5uduQ@mail.gmail.com","threadId":"49060","inReplyTo":"xmqqa7pjabvn.fsf@gitster-ct.c.googlers.com","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-20T12:21:21Z","receivedAt":"2018-08-20T12:21:35Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Sat, Aug 18, 2018 at 6:16 PM Junio C Hamano wrote:\n> > Actually, let's just lose the conditional. strbuf_add() would catch\n> > and issue an error message when it notices that we fed negative\n> > count anyway ;-).\n>\n> So I'll have this applied on top of the original topic to prevent a\n> buggy version from escaping the lab.\n>\n> -- >8 --\n> Subject: [PATCH] sideband: do not read beyond the end of input\n>\n> The caller of maybe_colorize_sideband() gives a counted buffer\n> , but the callee checked src[] as if it were a NUL terminated\n> buffer. If src[] had all isspace() bytes in it, we would have made\n> n negative, and then\n>\n> (1) made number of strncasecmp() calls to see if the remaining\n> bytes in src[] matched keywords, reading beyond the end of the\n> array (this actually happens even if n does not go negative),\n> and/or\n>\n> (2) called strbuf_add() with negative count, most likely triggering\n> the \"you want to use way too much memory\" error due to unsigned\n> integer overflow.\n>\n> Fix both issues by making sure we do not go beyond &src[n].\n>\n> In the longer term we may want to accept size_t as parameter for\n> clarity (even though we know that a sideband message we are painting\n> typically would fit on a line on a terminal and int is sufficient).\n> Write it down as a NEEDSWORK comment.\n>\n> Helped-by: Jonathan Nieder \n> Signed-off-by: Junio C Hamano \n> ---\n> sideband.c | 8 ++++++--\n> t/t5409-colorize-remote-messages.sh | 14 ++++++++++++++\n> 2 files changed, 20 insertions(+), 2 deletions(-)\n>\n> diff --git a/sideband.c b/sideband.c\n> index 1c6bb0e25b..368647acf8 100644\n> --- a/sideband.c\n> +++ b/sideband.c\n> @@ -65,6 +65,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n> * Optionally highlight one keyword in remote output if it appears at the start\n> * of the line. This should be called for a single line only, which is\n> * passed as the first N characters of the SRC array.\n> + *\n> + * NEEDSWORK: use \"size_t n\" instead for clarity.\n> */\n> static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> {\n> @@ -75,7 +77,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> return;\n> }\n>\n> - while (isspace(*src)) {\n> + while (0 < n && isspace(*src)) {\n> strbuf_addch(dest, *src);\n> src++;\n> n--;\n> @@ -84,6 +86,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> for (i = 0; i < ARRAY_SIZE(keywords); i++) {\n> struct keyword_entry *p = keywords + i;\n> int len = strlen(p->keyword);\n> +\n> + if (n <= len)\n> + continue;\n\nI would suggest\n\n if (n < len) continue;\n..\nif (!strncasecmp(p->keyword, src, len) && (n == len || !isalnum(src[len]))) {\n\nso we colorize a single line that looks like \"warning\" as well\n\n\nOther than that, LGTM.\n\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"356082","messageId":"CAFQ2z_PiNAHduht6dHRAw8ybAUYkFcKXExypYa178u0gu2owRg@mail.gmail.com","threadId":"49060","inReplyTo":"CAFQ2z_NEBeiHWeC8BLKeBhTq_zA2iN2cTrENfPTdfykyM5uduQ@mail.gmail.com","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-20T12:21:37Z","receivedAt":"2018-08-20T12:21:51Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"and, thanks for cleaning up after me :)\n\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"356087","messageId":"xmqqa7phce4d.fsf@gitster-ct.c.googlers.com","threadId":"49060","inReplyTo":"20180818232220.GA24533@sigill.intra.peff.net","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-20T14:21:54Z","receivedAt":"2018-08-20T14:22:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> In the longer term we may want to accept size_t as parameter for\n>> clarity (even though we know that a sideband message we are painting\n>> typically would fit on a line on a terminal and int is sufficient).\n>> Write it down as a NEEDSWORK comment.\n>\n> This \"typically\" made me nervous about what happens in the non-typical\n> case, but I think we can say something even stronger: the length comes\n> from a pktline, so the maximum is less than 16 bits. I wondered if we\n> might ever call this on the accumulated string from multiple sidebands,\n> but it doesn't look like it.\n\nI think a sideband packet may be split on one but not the other\ncodepath to result in multiple calls, but I do not think we coalesce\nthem together, so I agree that int is sufficient.\n"},{"id":"356089","messageId":"xmqq6005cdnc.fsf@gitster-ct.c.googlers.com","threadId":"49060","inReplyTo":"CAFQ2z_NEBeiHWeC8BLKeBhTq_zA2iN2cTrENfPTdfykyM5uduQ@mail.gmail.com","subject":"Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-20T14:32:07Z","receivedAt":"2018-08-20T14:32:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n>> @@ -84,6 +86,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n>> for (i = 0; i < ARRAY_SIZE(keywords); i++) {\n>> struct keyword_entry *p = keywords + i;\n>> int len = strlen(p->keyword);\n>> +\n>> + if (n <= len)\n>> + continue;\n>\n> I would suggest\n>\n> if (n < len) continue;\n> ..\n> if (!strncasecmp(p->keyword, src, len) && (n == len || !isalnum(src[len]))) {\n>\n> so we colorize a single line that looks like \"warning\" as well\n\nThat's the kind of thing I would have mentioned in the initial\nreview of the feature, and I do not think it is a bad idea.\n\nI do think it is a bad idea to roll it into this patch, though.\n\n\n"}]}