{"thread":{"id":"44693","subject":"[PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf","startedAt":"2016-12-13T13:27:31Z","lastAt":"2016-12-13T19:10:54Z","messageCount":4,"participants":["Elia Pinto","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"307596","messageId":"20161213132717.42965-1-gitter.spiros@gmail.com","threadId":"44693","inReplyTo":null,"subject":"[PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-12-13T13:27:17Z","receivedAt":"2016-12-13T13:27:31Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call from snprintf/sprintf/strcpy to xsnprintf.  This patch supersedes the\nsnprintf with several calls that make use of the heap rather than fixed\nlength buffers (eg. PATH_MAX) that may be incorrect on some systems.\n\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\n builtin/commit.c | 32 +++++++++++++++-----------------\n 1 file changed, 15 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 0ed634b26..37228330c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \n \tif (use_editor) {\n-\t\tchar index[PATH_MAX];\n-\t\tconst char *env[2] = { NULL };\n-\t\tenv[0] =  index;\n-\t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n-\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env)) {\n+\t\tstruct argv_array env = ARGV_ARRAY_INIT;\n+\n+\t\targv_array_pushf(&env, \"GIT_INDEX_FILE=%s\", index_file);\n+\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n+\t\t\targv_array_clear(&env);\n \t\t\texit(1);\n \t\t}\n+\t\targv_array_clear(&env);\n \t}\n \n \tif (!no_verify &&\n@@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n static int run_rewrite_hook(const unsigned char *oldsha1,\n \t\t\t    const unsigned char *newsha1)\n {\n-\t/* oldsha1 SP newsha1 LF NUL */\n-\tstatic char buf[2*40 + 3];\n+\tchar *buf;\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n \tconst char *argv[3];\n \tint code;\n-\tsize_t n;\n \n \targv[0] = find_hook(\"post-rewrite\");\n \tif (!argv[0])\n@@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n \tcode = start_command(&proc);\n \tif (code)\n \t\treturn code;\n-\tn = snprintf(buf, sizeof(buf), \"%s %s\\n\",\n-\t\t     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n+\tbuf = xstrfmt(\"%s %s\\n\", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n \tsigchain_push(SIGPIPE, SIG_IGN);\n-\twrite_in_full(proc.in, buf, n);\n+\twrite_in_full(proc.in, buf, strlen(buf));\n \tclose(proc.in);\n+\tfree(buf);\n \tsigchain_pop(SIGPIPE);\n \treturn finish_command(&proc);\n }\n \n int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)\n {\n-\tconst char *hook_env[3] =  { NULL };\n-\tchar index[PATH_MAX];\n+\tstruct argv_array hook_env = ARGV_ARRAY_INIT;\n \tva_list args;\n \tint ret;\n \n-\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n-\thook_env[0] = index;\n+\targv_array_pushf(&hook_env, \"GIT_INDEX_FILE=%s\", index_file);\n \n \t/*\n \t * Let the hook know that no editor will be launched.\n \t */\n \tif (!editor_is_used)\n-\t\thook_env[1] = \"GIT_EDITOR=:\";\n+\t\targv_array_push(&hook_env, \"GIT_EDITOR=:\");\n \n \tva_start(args, name);\n-\tret = run_hook_ve(hook_env, name, args);\n+\tret = run_hook_ve(hook_env.argv,name, args);\n \tva_end(args);\n+\targv_array_clear(&hook_env);\n \n \treturn ret;\n }\n-- \n2.11.0.153.gacc9cba\n\n"},{"id":"307597","messageId":"20161213135514.z7eituxgxsvybwgz@sigill.intra.peff.net","threadId":"44693","inReplyTo":"20161213132717.42965-1-gitter.spiros@gmail.com","subject":"Re: [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-13T13:55:14Z","receivedAt":"2016-12-13T13:55:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2016 at 01:27:17PM +0000, Elia Pinto wrote:\n\n> With the commits f2f02675 and 5096d490 we have been converted in some files the call from snprintf/sprintf/strcpy to xsnprintf.  This patch supersedes the\n> snprintf with several calls that make use of the heap rather than fixed\n> length buffers (eg. PATH_MAX) that may be incorrect on some systems.\n\nI had trouble parsing this, because it leads with mention of commits\nthat _aren't_ doing the same thing we are doing here. I think the\nargument you want to make is basically:\n\n  1. snprintf is bad because it may silently truncate results if we're\n     wrong.\n\n  2. In cases where we use PATH_MAX, we'd want to handle larger paths\n     anyway, so we should switch to dynamic allocation.\n\n  3. In other cases where we know the input is bounded to a certain\n     length we could use xsnprintf(), which asserts that we don't\n     truncate. But by switching to dynamic allocation, we can avoid\n     dealing with magic numbers in the code.\n\nI'd actually consider splitting cases (2) and (3) into separate patches.\nEven though the end result is the same, the reasoning is quite\ndifferent.\n\nAs far as the patch itself goes, they all look like improvements to me.\nThe extra allocations shouldn't matter because these are all heavy\noperations already.\n\nA few minor nits:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 0ed634b26..37228330c 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\treturn 0;\n>  \n>  \tif (use_editor) {\n> -\t\tchar index[PATH_MAX];\n> -\t\tconst char *env[2] = { NULL };\n> -\t\tenv[0] =  index;\n> -\t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n> -\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env)) {\n> +\t\tstruct argv_array env = ARGV_ARRAY_INIT;\n> +\n> +\t\targv_array_pushf(&env, \"GIT_INDEX_FILE=%s\", index_file);\n> +\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {\n>  \t\t\tfprintf(stderr,\n>  \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n> +\t\t\targv_array_clear(&env);\n>  \t\t\texit(1);\n>  \t\t}\n> +\t\targv_array_clear(&env);\n\nI'd generally skip the clear() right before exiting, though I saw\nsomebody disagree with me recently on that.  I wonder if we should\ndecide as a project on whether it is worth doing or not.\n\n> @@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n>  static int run_rewrite_hook(const unsigned char *oldsha1,\n>  \t\t\t    const unsigned char *newsha1)\n>  {\n> -\t/* oldsha1 SP newsha1 LF NUL */\n> -\tstatic char buf[2*40 + 3];\n> +\tchar *buf;\n>  \tstruct child_process proc = CHILD_PROCESS_INIT;\n>  \tconst char *argv[3];\n>  \tint code;\n> -\tsize_t n;\n>  \n>  \targv[0] = find_hook(\"post-rewrite\");\n>  \tif (!argv[0])\n> @@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n>  \tcode = start_command(&proc);\n>  \tif (code)\n>  \t\treturn code;\n> -\tn = snprintf(buf, sizeof(buf), \"%s %s\\n\",\n> -\t\t     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n> +\tbuf = xstrfmt(\"%s %s\\n\", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n>  \tsigchain_push(SIGPIPE, SIG_IGN);\n> -\twrite_in_full(proc.in, buf, n);\n> +\twrite_in_full(proc.in, buf, strlen(buf));\n>  \tclose(proc.in);\n> +\tfree(buf);\n\nAny time you care about the length of the result, I'd generally use an\nactual strbuf instead of xstrfmt. The extra strlen isn't a big deal\nhere, but it somehow seems simpler to me. It probably doesn't matter\nmuch either way, though.\n\n>  int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)\n>  {\n> -\tconst char *hook_env[3] =  { NULL };\n> -\tchar index[PATH_MAX];\n> +\tstruct argv_array hook_env = ARGV_ARRAY_INIT;\n>  \tva_list args;\n>  \tint ret;\n>  \n> -\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n> -\thook_env[0] = index;\n> +\targv_array_pushf(&hook_env, \"GIT_INDEX_FILE=%s\", index_file);\n>  \n>  \t/*\n>  \t * Let the hook know that no editor will be launched.\n>  \t */\n>  \tif (!editor_is_used)\n> -\t\thook_env[1] = \"GIT_EDITOR=:\";\n> +\t\targv_array_push(&hook_env, \"GIT_EDITOR=:\");\n>  \n>  \tva_start(args, name);\n> -\tret = run_hook_ve(hook_env, name, args);\n> +\tret = run_hook_ve(hook_env.argv,name, args);\n>  \tva_end(args);\n> +\targv_array_clear(&hook_env);\n\nMissing whitespace between arguments in the run_hook_ve() call.\n\n-Peff\n"},{"id":"307656","messageId":"xmqqy3zj3b3a.fsf@gitster.mtv.corp.google.com","threadId":"44693","inReplyTo":"20161213135514.z7eituxgxsvybwgz@sigill.intra.peff.net","subject":"Re: [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-13T19:03:53Z","receivedAt":"2016-12-13T19:04:17Z","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>> +\t\targv_array_pushf(&env, \"GIT_INDEX_FILE=%s\", index_file);\n>> +\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {\n>>  \t\t\tfprintf(stderr,\n>>  \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n>> +\t\t\targv_array_clear(&env);\n>>  \t\t\texit(1);\n>>  \t\t}\n>> +\t\targv_array_clear(&env);\n>\n> I'd generally skip the clear() right before exiting, though I saw\n> somebody disagree with me recently on that.  I wonder if we should\n> decide as a project on whether it is worth doing or not.\n\nI'd say it is a responsibility of the person who turns exit(1) to\nreturn -1 to ensure the code does not leak.\n\n>> @@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n>>  static int run_rewrite_hook(const unsigned char *oldsha1,\n>>  \t\t\t    const unsigned char *newsha1)\n>>  {\n>> -\t/* oldsha1 SP newsha1 LF NUL */\n>> -\tstatic char buf[2*40 + 3];\n>> +\tchar *buf;\n>>  \tstruct child_process proc = CHILD_PROCESS_INIT;\n>>  \tconst char *argv[3];\n>>  \tint code;\n>> -\tsize_t n;\n>>  \n>>  \targv[0] = find_hook(\"post-rewrite\");\n>>  \tif (!argv[0])\n>> @@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n>>  \tcode = start_command(&proc);\n>>  \tif (code)\n>>  \t\treturn code;\n>> -\tn = snprintf(buf, sizeof(buf), \"%s %s\\n\",\n>> -\t\t     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n>> +\tbuf = xstrfmt(\"%s %s\\n\", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n>>  \tsigchain_push(SIGPIPE, SIG_IGN);\n>> -\twrite_in_full(proc.in, buf, n);\n>> +\twrite_in_full(proc.in, buf, strlen(buf));\n>>  \tclose(proc.in);\n>> +\tfree(buf);\n>\n> Any time you care about the length of the result, I'd generally use an\n> actual strbuf instead of xstrfmt. The extra strlen isn't a big deal\n> here, but it somehow seems simpler to me. It probably doesn't matter\n> much either way, though.\n\nYour justification for this extra allocation was that it is a\nheavy-weight operation.  While I agree that the runtime cost of\nallocation and deallocation does not matter, I would be a bit\nworried about extra cognitive burden to programmers.  They did not\nhave to worry about leaking because they are writing a fixed length\nstring.  Now they do, whether they use xstrfmt() or struct strbuf.\nWhen they need to update what they write, they do have to remember\nto adjust the size of the \"fixed string\", and the original is not\nfree from the \"programmers' cognitive cost\" point of view, of\ncourse.  Probably use of strbuf/xstrfmt is an overall win.\n\nAnd of course, I think strbuf is more appropriate if you have to do\nstrlen().\n"},{"id":"307659","messageId":"20161213191046.vbl7st3qf5xxjhab@sigill.intra.peff.net","threadId":"44693","inReplyTo":"xmqqy3zj3b3a.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-13T19:10:46Z","receivedAt":"2016-12-13T19:10:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2016 at 11:03:53AM -0800, Junio C Hamano wrote:\n\n> >> @@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n> >>  static int run_rewrite_hook(const unsigned char *oldsha1,\n> >>  \t\t\t    const unsigned char *newsha1)\n> >>  {\n> >> -\t/* oldsha1 SP newsha1 LF NUL */\n> >> -\tstatic char buf[2*40 + 3];\n> >> +\tchar *buf;\n> >>  \tstruct child_process proc = CHILD_PROCESS_INIT;\n> >>  \tconst char *argv[3];\n> >>  \tint code;\n> >> -\tsize_t n;\n> >>  \n> >>  \targv[0] = find_hook(\"post-rewrite\");\n> >>  \tif (!argv[0])\n> >> @@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n> >>  \tcode = start_command(&proc);\n> >>  \tif (code)\n> >>  \t\treturn code;\n> >> -\tn = snprintf(buf, sizeof(buf), \"%s %s\\n\",\n> >> -\t\t     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n> >> +\tbuf = xstrfmt(\"%s %s\\n\", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n> >>  \tsigchain_push(SIGPIPE, SIG_IGN);\n> >> -\twrite_in_full(proc.in, buf, n);\n> >> +\twrite_in_full(proc.in, buf, strlen(buf));\n> >>  \tclose(proc.in);\n> >> +\tfree(buf);\n> >\n> > Any time you care about the length of the result, I'd generally use an\n> > actual strbuf instead of xstrfmt. The extra strlen isn't a big deal\n> > here, but it somehow seems simpler to me. It probably doesn't matter\n> > much either way, though.\n> \n> Your justification for this extra allocation was that it is a\n> heavy-weight operation.  While I agree that the runtime cost of\n> allocation and deallocation does not matter, I would be a bit\n> worried about extra cognitive burden to programmers.  They did not\n> have to worry about leaking because they are writing a fixed length\n> string.  Now they do, whether they use xstrfmt() or struct strbuf.\n> When they need to update what they write, they do have to remember\n> to adjust the size of the \"fixed string\", and the original is not\n> free from the \"programmers' cognitive cost\" point of view, of\n> course.  Probably use of strbuf/xstrfmt is an overall win.\n\nSo I think you are agreeing, but I have a minor nit to pick. :)\n\nThe fact that the extra allocation will not hurt performance is\n_necessary_, but not _sufficient_. So it's not a justification in\nitself, only something we have to check before proceeding.\n\nThe only justification here is that magic numbers like \"2*40 + 3\" are\nconfusing and a potential maintenance burden. And that's why I suggested\nsplitting this one out from the other two (whose justification is\n\"PATH_MAX is sometimes too small\").\n\nI agree with you that it's a tradeoff between \"magic numbers\" versus\n\"having to free resources\". In my opinion it's a net improvement, but I\nthink it would also be reasonable to switch to xsnprintf() here. Then\nthe programmer has an automatic check that the buffer size is\nsufficient.\n\n-Peff\n"}]}