{"thread":{"id":"30260","subject":"[PATCH] diff: avoid stack-buffer-read-overrun for very long name","startedAt":"2012-04-16T15:20:02Z","lastAt":"2012-04-27T15:07:55Z","messageCount":13,"participants":["Jim Meyering","Marcus Karlsson","Junio C Hamano","Bert Wesarg","Andreas Ericsson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"189414","messageId":"87ty0jbt5p.fsf@rho.meyering.net","threadId":"30260","inReplyTo":null,"subject":"[PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2012-04-16T15:20:02Z","receivedAt":"2012-04-16T15:20:02Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\nDue to the use of strncpy without explicit NUL termination,\nwe could end up passing names n1 or n2 that are not NUL-terminated\nto queue_diff, which requires NUL-terminated strings.\nEnsure that each is NUL terminated.\n\nSigned-off-by: Jim Meyering \n---\nAfter finding strncpy problems in other projects, I audited\ngit for the same and found only these two.\n\n diff-no-index.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 3a36144..5cd3ff5 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -109,6 +109,7 @@ static int queue_diff(struct diff_options *o,\n \t\t\t\tn1 = buffer1;\n \t\t\t\tstrncpy(buffer1 + len1, p1.items[i1++].string,\n \t\t\t\t\t\tPATH_MAX - len1);\n+\t\t\t\tbuffer1[PATH_MAX-1] = 0;\n \t\t\t}\n\n \t\t\tif (comp < 0)\n@@ -117,6 +118,7 @@ static int queue_diff(struct diff_options *o,\n \t\t\t\tn2 = buffer2;\n \t\t\t\tstrncpy(buffer2 + len2, p2.items[i2++].string,\n \t\t\t\t\t\tPATH_MAX - len2);\n+\t\t\t\tbuffer2[PATH_MAX-1] = 0;\n \t\t\t}\n\n \t\t\tret = queue_diff(o, n1, n2);\n--\n1.7.10.169.g146fe\n"},{"id":"189486","messageId":"20120416222713.GA2396@moj","threadId":"30260","inReplyTo":"87ty0jbt5p.fsf@rho.meyering.net","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Marcus Karlsson","fromEmail":"mk@acc.umu.se","sentAt":"2012-04-16T22:27:17Z","receivedAt":"2012-04-16T22:27:17Z","isPatch":true,"sender":{"key":"mk@acc.umu.se","avatar":null},"body":"On Mon, Apr 16, 2012 at 05:20:02PM +0200, Jim Meyering wrote:\n> \n> Due to the use of strncpy without explicit NUL termination,\n> we could end up passing names n1 or n2 that are not NUL-terminated\n> to queue_diff, which requires NUL-terminated strings.\n> Ensure that each is NUL terminated.\n> \n> Signed-off-by: Jim Meyering \n> ---\n> After finding strncpy problems in other projects, I audited\n> git for the same and found only these two.\n> \n> diff-no-index.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 3a36144..5cd3ff5 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -109,6 +109,7 @@ static int queue_diff(struct diff_options *o,\n> \t\t\t\tn1 = buffer1;\n> \t\t\t\tstrncpy(buffer1 + len1, p1.items[i1++].string,\n> \t\t\t\t\t\tPATH_MAX - len1);\n> +\t\t\t\tbuffer1[PATH_MAX-1] = 0;\n> \t\t\t}\n> \n> \t\t\tif (comp < 0)\n> @@ -117,6 +118,7 @@ static int queue_diff(struct diff_options *o,\n> \t\t\t\tn2 = buffer2;\n> \t\t\t\tstrncpy(buffer2 + len2, p2.items[i2++].string,\n> \t\t\t\t\t\tPATH_MAX - len2);\n> +\t\t\t\tbuffer2[PATH_MAX-1] = 0;\n> \t\t\t}\n> \n> \t\t\tret = queue_diff(o, n1, n2);\n> --\n> 1.7.10.169.g146fe\n\nAre there any guarantees that len1 and len2 does not exceed PATH_MAX?\nBecause if there aren't any then that function looks like it could need\neven more improvements.\n\n\tMarcus\n"},{"id":"189969","messageId":"87397t862o.fsf@rho.meyering.net","threadId":"30260","inReplyTo":"20120416222713.GA2396@moj","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2012-04-24T16:09:35Z","receivedAt":"2012-04-24T16:09:35Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Marcus Karlsson wrote:\n> On Mon, Apr 16, 2012 at 05:20:02PM +0200, Jim Meyering wrote:\n>>\n>> Due to the use of strncpy without explicit NUL termination,\n>> we could end up passing names n1 or n2 that are not NUL-terminated\n>> to queue_diff, which requires NUL-terminated strings.\n>> Ensure that each is NUL terminated.\n>>\n>> Signed-off-by: Jim Meyering \n>> ---\n>> After finding strncpy problems in other projects, I audited\n>> git for the same and found only these two.\n>>\n>> diff-no-index.c | 2 ++\n>> 1 file changed, 2 insertions(+)\n>>\n>> diff --git a/diff-no-index.c b/diff-no-index.c\n>> index 3a36144..5cd3ff5 100644\n>> --- a/diff-no-index.c\n>> +++ b/diff-no-index.c\n>> @@ -109,6 +109,7 @@ static int queue_diff(struct diff_options *o,\n>> \t\t\t\tn1 = buffer1;\n>> \t\t\t\tstrncpy(buffer1 + len1, p1.items[i1++].string,\n>> \t\t\t\t\t\tPATH_MAX - len1);\n>> +\t\t\t\tbuffer1[PATH_MAX-1] = 0;\n>> \t\t\t}\n>>\n>> \t\t\tif (comp < 0)\n>> @@ -117,6 +118,7 @@ static int queue_diff(struct diff_options *o,\n>> \t\t\t\tn2 = buffer2;\n>> \t\t\t\tstrncpy(buffer2 + len2, p2.items[i2++].string,\n>> \t\t\t\t\t\tPATH_MAX - len2);\n>> +\t\t\t\tbuffer2[PATH_MAX-1] = 0;\n>> \t\t\t}\n>>\n>> \t\t\tret = queue_diff(o, n1, n2);\n>> --\n>> 1.7.10.169.g146fe\n>\n> Are there any guarantees that len1 and len2 does not exceed PATH_MAX?\n> Because if there aren't any then that function looks like it could need\n> even more improvements.\n\nHi Marcus,\n\nYou're right to ask.\nI've just confirmed that there is such a guarantee. The question\nis whether either of queue_diff's name1 or name2 parameters may have\nstrlen larger than PATH_MAX, in which case, this code would misbehave,\npassing a negative length to strncpy:\n\n\t\t\t\tstrncpy(buffer1 + len1, p1.items[i1++].string,\n\t\t\t\t\t\tPATH_MAX - len1);\n\t\t\t\tbuffer1[PATH_MAX-1] = 0;\n\nqueue_diff is called from only two places:\n\n - from itself, recursively\n - from diff_no_index\n\nThe latter looks fine, since it's called with already-vetted names:\n\n\tif (queue_diff(&revs->diffopt, revs->diffopt.pathspec.raw[0],\n\t\t revs->diffopt.pathspec.raw[1]))\n\nThe recursive call is reachable only when both name1 and name2 are lstat'able.\nIf they're not (assuming they're non-trivial), this get_mode call fails:\n\n static int queue_diff(struct diff_options *o,\n const char *name1, const char *name2)\n {\n int mode1 = 0, mode2 = 0;\n\n if (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n return -1;\n\nThus, as long as a file with name longer than PATH_MAX is not\nlstat'able (what about hurd?), we're ok.\n\nHowever, a further improvement is possible if you care what happens\nwhen a very long newly-formed name is truncated by that use of strncpy.\nWhen that happens, in a pathological case in which the truncated\nname exists as well as the original, queue_diff could print totally\nbogus results.\n\nI.e., if dir/.../.../some-name is 5 bytes too long,\nand the truncated \"n1\" formed in queue_diff, \"dir/.../.../some\"\nrefers to a file that actually exists, queue_diff will mistakenly\nuse the truncated file name.\n"},{"id":"190075","messageId":"xmqq1unbd2m5.fsf@junio.mtv.corp.google.com","threadId":"30260","inReplyTo":"87397t862o.fsf@rho.meyering.net","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-25T19:37:38Z","receivedAt":"2012-04-25T19:37:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> Marcus Karlsson wrote:\n> ...\n>> Are there any guarantees that len1 and len2 does not exceed PATH_MAX?\n>> Because if there aren't any then that function looks like it could need\n>> even more improvements.\n>\n> Hi Marcus,\n>\n> You're right to ask.\n> I've just confirmed that there is such a guarantee.\n\nIn any case, I think this is an old part of the codebase that has not\nbeen updated to take advantage of newer API, partly because not many\npeople cared, and partly because there wasn't any serious bug there,\nthat can use some facelifting. Wouldn't it make more sense to use\nstrbuf here, perhaps like this (not even compile tested), on top of your\npatch?\n\n diff-no-index.c | 40 +++++++++++++++++-----------------------\n 1 file changed, 17 insertions(+), 23 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 5cd3ff5..b44473e 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)\n }\n \n static int queue_diff(struct diff_options *o,\n-\t\tconst char *name1, const char *name2)\n+\t\t const char *name1, const char *name2)\n {\n \tint mode1 = 0, mode2 = 0;\n \n@@ -63,10 +63,11 @@ static int queue_diff(struct diff_options *o,\n \t\treturn error(\"file/directory conflict: %s, %s\", name1, name2);\n \n \tif (S_ISDIR(mode1) || S_ISDIR(mode2)) {\n-\t\tchar buffer1[PATH_MAX], buffer2[PATH_MAX];\n+\t\tstruct strbuf buffer1 = STRBUF_INIT;\n+\t\tstruct strbuf buffer2 = STRBUF_INIT;\n \t\tstruct string_list p1 = STRING_LIST_INIT_DUP;\n \t\tstruct string_list p2 = STRING_LIST_INIT_DUP;\n-\t\tint len1 = 0, len2 = 0, i1, i2, ret = 0;\n+\t\tint i1, i2, ret = 0;\n \n \t\tif (name1 && read_directory(name1, &p1))\n \t\t\treturn -1;\n@@ -76,19 +77,15 @@ static int queue_diff(struct diff_options *o,\n \t\t}\n \n \t\tif (name1) {\n-\t\t\tlen1 = strlen(name1);\n-\t\t\tif (len1 > 0 && name1[len1 - 1] == '/')\n-\t\t\t\tlen1--;\n-\t\t\tmemcpy(buffer1, name1, len1);\n-\t\t\tbuffer1[len1++] = '/';\n+\t\t\tstrbuf_addstr(&buffer1, name1);\n+\t\t\tif (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')\n+\t\t\t\tstrbuf_addch(&buffer1, '/');\n \t\t}\n \n \t\tif (name2) {\n-\t\t\tlen2 = strlen(name2);\n-\t\t\tif (len2 > 0 && name2[len2 - 1] == '/')\n-\t\t\t\tlen2--;\n-\t\t\tmemcpy(buffer2, name2, len2);\n-\t\t\tbuffer2[len2++] = '/';\n+\t\t\tstrbuf_addstr(&buffer2, name2);\n+\t\t\tif (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')\n+\t\t\t\tstrbuf_addch(&buffer2, '/');\n \t\t}\n \n \t\tfor (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {\n@@ -100,31 +97,28 @@ static int queue_diff(struct diff_options *o,\n \t\t\telse if (i2 == p2.nr)\n \t\t\t\tcomp = -1;\n \t\t\telse\n-\t\t\t\tcomp = strcmp(p1.items[i1].string,\n-\t\t\t\t\tp2.items[i2].string);\n+\t\t\t\tcomp = strcmp(p1.items[i1].string, p2.items[i2].string);\n \n \t\t\tif (comp > 0)\n \t\t\t\tn1 = NULL;\n \t\t\telse {\n-\t\t\t\tn1 = buffer1;\n-\t\t\t\tstrncpy(buffer1 + len1, p1.items[i1++].string,\n-\t\t\t\t\t\tPATH_MAX - len1);\n-\t\t\t\tbuffer1[PATH_MAX-1] = 0;\n+\t\t\t\tstrbuf_addstr(&buffer1, p1.items[i1++].string);\n+\t\t\t\tn1 = buffer1.buf;\n \t\t\t}\n \n \t\t\tif (comp < 0)\n \t\t\t\tn2 = NULL;\n \t\t\telse {\n-\t\t\t\tn2 = buffer2;\n-\t\t\t\tstrncpy(buffer2 + len2, p2.items[i2++].string,\n-\t\t\t\t\t\tPATH_MAX - len2);\n-\t\t\t\tbuffer2[PATH_MAX-1] = 0;\n+\t\t\t\tstrbuf_addstr(&buffer2, p2.items[i2++].string);\n+\t\t\t\tn2 = buffer2.buf;\n \t\t\t}\n \n \t\t\tret = queue_diff(o, n1, n2);\n \t\t}\n \t\tstring_list_clear(&p1, 0);\n \t\tstring_list_clear(&p2, 0);\n+\t\tstrbuf_reset(&buffer1);\n+\t\tstrbuf_reset(&buffer2);\n \n \t\treturn ret;\n \t} else {\n"},{"id":"190118","messageId":"87d36uxzfw.fsf@rho.meyering.net","threadId":"30260","inReplyTo":"xmqq1unbd2m5.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2012-04-26T15:52:51Z","receivedAt":"2012-04-26T15:52:51Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering writes:\n>\n>> Marcus Karlsson wrote:\n>> ...\n>>> Are there any guarantees that len1 and len2 does not exceed PATH_MAX?\n>>> Because if there aren't any then that function looks like it could need\n>>> even more improvements.\n>>\n>> Hi Marcus,\n>>\n>> You're right to ask.\n>> I've just confirmed that there is such a guarantee.\n>\n> In any case, I think this is an old part of the codebase that has not\n> been updated to take advantage of newer API, partly because not many\n> people cared, and partly because there wasn't any serious bug there,\n> that can use some facelifting. Wouldn't it make more sense to use\n> strbuf here, perhaps like this (not even compile tested), on top of your\n> patch?\n>\n> diff-no-index.c | 40 +++++++++++++++++-----------------------\n> 1 file changed, 17 insertions(+), 23 deletions(-)\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 5cd3ff5..b44473e 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)\n> }\n>\n> static int queue_diff(struct diff_options *o,\n> -\t\tconst char *name1, const char *name2)\n> +\t\t const char *name1, const char *name2)\n> {\n> \tint mode1 = 0, mode2 = 0;\n>\n> @@ -63,10 +63,11 @@ static int queue_diff(struct diff_options *o,\n> \t\treturn error(\"file/directory conflict: %s, %s\", name1, name2);\n>\n> \tif (S_ISDIR(mode1) || S_ISDIR(mode2)) {\n> -\t\tchar buffer1[PATH_MAX], buffer2[PATH_MAX];\n> +\t\tstruct strbuf buffer1 = STRBUF_INIT;\n> +\t\tstruct strbuf buffer2 = STRBUF_INIT;\n> \t\tstruct string_list p1 = STRING_LIST_INIT_DUP;\n> \t\tstruct string_list p2 = STRING_LIST_INIT_DUP;\n> -\t\tint len1 = 0, len2 = 0, i1, i2, ret = 0;\n> +\t\tint i1, i2, ret = 0;\n>\n> \t\tif (name1 && read_directory(name1, &p1))\n> \t\t\treturn -1;\n> @@ -76,19 +77,15 @@ static int queue_diff(struct diff_options *o,\n> \t\t}\n>\n> \t\tif (name1) {\n> -\t\t\tlen1 = strlen(name1);\n> -\t\t\tif (len1 > 0 && name1[len1 - 1] == '/')\n> -\t\t\t\tlen1--;\n> -\t\t\tmemcpy(buffer1, name1, len1);\n> -\t\t\tbuffer1[len1++] = '/';\n> +\t\t\tstrbuf_addstr(&buffer1, name1);\n> +\t\t\tif (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')\n> +\t\t\t\tstrbuf_addch(&buffer1, '/');\n> \t\t}\n>\n> \t\tif (name2) {\n> -\t\t\tlen2 = strlen(name2);\n> -\t\t\tif (len2 > 0 && name2[len2 - 1] == '/')\n> -\t\t\t\tlen2--;\n> -\t\t\tmemcpy(buffer2, name2, len2);\n> -\t\t\tbuffer2[len2++] = '/';\n> +\t\t\tstrbuf_addstr(&buffer2, name2);\n> +\t\t\tif (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')\n> +\t\t\t\tstrbuf_addch(&buffer2, '/');\n\nHi Junio,\n\nThat looks much better.\nI verified that it compiles and passes the tests on x86_64/Fedora 17.\n\nWhat do you think about replacing those two append-if-needed two-liners:\n\n if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')\n strbuf_addch(&buffer2, '/');\n\nby something that readably encapsulates the idiom:\n\n strbuf_append_if_absent (&buffer2, '/');\n\n(though the name isn't particularly apt, because you might\ntake \"absent\" to mean \"not anywhere in the string,\" so maybe\n strbuf_append_if_not_already_at_end (ugly) or\n strbuf_append_uniq\n)\n\nThere are several other uses that would benefit from such a transformation:\nTo find the easy ones, I ran this:\n\n git grep -B1 \"strbuf_addch.*'\"|grep -A1 '!='\n\nI've manually marked/separated the ones that don't apply.\nNote how only 2 of the 6 candidates ensure that length is positive\nbefore using \".len - 1\":\n\n------------------------------------\nbuiltin/branch.c-\tif (!buf.len || buf.buf[buf.len-1] != '\\n')\nbuiltin/branch.c:\t\tstrbuf_addch(&buf, '\\n');\n--\nbuiltin/fmt-merge-msg.c-\t\tif (out->buf[out->len - 1] != '\\n')\nbuiltin/fmt-merge-msg.c:\t\t\tstrbuf_addch(out, '\\n');\n--\nbuiltin/log.c-\t\tif (filename.buf[filename.len - 1] != '/')\nbuiltin/log.c:\t\t\tstrbuf_addch(&filename, '/');\n--\nbuiltin/notes.c-\tif (buf.buf[buf.len - 1] != '\\n')\nbuiltin/notes.c:\t\tstrbuf_addch(&buf, '\\n'); /* Make sure msg ends with newline */\n--\nrefs.c-\t\tif (real_pattern.buf[real_pattern.len - 1] != '/')\nrefs.c:\t\t\tstrbuf_addch(&real_pattern, '/');\n--\nstrbuf.h-\tif (sb->len && sb->buf[sb->len - 1] != '\\n')\nstrbuf.h:\t\tstrbuf_addch(sb, '\\n');\n\n\n\n\n\n\n--\nNO wt-status.c-\t\t\tif (*line != '\\n' && *line != '\\t')\nNO wt-status.c:\t\t\t\tstrbuf_addch(&linebuf, ' ');\n--\nNO builtin/merge.c-\twhile ((commit = get_revision(&rev)) != NULL) {\nNO builtin/merge.c:\t\tstrbuf_addch(&out, '\\n');\n--\nNO builtin/shortlog.c-\tif (col != log->wrap)\nNO builtin/shortlog.c:\t\tstrbuf_addch(sb, '\\n');\n--\nNO dir.c-\tif (path->buf[original_len - 1] != '/')\nNO dir.c:\t\tstrbuf_addch(path, '/');\n--\nNO path.c-\tif (len && path[len-1] != '/')\nNO path.c:\t\tstrbuf_addch(&buf, '/');\n--\nNO pretty.c-\t\t\tif (p != commit->parents)\nNO pretty.c:\t\t\t\tstrbuf_addch(sb, ' ');\n--\nNO pretty.c-\t\t\tif (p != commit->parents)\nNO pretty.c:\t\t\t\tstrbuf_addch(sb, ' ');\n--\nNO pretty.c-\tif (pp->fmt != CMIT_FMT_ONELINE && !pp->subject) {\nNO pretty.c:\t\tstrbuf_addch(sb, '\\n');\n--\nNO pretty.c-\tif (pp->fmt != CMIT_FMT_ONELINE)\nNO pretty.c:\t\tstrbuf_addch(sb, '\\n');\n"},{"id":"190120","messageId":"xmqq62cma2uo.fsf@junio.mtv.corp.google.com","threadId":"30260","inReplyTo":"87d36uxzfw.fsf@rho.meyering.net","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-26T16:13:03Z","receivedAt":"2012-04-26T16:13:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> What do you think about replacing those two append-if-needed two-liners:\n>\n> if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')\n> strbuf_addch(&buffer2, '/');\n>\n> by something that readably encapsulates the idiom:\n>\n> strbuf_append_if_absent (&buffer2, '/');\n>\n> (though the name isn't particularly apt, because you might\n> take \"absent\" to mean \"not anywhere in the string,\" so maybe\n> strbuf_append_if_not_already_at_end (ugly) or\n> strbuf_append_uniq\n> )\n\nI am not good at names, but strbuf_terminate_with(&buffer2, '/')\nperhaps?\n\n> There are several other uses that would benefit from such a transformation:\n> To find the easy ones, I ran this:\n>\n> git grep -B1 \"strbuf_addch.*'\"|grep -A1 '!='\n>\n> I've manually marked/separated the ones that don't apply.\n>\n> Note how only 2 of the 6 candidates ensure that length is positive\n> before using \".len - 1\":\n\nYikes, that is embarrasing ;-)\n\n>\n> ------------------------------------\n> builtin/branch.c-\tif (!buf.len || buf.buf[buf.len-1] != '\\n')\n> builtin/branch.c:\t\tstrbuf_addch(&buf, '\\n');\n> --\n> builtin/fmt-merge-msg.c-\t\tif (out->buf[out->len - 1] != '\\n')\n> builtin/fmt-merge-msg.c:\t\t\tstrbuf_addch(out, '\\n');\n> --\n> builtin/log.c-\t\tif (filename.buf[filename.len - 1] != '/')\n> builtin/log.c:\t\t\tstrbuf_addch(&filename, '/');\n> --\n> builtin/notes.c-\tif (buf.buf[buf.len - 1] != '\\n')\n> builtin/notes.c:\t\tstrbuf_addch(&buf, '\\n'); /* Make sure msg ends with newline */\n> --\n> refs.c-\t\tif (real_pattern.buf[real_pattern.len - 1] != '/')\n> refs.c:\t\t\tstrbuf_addch(&real_pattern, '/');\n> --\n> strbuf.h-\tif (sb->len && sb->buf[sb->len - 1] != '\\n')\n> strbuf.h:\t\tstrbuf_addch(sb, '\\n');\n"},{"id":"190121","messageId":"CAKPyHN1mFGiZd7dDH-stUmr3H1JHwxcP1DkjCYNXZd6Bt-P7+w@mail.gmail.com","threadId":"30260","inReplyTo":"xmqq62cma2uo.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2012-04-26T16:21:33Z","receivedAt":"2012-04-26T16:21:33Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Thu, Apr 26, 2012 at 18:13, Junio C Hamano wrote:\n> Jim Meyering writes:\n>\n>> What do you think about replacing those two append-if-needed two-liners:\n>>\n>>     if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')\n>>             strbuf_addch(&buffer2, '/');\n>>\n>> by something that readably encapsulates the idiom:\n>>\n>>     strbuf_append_if_absent (&buffer2, '/');\n>>\n>> (though the name isn't particularly apt, because you might\n>> take \"absent\" to mean \"not anywhere in the string,\" so maybe\n>>   strbuf_append_if_not_already_at_end (ugly) or\n>>   strbuf_append_uniq\n>> )\n>\n> I am not good at names, but strbuf_terminate_with(&buffer2, '/')\n> perhaps?\n\nstrbuf_ensure_terminator(struct strbuf* buf, int term, int always)?\n\n>\n>> There are several other uses that would benefit from such a transformation:\n>> To find the easy ones, I ran this:\n>>\n>>   git grep -B1 \"strbuf_addch.*'\"|grep -A1 '!='\n>>\n>> I've manually marked/separated the ones that don't apply.\n>>\n>> ------------------------------------\n>> builtin/branch.c- if (!buf.len || buf.buf[buf.len-1] != '\\n')\n>> builtin/branch.c: strbuf_addch(&buf, '\\n');\n>> --\n>> strbuf.h- if (sb->len && sb->buf[sb->len - 1] != '\\n')\n>> strbuf.h: strbuf_addch(sb, '\\n');\n\nPlease note, that while they are checking the .len, they both behave\ndifferently if .len == 0 or not.\nThe first always append a '\\n', the latter only, if the string isn't empty.\n\nBert\n"},{"id":"190122","messageId":"874ns6xy30.fsf@rho.meyering.net","threadId":"30260","inReplyTo":"xmqq62cma2uo.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2012-04-26T16:22:11Z","receivedAt":"2012-04-26T16:22:11Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering writes:\n>\n>> What do you think about replacing those two append-if-needed two-liners:\n>>\n>> if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')\n>> strbuf_addch(&buffer2, '/');\n>>\n>> by something that readably encapsulates the idiom:\n>>\n>> strbuf_append_if_absent (&buffer2, '/');\n>>\n>> (though the name isn't particularly apt, because you might\n>> take \"absent\" to mean \"not anywhere in the string,\" so maybe\n>> strbuf_append_if_not_already_at_end (ugly) or\n>> strbuf_append_uniq\n>> )\n>\n> I am not good at names, but strbuf_terminate_with(&buffer2, '/')\n> perhaps?\n\nMaybe, but it still doesn't evoke the conditional nature\nof don't-append-if-already-there the operation. i.e., one\nmight wonder how it's different from \"strbuf_append\".\n\nHow about one of these?\n\n strbuf_ensure_suffix // but might make you think suffix==more than 1 byte\n strbuf_ensure_last_byte // maybe?\n strbuf_ensure_last_byte_is // rather long, but apt\n\n>> There are several other uses that would benefit from such a transformation:\n>> To find the easy ones, I ran this:\n>>\n>> git grep -B1 \"strbuf_addch.*'\"|grep -A1 '!='\n>>\n>> I've manually marked/separated the ones that don't apply.\n>>\n>> Note how only 2 of the 6 candidates ensure that length is positive\n>> before using \".len - 1\":\n>\n> Yikes, that is embarrasing ;-)\n\nKnowing you/git, each is because the buffer is known to be non-empty.\n"},{"id":"190123","messageId":"87y5piwjay.fsf@rho.meyering.net","threadId":"30260","inReplyTo":"CAKPyHN1mFGiZd7dDH-stUmr3H1JHwxcP1DkjCYNXZd6Bt-P7+w@mail.gmail.com","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2012-04-26T16:26:45Z","receivedAt":"2012-04-26T16:26:45Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Bert Wesarg wrote:\n> On Thu, Apr 26, 2012 at 18:13, Junio C Hamano wrote:\n>> Jim Meyering writes:\n>>\n>>> What do you think about replacing those two append-if-needed two-liners:\n>>>\n>>>     if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')\n>>>             strbuf_addch(&buffer2, '/');\n>>>\n>>> by something that readably encapsulates the idiom:\n>>>\n>>>     strbuf_append_if_absent (&buffer2, '/');\n>>>\n>>> (though the name isn't particularly apt, because you might\n>>> take \"absent\" to mean \"not anywhere in the string,\" so maybe\n>>>   strbuf_append_if_not_already_at_end (ugly) or\n>>>   strbuf_append_uniq\n>>> )\n>>\n>> I am not good at names, but strbuf_terminate_with(&buffer2, '/')\n>> perhaps?\n>\n> strbuf_ensure_terminator(struct strbuf* buf, int term, int always)?\n\nNice! So far, that's the name I prefer.\nBut why the third parameter?\n"},{"id":"190126","messageId":"CAKPyHN2VkBo6OKgbhTNSu-LFwabGkFFKAF595rJuXbhWwdte+g@mail.gmail.com","threadId":"30260","inReplyTo":"87y5piwjay.fsf@rho.meyering.net","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2012-04-26T16:53:07Z","receivedAt":"2012-04-26T16:53:07Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Thu, Apr 26, 2012 at 18:26, Jim Meyering wrote:\n> Bert Wesarg wrote:\n>> On Thu, Apr 26, 2012 at 18:13, Junio C Hamano wrote:\n>>> Jim Meyering writes:\n>> strbuf_ensure_terminator(struct strbuf* buf, int term, int always)?\n>\n> Nice!  So far, that's the name I prefer.\n> But why the third parameter?\n\nSee the second part of my reply:\n\n>>> ------------------------------------\n>>> builtin/branch.c- if (!buf.len || buf.buf[buf.len-1] != '\\n')\n>>> builtin/branch.c: strbuf_addch(&buf, '\\n');\n>>> --\n>>> strbuf.h- if (sb->len && sb->buf[sb->len - 1] != '\\n')\n>>> strbuf.h: strbuf_addch(sb, '\\n');\n>\n> Please note, that while they are checking the .len, they both behave\n> differently if .len == 0 or not.\n> The first always append a '\\n', the latter only, if the string isn't empty.\n"},{"id":"190127","messageId":"87ehrawgja.fsf@rho.meyering.net","threadId":"30260","inReplyTo":"CAKPyHN2VkBo6OKgbhTNSu-LFwabGkFFKAF595rJuXbhWwdte+g@mail.gmail.com","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2012-04-26T17:26:33Z","receivedAt":"2012-04-26T17:26:33Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Bert Wesarg wrote:\n> On Thu, Apr 26, 2012 at 18:26, Jim Meyering wrote:\n>> Bert Wesarg wrote:\n>>> On Thu, Apr 26, 2012 at 18:13, Junio C Hamano wrote:\n>>>> Jim Meyering writes:\n>>> strbuf_ensure_terminator(struct strbuf* buf, int term, int always)?\n>>\n>> Nice!  So far, that's the name I prefer.\n>> But why the third parameter?\n>\n> See the second part of my reply:\n\nOh. I missed that.\n\n>>>> ------------------------------------\n>>>> builtin/branch.c- if (!buf.len || buf.buf[buf.len-1] != '\\n')\n>>>> builtin/branch.c: strbuf_addch(&buf, '\\n');\n>>>> --\n>>>> strbuf.h- if (sb->len && sb->buf[sb->len - 1] != '\\n')\n>>>> strbuf.h: strbuf_addch(sb, '\\n');\n>>\n>> Please note, that while they are checking the .len, they both behave\n>> differently if .len == 0 or not.\n>> The first always append a '\\n', the latter only, if the string isn't empty.\n\nGlad you noticed the difference.\nHowever, is one exception worth complicating the interface?\n"},{"id":"190210","messageId":"4F9A972E.7050401@op5.se","threadId":"30260","inReplyTo":"xmqq62cma2uo.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2012-04-27T12:55:10Z","receivedAt":"2012-04-27T12:55:10Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 04/26/2012 06:13 PM, Junio C Hamano wrote:\n> Jim Meyering writes:\n> \n>> What do you think about replacing those two append-if-needed two-liners:\n>>\n>> if (buffer2.len&& buffer2.buf[buffer2.len - 1] != '/')\n>> strbuf_addch(&buffer2, '/');\n>>\n>> by something that readably encapsulates the idiom:\n>>\n>> strbuf_append_if_absent (&buffer2, '/');\n>>\n>> (though the name isn't particularly apt, because you might\n>> take \"absent\" to mean \"not anywhere in the string,\" so maybe\n>> strbuf_append_if_not_already_at_end (ugly) or\n>> strbuf_append_uniq\n>> )\n> \n> I am not good at names, but strbuf_terminate_with(&buffer2, '/')\n> perhaps?\n> \n\n\"terminate\" sounds pretty final though. How about strbuf_ensure_suffixch()?\nIt embeds the 'ch', marking it as a char argument and provides natural names\nfor\n strbuf_ensure_suffix(buf, char *str);\n strbuf_ensure_prefix(buf, char *str);\n strbuf_ensure_prefixch(buf, char c);\nif those are ever needed.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"190218","messageId":"xmqqmx5x9pro.fsf@junio.mtv.corp.google.com","threadId":"30260","inReplyTo":"4F9A972E.7050401@op5.se","subject":"Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-27T15:07:55Z","receivedAt":"2012-04-27T15:07:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Ericsson writes:\n\n>> I am not good at names, but strbuf_terminate_with(&buffer2, '/')\n>> perhaps?\n>> \n> \"terminate\" sounds pretty final though.\n\nYeah, but that function is adding a record terminator to the buffer, so...\n\n> How about strbuf_ensure_suffixch()?\n> It embeds the 'ch', marking it as a char argument...\n\nPerhaps.\n"}]}