{"thread":{"id":"44854","subject":"[PATCH 0/2] sanitizing error message contents","startedAt":"2017-01-11T14:01:49Z","lastAt":"2017-01-12T06:59:56Z","messageCount":6,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"309212","messageId":"20170111140138.5p647xuqpqrej63b@sigill.intra.peff.net","threadId":"44854","inReplyTo":null,"subject":"[PATCH 0/2] sanitizing error message contents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-11T14:01:39Z","receivedAt":"2017-01-11T14:01:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When adding a warning() call in 50d341374 (http: make redirects more\nobvious, 2016-12-06), somebody brought up that evil servers can redirect\nyou to something like:\n\n  https://evil.example.com/some/repo?unused=\\rwarning:+rainbows+and_unicorns_ahead\n\n(where \"\\r\" is a literal CR), and instead of seeing:\n\n  warning: redirecting to https://evil.example.com/...\n\nyou just get:\n\n  warning: rainbows and unicorns ahead\n\nor whatever innocuous looking line they prefer (probably just ANSI\n\"clear to beginning of line\" would be even more effective).\n\nSince it's hard to figure out which error messages could potentially\ncontain malicious contents, and since spewing control characters to the\nterminal is generally bad anyway, this series sanitizes at the lowest\nlevel.\n\nNote that this doesn't cover \"remote:\" lines coming over the sideband.\nThose are already covered for \"\\r\", as we have to parse it to handle\nprinting \"remote:\" consistently. But you can play tricks like putting:\n\n  printf '\\0331K\\033[0Efatal: this looks local\\n'\n\ninto a pre-receive hook. I'm not sure if we would want to do more\nsanitizing there. The goal of this series is not so much that a remote\ncan't send funny strings that may look local, but that they can't\nprevent local strings from being displayed. OTOH, I suspect clever use\nof ANSI codes (moving the cursor, clearing lines, etc) could get you\npretty far.\n\nI'd be hesitant to disallow control codes entirely, though, as I suspect\nsome servers do send colors over the sideband. So I punted on that here,\nbut I think this is at least an incremental improvement.\n\n  [1/2]: Revert \"vreportf: avoid intermediate buffer\"\n  [2/2]: vreport: sanitize ASCII control chars\n\n usage.c | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\n-Peff\n"},{"id":"309213","messageId":"20170111140203.obhd2t6yorszex73@sigill.intra.peff.net","threadId":"44854","inReplyTo":"20170111140138.5p647xuqpqrej63b@sigill.intra.peff.net","subject":"[PATCH 1/2] Revert \"vreportf: avoid intermediate buffer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-11T14:02:03Z","receivedAt":"2017-01-11T14:02:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit f4c3edc0b156362a92bf9de4f0ec794e90a757fc.\n\nThe purpose of that commit was to let us write errors of\narbitrary length to stderr by skipping the intermediate\nbuffer and sending our varargs straight to fprintf. That\nworks, but it comes with a downside: we do not get access to\nthe varargs before they are sent to stderr.\n\nOn balance, it's not a good tradeoff. Error messages larger\nthan our 4K buffer are quite uncommon, and we've lost the\nability to make any modifications to the output (e.g., to\nremove non-printable characters).\n\nThe only way to have both would be one of:\n\n  1. Write into a dynamic buffer. But this is a bad idea for\n     a low-level function that may be called when malloc()\n     has failed.\n\n  2. Do our own printf-format varargs parsing. This is too\n     complex to be worth the trouble.\n\nLet's just revert that change and go back to a fixed buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n usage.c | 15 +++------------\n 1 file changed, 3 insertions(+), 12 deletions(-)\n\ndiff --git a/usage.c b/usage.c\nindex 17f52c1b5..b1cbe6799 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -7,21 +7,13 @@\n #include \"cache.h\"\n \n static FILE *error_handle;\n-static int tweaked_error_buffering;\n \n void vreportf(const char *prefix, const char *err, va_list params)\n {\n+\tchar msg[4096];\n \tFILE *fh = error_handle ? error_handle : stderr;\n-\n-\tfflush(fh);\n-\tif (!tweaked_error_buffering) {\n-\t\tsetvbuf(fh, NULL, _IOLBF, 0);\n-\t\ttweaked_error_buffering = 1;\n-\t}\n-\n-\tfputs(prefix, fh);\n-\tvfprintf(fh, err, params);\n-\tfputc('\\n', fh);\n+\tvsnprintf(msg, sizeof(msg), err, params);\n+\tfprintf(fh, \"%s%s\\n\", prefix, msg);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n@@ -93,7 +85,6 @@ void set_die_is_recursing_routine(int (*routine)(void))\n void set_error_handle(FILE *fh)\n {\n \terror_handle = fh;\n-\ttweaked_error_buffering = 0;\n }\n \n void NORETURN usagef(const char *err, ...)\n-- \n2.11.0.627.gfa6151259\n\n"},{"id":"309214","messageId":"20170111140223.7irjqrtz77kaa5r7@sigill.intra.peff.net","threadId":"44854","inReplyTo":"20170111140138.5p647xuqpqrej63b@sigill.intra.peff.net","subject":"[PATCH 2/2] vreport: sanitize ASCII control chars","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-11T14:02:23Z","receivedAt":"2017-01-11T14:02:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our error() and die() calls may report messages with\narbitrary data (e.g., filenames or even data from a remote\nserver). Let's make it harder to cause confusion with\nmischievous filenames. E.g., try:\n\n  git rev-parse \"$(printf \"\\rfatal: this argument is too sneaky\")\" --\n\nor\n\n  git rev-parse \"$(printf \"\\x1b[5mblinky\\x1b[0m\")\" --\n\nLet's block all ASCII control characters, with the exception\nof TAB and LF. We use both in our own messages (and we are\nnecessarily sanitizing the complete output of snprintf here,\nas we do not have access to the individual varargs). And TAB\nand LF are unlikely to cause confusion (you could put\n\"\\nfatal: sneaky\\n\" in your filename, but it would at least\nnot _cover up_ the message leading to it, unlike \"\\r\").\n\nWe'll replace the characters with a \"?\", which is similar to\nhow \"ls\" behaves. It might be nice to do something less\nlossy, like converting them to \"\\x\" hex codes. But replacing\nwith a single character makes it easy to do in-place and\nwithout worrying about length limitations. This feature\nshould kick in rarely enough that the \"?\" marks are almost\nnever seen.\n\nWe'll leave high-bit characters as-is, as they are likely to\nbe UTF-8 (though there may be some Unicode mischief you\ncould cause, which may require further patches).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n usage.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/usage.c b/usage.c\nindex b1cbe6799..ad6d2910f 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -12,7 +12,13 @@ void vreportf(const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n \tFILE *fh = error_handle ? error_handle : stderr;\n+\tchar *p;\n+\n \tvsnprintf(msg, sizeof(msg), err, params);\n+\tfor (p = msg; *p; p++) {\n+\t\tif (iscntrl(*p) && *p != '\\t' && *p != '\\n')\n+\t\t\t*p = '?';\n+\t}\n \tfprintf(fh, \"%s%s\\n\", prefix, msg);\n }\n \n-- \n2.11.0.627.gfa6151259\n"},{"id":"309215","messageId":"20170111140758.yyfsc3r3spqpi6es@sigill.intra.peff.net","threadId":"44854","inReplyTo":"20170111140138.5p647xuqpqrej63b@sigill.intra.peff.net","subject":"[RFC PATCH 3/2] vreportf: add prefix to each line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-11T14:07:58Z","receivedAt":"2017-01-11T14:08:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 11, 2017 at 09:01:38AM -0500, Jeff King wrote:\n\n>   [1/2]: Revert \"vreportf: avoid intermediate buffer\"\n>   [2/2]: vreport: sanitize ASCII control chars\n\nWe've talked before about repeating the \"error:\" header for multi-line\nmessages. The reversion in patch 1 makes this easy to play with, so I\ndid. I kind of hate it. But here it is, for your viewing pleasure.\n\n-- >8 --\nSubject: vreportf: add prefix to each line\n\nSome error and warning messages are multi-line, but we put\nthe prefix only on the first line. This means that either\nthe subsequent lines don't have any indication that they're\nconnected to the original error, or the caller has to make\nmultiple calls to error() or warning(). And that's not even\npossible for die().\n\nInstead, let's put the prefix at the start of each line,\njust as advise() does.\n\nNote that this patch doesn't update the tests yet, so it\ncauses tons of failures. This is just to see what it might\nlook like.\n\nIt's actually kind of ugly.  For instance, a failing test in\nt3600 now produces:\n\n   error: the following files have staged content different from both the\n   error: file and the HEAD:\n   error:     bar.txt\n   error:     foo.txt\n   error: (use -f to force removal)\n\nwhich seems a bit aggressive.  It also screws up messages\nwhich indent with tabs (the prefix eats up some of the\ntabstop, making the indentation smaller). And the result is\na little harder if you need to cut-and-paste the file lines\n(if your terminal lets you triple-click to select a whole\nline, now you have this \"error:\" cruft on the front).\n\nIt may be possible to address some of that by using some\nother kind of continuation marker (instead of just repeating\nthe prefix), and expanding initial tabs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n usage.c | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/usage.c b/usage.c\nindex ad6d2910f..8a1f6ff4e 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -8,18 +8,30 @@\n \n static FILE *error_handle;\n \n+static void show_line(FILE *fh, const char *prefix, const char *line, int len)\n+{\n+\tfprintf(fh, \"%s%.*s\\n\", prefix, len, line);\n+}\n+\n void vreportf(const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n \tFILE *fh = error_handle ? error_handle : stderr;\n+\tconst char *base;\n \tchar *p;\n \n \tvsnprintf(msg, sizeof(msg), err, params);\n+\n+\tbase = msg;\n \tfor (p = msg; *p; p++) {\n-\t\tif (iscntrl(*p) && *p != '\\t' && *p != '\\n')\n+\t\tif (*p == '\\n') {\n+\t\t\tshow_line(fh, prefix, base, p - base);\n+\t\t\tbase = p + 1;\n+\t\t} else if (iscntrl(*p) && *p != '\\t')\n \t\t\t*p = '?';\n \t}\n-\tfprintf(fh, \"%s%s\\n\", prefix, msg);\n+\n+\tshow_line(fh, prefix, base, p - base);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n-- \n2.11.0.627.gfa6151259\n\n"},{"id":"309236","messageId":"xmqqpojtmeld.fsf@gitster.mtv.corp.google.com","threadId":"44854","inReplyTo":"20170111140758.yyfsc3r3spqpi6es@sigill.intra.peff.net","subject":"Re: [RFC PATCH 3/2] vreportf: add prefix to each line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-11T22:11:42Z","receivedAt":"2017-01-11T22:12:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jan 11, 2017 at 09:01:38AM -0500, Jeff King wrote:\n>\n>>   [1/2]: Revert \"vreportf: avoid intermediate buffer\"\n>>   [2/2]: vreport: sanitize ASCII control chars\n>\n> We've talked before about repeating the \"error:\" header for multi-line\n> messages. The reversion in patch 1 makes this easy to play with, so I\n> did. I kind of hate it. But here it is, for your viewing pleasure.\n\nThanks.\n\n>\n> -- >8 --\n> Subject: vreportf: add prefix to each line\n>\n> Some error and warning messages are multi-line, but we put\n> the prefix only on the first line. This means that either\n> the subsequent lines don't have any indication that they're\n> connected to the original error, or the caller has to make\n> multiple calls to error() or warning(). And that's not even\n> possible for die().\n>\n> Instead, let's put the prefix at the start of each line,\n> just as advise() does.\n>\n> Note that this patch doesn't update the tests yet, so it\n> causes tons of failures. This is just to see what it might\n> look like.\n>\n> It's actually kind of ugly.  For instance, a failing test in\n> t3600 now produces:\n>\n>    error: the following files have staged content different from both the\n>    error: file and the HEAD:\n>    error:     bar.txt\n>    error:     foo.txt\n>    error: (use -f to force removal)\n>\n> which seems a bit aggressive.  \n\nI agree that it is ugly, but one reason I was hoping to do this\nmyself (or have somebody else do it by procrastinating) was that I\nthought it may help i18n.  That is, for an original\n\n\terror(_(\"we found an error\"))\n\na .po file may translate the string to a multi-line string that has\nLFs in it and the output would look correct.  The translator already\ncan do so by indenting the second and subsequent lines by the same\ncolumn-width as \"error: \" (or its translation in their language, if\nwe are going to i18n these headers), but that (1) is extra work for\nthem, and (2) makes it impossible to have the same message appear in\ndifferent contexts (i.e. \"error:\" vs \"warning:\" that have different\ncolumn-widths).\n\n> It also screws up messages which indent with tabs (the prefix eats\n> up some of the tabstop, making the indentation smaller).\n\nThis is unavoidable and at the same time is a non-issue, isn't it?\nMessages that indent the second and subsequent lines with tabs are\ncompensating the lack of the multi-line support of vreportf(), which\nthis RFC patch fixes.  They may need to be adjusted to the new world\norder, but that is a good thing.  New multi-line messages no longer\nhave to worry about the prefix that is added only to the first line\nwhen continuing the message to multiple lines.\n\n> And the result is\n> a little harder if you need to cut-and-paste the file lines\n> (if your terminal lets you triple-click to select a whole\n> line, now you have this \"error:\" cruft on the front).\n\nThis might be an issue, but I personally do not care (as I know how\nto drive my \"screen\", namely, use 'c' while cutting) ;-).\n\n> It may be possible to address some of that by using some\n> other kind of continuation marker (instead of just repeating\n> the prefix), and expanding initial tabs.\n\nYes indeed.  The \"some other kind of continuation marker\" could just\nbe a run of spaces that fill the same column as the \"error: \" or\nother prefix given to the first line.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  usage.c | 16 ++++++++++++++--\n>  1 file changed, 14 insertions(+), 2 deletions(-)\n>\n> diff --git a/usage.c b/usage.c\n> index ad6d2910f..8a1f6ff4e 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -8,18 +8,30 @@\n>  \n>  static FILE *error_handle;\n>  \n> +static void show_line(FILE *fh, const char *prefix, const char *line, int len)\n> +{\n> +\tfprintf(fh, \"%s%.*s\\n\", prefix, len, line);\n> +}\n> +\n>  void vreportf(const char *prefix, const char *err, va_list params)\n>  {\n>  \tchar msg[4096];\n>  \tFILE *fh = error_handle ? error_handle : stderr;\n> +\tconst char *base;\n>  \tchar *p;\n>  \n>  \tvsnprintf(msg, sizeof(msg), err, params);\n> +\n> +\tbase = msg;\n>  \tfor (p = msg; *p; p++) {\n> -\t\tif (iscntrl(*p) && *p != '\\t' && *p != '\\n')\n> +\t\tif (*p == '\\n') {\n> +\t\t\tshow_line(fh, prefix, base, p - base);\n> +\t\t\tbase = p + 1;\n> +\t\t} else if (iscntrl(*p) && *p != '\\t')\n>  \t\t\t*p = '?';\n>  \t}\n> -\tfprintf(fh, \"%s%s\\n\", prefix, msg);\n> +\n> +\tshow_line(fh, prefix, base, p - base);\n>  }\n>  \n>  static NORETURN void usage_builtin(const char *err, va_list params)\n"},{"id":"309254","messageId":"20170112065947.qgukcozdi333yqgi@sigill.intra.peff.net","threadId":"44854","inReplyTo":"xmqqpojtmeld.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 3/2] vreportf: add prefix to each line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-12T06:59:47Z","receivedAt":"2017-01-12T06:59:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 11, 2017 at 02:11:42PM -0800, Junio C Hamano wrote:\n\n> > It's actually kind of ugly.  For instance, a failing test in\n> > t3600 now produces:\n> >\n> >    error: the following files have staged content different from both the\n> >    error: file and the HEAD:\n> >    error:     bar.txt\n> >    error:     foo.txt\n> >    error: (use -f to force removal)\n> >\n> > which seems a bit aggressive.  \n> \n> I agree that it is ugly, but one reason I was hoping to do this\n> myself (or have somebody else do it by procrastinating) was that I\n> thought it may help i18n.  That is, for an original\n> \n> \terror(_(\"we found an error\"))\n> \n> a .po file may translate the string to a multi-line string that has\n> LFs in it and the output would look correct.  The translator already\n> can do so by indenting the second and subsequent lines by the same\n> column-width as \"error: \" (or its translation in their language, if\n> we are going to i18n these headers), but that (1) is extra work for\n> them, and (2) makes it impossible to have the same message appear in\n> different contexts (i.e. \"error:\" vs \"warning:\" that have different\n> column-widths).\n\nYes, I agree that would be a functional benefit. I'm just hoping we can\ndo it in a way that is visually pleasing.\n\n> > It also screws up messages which indent with tabs (the prefix eats\n> > up some of the tabstop, making the indentation smaller).\n> \n> This is unavoidable and at the same time is a non-issue, isn't it?\n> Messages that indent the second and subsequent lines with tabs are\n> compensating the lack of the multi-line support of vreportf(), which\n> this RFC patch fixes.  They may need to be adjusted to the new world\n> order, but that is a good thing.  New multi-line messages no longer\n> have to worry about the prefix that is added only to the first line\n> when continuing the message to multiple lines.\n\nI'm not so sure it is just about compensating. Look at the message\nquoted above. The original looks like:\n\n  error: the following files have staged content different from both the\n  file and the HEAD:\n      bar.txt\n      foo.txt\n  (use -f to force removal)\n\nThe leading whitespace is visually separating the list of files, not\njust from the line with \"error:\", but from the other lines.\n\nThough I think if we replaced tabs with spaces in this instance, then\nthey would still be bumped relative to the rest of the text.\n\n> > It may be possible to address some of that by using some\n> > other kind of continuation marker (instead of just repeating\n> > the prefix), and expanding initial tabs.\n> \n> Yes indeed.  The \"some other kind of continuation marker\" could just\n> be a run of spaces that fill the same column as the \"error: \" or\n> other prefix given to the first line.\n\nI tried that, along with several other variants, but it gets\nconfusing/ugly when mixed with indentation that is significant to the\nline. For example, the t3600 message becomes:\n\n  error: the following files have staged content different from both the\n         file and the HEAD:\n             bar.txt\n             foo.txt\n         (use -f to force removal)\n\nWhich is arguably better than what we have now, but still looks pretty\nbad to me.\n\nI wonder if it would help for the marker to end in a non-whitespace\ncharacter. Like:\n\n  error: the following files have staged content different from both the\n       :  file and the HEAD:\n       :     bar.txt\n       :     foo.txt\n       : (use -f to force removal)\n\nor something. The \":\" is a bit sparse looking. Maybe there are better\noptions. That does ruin line-by-line selection for cut-and-paste,\nthough.\n\nSo I dunno. I am open to ideas, but I didn't find one that I really\nliked (this patch is actually a leftover from before Christmas, so I\ndon't even remember all the things I tried, just that I didn't like any\nof them ;) ).\n\n-Peff\n"}]}