{"thread":{"id":"37241","subject":"[PATCH] utf8.c: fix strbuf_utf8_replace copying the last NUL to dst string","startedAt":"2014-07-29T13:10:29Z","lastAt":"2014-08-10T07:05:21Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"246906","messageId":"1406639429-28748-1-git-send-email-pclouds@gmail.com","threadId":"37241","inReplyTo":null,"subject":"[PATCH] utf8.c: fix strbuf_utf8_replace copying the last NUL to dst string","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-29T13:10:29Z","receivedAt":"2014-07-29T13:10:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When utf8_width(&src) is called with *src == NULL (because the\nsource string ends with an ansi sequence), it returns 0 and steps\n'src' by one. This stepping makes strbuf_utf8_replace add NUL to the\ndestination string at the end of the loop. Check and break the loop\nearly.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n utf8.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/utf8.c b/utf8.c\nindex b30790d..cd090a1 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -381,6 +381,8 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,\n \t\t\tsrc += n;\n \t\t\tdst += n;\n \t\t}\n+\t\tif (src >= end)\n+\t\t\tbreak;\n \n \t\told = src;\n \t\tn = utf8_width((const char**)&src, NULL);\n-- \n2.1.0.rc0.66.gb9187ad\n"},{"id":"246941","messageId":"xmqqha202b2v.fsf@gitster.dls.corp.google.com","threadId":"37241","inReplyTo":"1406639429-28748-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] utf8.c: fix strbuf_utf8_replace copying the last NUL to dst string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-29T19:56:24Z","receivedAt":"2014-07-29T19:56:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> When utf8_width(&src) is called with *src == NULL (because the\n> source string ends with an ansi sequence),\n\nI am not sure what you mean by \"because\" here. Do you mean somebody\n(who?) decides to call the utf8_width() with NULL pointer stored in\n*src because of \"ansi sequence\"?\n\nWhat do you mean by \"ansi sequence\"? I'll assume that you mean\nthose terminal control that all use bytes with hi-bit clear.\n\nAt the very beginning of utf8_width(), *start can be cleared to\npoint at a NULL pointer by pick_one_utf8_char() if the pointer that\ncomes into utf8_width() originally points at an invalid UTF-8\nstring, but as far as I can see, ESC (or any bytes that would be\nused in those terminal control sequences like colors and cursor\ncontrol) will simply be returned as a single byte, without going\ninto error path that clears *start = NULL.\n\nPuzzled...\n\n> it returns 0 and steps\n> 'src' by one. \n\nHere \"it\" refers to utf8_width()? Who steps 'src' by one?\n\nAhh, did you mean *src == NUL, i.e. \"already at the end of the\nstring\"?\n\nI think utf8_width() called with an empty string should not move the\npointer past that end-of-string NUL in the first place. It makes me\nwonder if it would be a better fix to make it not to do that (and\nreturn 0), but if we declare it is the caller's fault, perhaps we\nmay want to add\n\n\tif (!**start)\n \tdie(\"BUG: empty string to utf8_width()???\");\n\nat the very beginning of utf8_width(), even before it calls\npick-one-utf8-char.\n\nStill puzzled...\n\n> This stepping makes strbuf_utf8_replace add NUL to the\n> destination string at the end of the loop. Check and break the loop\n> early.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> utf8.c | 2 ++\n> 1 file changed, 2 insertions(+)\n\nTests?\n\n> diff --git a/utf8.c b/utf8.c\n> index b30790d..cd090a1 100644\n> --- a/utf8.c\n> +++ b/utf8.c\n> @@ -381,6 +381,8 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,\n> \t\t\tsrc += n;\n> \t\t\tdst += n;\n> \t\t}\n> +\t\tif (src >= end)\n> +\t\t\tbreak;\n> \n> \t\told = src;\n> \t\tn = utf8_width((const char**)&src, NULL);\n"},{"id":"246959","messageId":"20140730102022.GA5653@lanh","threadId":"37241","inReplyTo":"xmqqha202b2v.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] utf8.c: fix strbuf_utf8_replace copying the last NUL to dst string","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-30T10:20:22Z","receivedAt":"2014-07-30T10:20:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 29, 2014 at 12:56:24PM -0700, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n> \n> > When utf8_width(&src) is called with *src == NULL (because the\n> > source string ends with an ansi sequence),\n> \n> I am not sure what you mean by \"because\" here. Do you mean somebody\n> (who?) decides to call the utf8_width() with NULL pointer stored in\n> *src because of \"ansi sequence\"?\n> \n> What do you mean by \"ansi sequence\"? I'll assume that you mean\n> those terminal control that all use bytes with hi-bit clear.\n\nOK let's try with an example, strbuf_utf8_replace(&sb, 0, 0, \"\") where\n\"sb\" contains \"\\033[m\". The expected result is exactly the same string\nwith length of 3. After this block\n\n\t\twhile ((n = display_mode_esc_sequence_len(src))) {\n\t\t\tmemcpy(dst, src, n);\n\t\t\tsrc += n;\n\t\t\tdst += n;\n\t\t}\n\nsrc will be right after 'm', *src is NUL (iow. src == end). After\n\n\t\tn = utf8_width((const char**)&src, NULL);\n\nn is zero but 'src' is advanced by another character, so\nsrc - old_src == 1. This NUL character is counted as part of the\nstring, so after the _setlen, we have the same string with length of\n_4_ (instead of 3).\n\n> At the very beginning of utf8_width(), *start can be cleared to\n> point at a NULL pointer by pick_one_utf8_char() if the pointer that\n> comes into utf8_width() originally points at an invalid UTF-8\n> string, but as far as I can see, ESC (or any bytes that would be\n> used in those terminal control sequences like colors and cursor\n> control) will simply be returned as a single byte, without going\n> into error path that clears *start = NULL.\n> \n> Puzzled...\n> \n> > it returns 0 and steps 'src' by one. \n> \n> Here \"it\" refers to utf8_width()? Who steps 'src' by one?\n\nutf8_width() steps 'src'.\n\n> \n> Ahh, did you mean *src == NUL, i.e. \"already at the end of the\n> string\"?\n\nYes.. I guess you have a better commit message prepared for me now :)\n\n> I think utf8_width() called with an empty string should not move the\n> pointer past that end-of-string NUL in the first place. It makes me\n> wonder if it would be a better fix to make it not to do that (and\n> return 0), but if we declare it is the caller's fault, perhaps we\n> may want to add\n> \n> \tif (!**start)\n> \tdie(\"BUG: empty string to utf8_width()???\");\n> \n> at the very beginning of utf8_width(), even before it calls\n> pick-one-utf8-char.\n> \n> Still puzzled...\n\nI don't know. I mean, in a UTF-8 byte sequence, NUL is a valid\ncharacter (part of the ASCII subset), so advancing '*start' by one\nmakes sense. Whether we have any call sites crazy enough to do that on\npurpose is a different matter.\n\n> > This stepping makes strbuf_utf8_replace add NUL to the\n> > destination string at the end of the loop. Check and break the loop\n> > early.\n> >\n> > Signed-off-by: Nguyễn Thái Ngọc Duy \n> > ---\n> > utf8.c | 2 ++\n> > 1 file changed, 2 insertions(+)\n> \n> Tests?\n\nUgh.. this was triggered by one of the alignment specifier in log\n--pretty. Probably easier to export strbuf_utf8_replace() as a\ntest-command and verify the output?\n\n--\nDuy\n"},{"id":"246981","messageId":"xmqqha1yzowz.fsf@gitster.dls.corp.google.com","threadId":"37241","inReplyTo":"20140730102022.GA5653@lanh","subject":"Re: [PATCH] utf8.c: fix strbuf_utf8_replace copying the last NUL to dst string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-30T18:23:24Z","receivedAt":"2014-07-30T18:23:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> > it returns 0 and steps 'src' by one. \n>> \n>> Here \"it\" refers to utf8_width()? Who steps 'src' by one?\n>\n> utf8_width() steps 'src'.\n>\n>> \n>> Ahh, did you mean *src == NUL, i.e. \"already at the end of the\n>> string\"?\n>\n> Yes.. I guess you have a better commit message prepared for me now :)\n\nHeh. At least you realized that the log message was uninformative ;-)\n\n>> I think utf8_width() called with an empty string should not move the\n>> pointer past that end-of-string NUL in the first place. It makes me\n>> wonder if it would be a better fix to make it not to do that (and\n>> return 0), but if we declare it is the caller's fault, perhaps we\n>> may want to add\n>> \n>> \tif (!**start)\n>> \tdie(\"BUG: empty string to utf8_width()???\");\n>> \n>> at the very beginning of utf8_width(), even before it calls\n>> pick-one-utf8-char.\n>> \n>> Still puzzled...\n>\n> I don't know. I mean, in a UTF-8 byte sequence, NUL is a valid\n> character (part of the ASCII subset), so advancing '*start' by one\n> makes sense.\n\nDubious. NUL may be valid but it is valid only in the sense that it\nwill mark the end of the string, i.e. the caller is expected to do:\n\n const char *string = ...;\n\n while (not reached the end of the string) {\n this_char = pick_one_char(&string);\n do something to this_char;\n }\n\nand there are two ways to structure such a loop:\n\n (1) Make pick-one-char signal the termination condition. i.e.\n\n\twhile ((this_char = pick_one(&string)) != EOF)\n\t\tdo something to this_char;\n\n That's a typical getchar()-style loop. It would better not\n advance string when it returns EOF.\n\n (2) Make it the responsibility of the caller not to call beyond the\n end. i.e.\n\n\twhile (string < end) {\n \tthis_char = pick_one_char(&string);\n\t\tdo something to this char;\n\t}\n\nand your patch takes the latter, which I think is in line with the\nway how existing callchain works. So the addition of \"BUG\" merely\nis to make sure that everybody follows that same convention.\n\nWe cannot declare that it is caller's responsibility to feed\nsensible input to the function only at one callsite and allow other\ncallsites feed garbage to the same function, after all, no?\n"},{"id":"247545","messageId":"1407654321-7509-1-git-send-email-pclouds@gmail.com","threadId":"37241","inReplyTo":"1406639429-28748-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2] utf8.c: fix strbuf_utf8_replace() consuming data beyond input string","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-08-10T07:05:21Z","receivedAt":"2014-08-10T07:05:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The main loop in strbuf_utf8_replace() could summed up as:\n\n while ('src' is still valid) {\n 1) advance 'src' to copy ANSI escape sequences\n 2) advance 'src' to copy/replace visible characters\n }\n\nThe problem is after #1, 'src' may have reached the end of the string\n(so 'src' points to NUL) and #2 will continue to copy that NUL as if\nit's a normal character. Because the output is stored in a strbuf,\nthis NULL accounted in the 'len' field as well. Check after #1 and\nbreak the loop if necessary.\n\nThe test does not look obvious, but the combination of %>>() should\nmake a call trace like this\n\n show_log()\n pretty_print_commit()\n format_commit_message()\n strbuf_expand()\n format_commit_item()\n format_and_pad_commit()\n strbuf_utf8_replace()\n\nwhere %C(auto)%d would insert a color reset escape sequence in the end\nof the string given to strbuf_utf8_replace() and show_log() uses\nfwrite() to send everything to stdout (including the incorrect NUL\ninserted by strbuf_utf8_replace)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n t/t4205-log-pretty-formats.sh | 7 +++++++\n utf8.c | 3 +++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 349c531..de0cc4a 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -431,6 +431,13 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'strbuf_utf8_replace() not producing NUL' '\n+\tgit log --color --pretty=\"tformat:%<(10,trunc)%s%>>(10,ltrunc)%C(auto)%d\" |\n+\t\ttest_decode_color |\n+\t\tnul_to_q >actual &&\n+\t! grep Q actual\n+'\n+\n # get new digests (with no abbreviations)\n head1=$(git rev-parse --verify HEAD~0) &&\n head2=$(git rev-parse --verify HEAD~1) &&\ndiff --git a/utf8.c b/utf8.c\nindex b30790d..401a6a5 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -382,6 +382,9 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,\n \t\t\tdst += n;\n \t\t}\n \n+\t\tif (src >= end)\n+\t\t\tbreak;\n+\n \t\told = src;\n \t\tn = utf8_width((const char**)&src, NULL);\n \t\tif (!src) \t/* broken utf-8, do nothing */\n-- \n2.1.0.rc0.78.gc0d8480\n"}]}