{"thread":{"id":"38804","subject":"[PATCH] git.c: two memory leak fixes","startedAt":"2015-03-14T18:43:33Z","lastAt":"2015-03-15T11:53:14Z","messageCount":3,"participants":["Alexander Kuleshov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"257684","messageId":"1426358613-30180-1-git-send-email-kuleshovmail@gmail.com","threadId":"38804","inReplyTo":null,"subject":"[PATCH] git.c: two memory leak fixes","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-03-14T18:43:33Z","receivedAt":"2015-03-14T18:43:33Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"This patch provides fixes for two minor memory leaks in the\nhandle_alias function:\n\n1. We allocate memory for alias_argv with the xmalloc function call,\nafter run_command_v_opt function will be executed we no need in this\nvariable anymore, so let's free it.\n\n2. Memory allocated for alias_string variable in the alias_lookup function,\nneed to free it.\n\nSigned-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>\n---\n git.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex 086fac1..501e5bd 100644\n--- a/git.c\n+++ b/git.c\n@@ -252,6 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\talias_argv[argc] = NULL;\n \n \t\t\tret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n+\t\t\tfree(alias_argv);\n \t\t\tif (ret >= 0)   /* normal exit */\n \t\t\t\texit(ret);\n \n@@ -259,6 +260,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\t    alias_command, alias_string + 1);\n \t\t}\n \t\tcount = split_cmdline(alias_string, &new_argv);\n+\t\tfree(alias_string);\n \t\tif (count < 0)\n \t\t\tdie(\"Bad alias.%s string: %s\", alias_command,\n \t\t\t    split_cmdline_strerror(count));\n-- \n2.3.3.469.g69a3822.dirty\n"},{"id":"257703","messageId":"xmqq3857vw9b.fsf@gitster.dls.corp.google.com","threadId":"38804","inReplyTo":"1426358613-30180-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH] git.c: two memory leak fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-15T02:18:24Z","receivedAt":"2015-03-15T02:18:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov <kuleshovmail@gmail.com> writes:\n\n> This patch provides fixes for two minor memory leaks in the\n> handle_alias function:\n>\n> 1. We allocate memory for alias_argv with the xmalloc function call,\n> after run_command_v_opt function will be executed we no need in this\n> variable anymore, so let's free it.\n\nThis is technically correct, but do we really care about it?\n\nWe have finished running the command and all that is left is either\nto exit(ret) or to die(); either will let the operating system clear\nthe memory together with the entire process for us.\n\nThe \"normal exit\" codepath still holds a live \"alias_string\" when it\ncalls exist(ret), but you do not free it even after this patch,\nwhich is an inconsistent stance to take.  I think it is OK not to\nbother freeing alias_string just before exit(), and I would apply\nthe same reasoning to alias_argv[], too.\n\n> 2. Memory allocated for alias_string variable in the alias_lookup function,\n> need to free it.\n\nI think it is wrong to free alias_string after passing it to\nsplit_cmdline() and while you still need to use the new_argv.\n\nReading the split_cmdline() implementation again, the new argv which\nis an array of pointers is newly allocated, but I think the pointers\nin the array point into the cmdline parameter.  From the caller's\npoint of view, new_argv[0] points at the beginning of alias_string,\nnew_argv[1] points at the beginning of the second \"word\" of\nalias_string, etc., all of which will become invalid if you free\nalias_string, no?\n\n> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>\n> ---\n>  git.c | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git a/git.c b/git.c\n> index 086fac1..501e5bd 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -252,6 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)\n>  \t\t\talias_argv[argc] = NULL;\n>  \n>  \t\t\tret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n> +\t\t\tfree(alias_argv);\n>  \t\t\tif (ret >= 0)   /* normal exit */\n>  \t\t\t\texit(ret);\n>  \n> @@ -259,6 +260,7 @@ static int handle_alias(int *argcp, const char ***argv)\n>  \t\t\t    alias_command, alias_string + 1);\n>  \t\t}\n>  \t\tcount = split_cmdline(alias_string, &new_argv);\n> +\t\tfree(alias_string);\n>  \t\tif (count < 0)\n>  \t\t\tdie(\"Bad alias.%s string: %s\", alias_command,\n>  \t\t\t    split_cmdline_strerror(count));\n"},{"id":"257717","messageId":"CANCZXo4rJWJohDpeb+nt3t=WBjmq14473+ynuPhvOuPd9+Vs_w@mail.gmail.com","threadId":"38804","inReplyTo":"xmqq3857vw9b.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git.c: two memory leak fixes","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-03-15T11:53:14Z","receivedAt":"2015-03-15T11:53:14Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Hello Junio,\n\n>>This is technically correct, but do we really care about it?\n\nYes, as i saw handle_alias called once, and we no need to care about it.\n\n>>I think it is wrong to free alias_string after passing it to\n>>split_cmdline() and while you still need to use the new_argv.\n\nYes, right. sorry for the noise.\n"}]}