{"thread":{"id":"27419","subject":"[BUG] realloc failed","startedAt":"2011-05-21T01:01:32Z","lastAt":"2011-05-27T13:59:15Z","messageCount":6,"participants":["Kazuki Tsujimoto","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"168341","messageId":"20110521100126.E3CD.BA9123DE@callcc.net","threadId":"27419","inReplyTo":null,"subject":"[BUG] realloc failed","fromName":"Kazuki Tsujimoto","fromEmail":"kazuki@callcc.net","sentAt":"2011-05-21T01:01:32Z","receivedAt":"2011-05-21T01:01:32Z","isPatch":false,"sender":{"key":"kazuki@callcc.net","avatar":"https://gravatar.com/avatar/4d88894b5eb851a4542b02acb4775858bcc8fb2125607c65b7c6fba0a6286465?d=mp&s=160"},"body":"The following command causes \"fatal: Out of memory, realloc failed\" error.\n\n$ ./git --version\ngit version 1.7.5.GIT\n\n$ cat ~/.gitconfig\n[alias]\n a = -c n=v status\n\n$ MALLOC_CHECK_=0 ./git a\nfatal: Out of memory, realloc failed\n\n$ gdb --args ./git a\n(gdb) run\n*** glibc detected *** /tmp/git/git: realloc(): invalid pointer: 0x00000000007cd328 ***\n...snip...\n(gdb) bt\n#0 0x00007ffff72c8a75 in raise () from /lib/libc.so.6\n#1 0x00007ffff72cc5c0 in abort () from /lib/libc.so.6\n#2 0x00007ffff73024fb in ?? () from /lib/libc.so.6\n#3 0x00007ffff730c5b6 in ?? () from /lib/libc.so.6\n#4 0x00007ffff73132e2 in realloc () from /lib/libc.so.6\n#5 0x0000000000510bfb in xrealloc (ptr=0x7cd328, size=16) at wrapper.c:82\n#6 0x0000000000405013 in handle_alias (argcp=0x7fffffffdfdc, argv=0x7fffffffdfd0) at git.c:236\n#7 0x000000000040550a in run_argv (argcp=0x7fffffffdfdc, argv=0x7fffffffdfd0) at git.c:515\n#8 0x000000000040566a in main (argc=1, argv=0x7fffffffe0f0) at git.c:579\n\n\nWhen the \"-c\" option is specified, setenv will be called in git_config_push_parameter.\nSo it seems \"envchanged\" flag must be set to true in this case.\n\n git.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex a5ef3c6..e04e4d4 100644\n--- a/git.c\n+++ b/git.c\n@@ -153,6 +153,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n \t\t\tgit_config_push_parameter((*argv)[1]);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else {\n\nAfter applying this patch, it works.\n\n$ ./git a\nfatal: alias 'a' changes environment variables\nYou can use '!git' in the alias to do this.\n\n$ vi ~/.gitconfig\n[alias]\n a = !git -c n=v status\n ~~~~\n\n$ ./git a\n# On branch master\nnothing to commit (working directory clean)\n\n-- \nKazuki Tsujimoto\n"},{"id":"168342","messageId":"7vhb8o25tt.fsf@alter.siamese.dyndns.org","threadId":"27419","inReplyTo":"20110521100126.E3CD.BA9123DE@callcc.net","subject":"Re: [BUG] realloc failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-21T01:35:58Z","receivedAt":"2011-05-21T01:35:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kazuki Tsujimoto writes:\n\n> The following command causes \"fatal: Out of memory, realloc failed\" error.\n> ...\n\nCare to send a patch with test (see Documentation/SubmittingPatches)?\n\nThanks.\n"},{"id":"168347","messageId":"20110521145056.E3F5.BA9123DE@callcc.net","threadId":"27419","inReplyTo":"7vhb8o25tt.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] realloc failed","fromName":"Kazuki Tsujimoto","fromEmail":"kazuki@callcc.net","sentAt":"2011-05-21T05:50:58Z","receivedAt":"2011-05-21T05:50:58Z","isPatch":false,"sender":{"key":"kazuki@callcc.net","avatar":"https://gravatar.com/avatar/4d88894b5eb851a4542b02acb4775858bcc8fb2125607c65b7c6fba0a6286465?d=mp&s=160"},"body":"From: Junio C Hamano \nDate: Fri, 20 May 2011 18:35:58 -0700\n\n> Care to send a patch with test (see Documentation/SubmittingPatches)?\n\nThanks for your comment.\nHere is a patch with test.\n\n\n[PATCH] Avoid \"realloc failed\" error in alias expansion including -c option\n\nWhen the -c option is specified, setenv will be called.\nTherefore, set envchanged flag to true so that git exits with\n\"alias '%s' changes environment variables\" error.\n\nSigned-off-by: Kazuki Tsujimoto \n---\n git.c | 2 ++\n t/t1020-subdirectory.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex a5ef3c6..e04e4d4 100644\n--- a/git.c\n+++ b/git.c\n@@ -153,6 +153,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n \t\t\tgit_config_push_parameter((*argv)[1]);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else {\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex ddc3921..94fed9c 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -119,6 +119,21 @@ test_expect_success 'alias expansion' '\n \t)\n '\n \n+test_expect_success 'alias expansion including \"-c =\" option' '\n+\tprintf \"%s\\n%s\\n\" \\\n+\t \"fatal: alias '\"'ss'\"' changes environment variables\" \\\n+\t \"You can use '\"'!git'\"' in the alias to do this.\">expect &&\n+\t(\n+\t\tcat > .git/config < ../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '!alias expansion' '\n \tpwd >expect &&\n \t(\n\n\n-- \nKazuki Tsujimoto\n"},{"id":"168452","messageId":"7v8vtyy1ft.fsf@alter.siamese.dyndns.org","threadId":"27419","inReplyTo":"20110521145056.E3F5.BA9123DE@callcc.net","subject":"Re: [BUG] realloc failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-22T19:34:14Z","receivedAt":"2011-05-22T19:34:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kazuki Tsujimoto writes:\n\n> From: Junio C Hamano \n> Date: Fri, 20 May 2011 18:35:58 -0700\n>\n>> Care to send a patch with test (see Documentation/SubmittingPatches)?\n>\n> Thanks for your comment.\n> Here is a patch with test.\n>\n> [PATCH] Avoid \"realloc failed\" error in alias expansion including -c option\n\nJust for your future reference.\n\nRetitle the \"Subject:\" to the above line, and move everything above after\nthe \"---\" line. Another style that is usable in a discussion thread is to\nuse a scissors-line \"-- >8 --\" to say \"ignore everything above this line\"\n(I'll use this message as a demonstration), but we didn't have a long\nthread here, so just a straight patch with comment after \"---\" is preferred\nin this case.\n\n>\n> When the -c option is specified, setenv will be called.\n> Therefore, set envchanged flag to true so that git exits with\n> \"alias '%s' changes environment variables\" error.\n>\n> Signed-off-by: Kazuki Tsujimoto \n> ---\n> git.c | 2 ++\n> t/t1020-subdirectory.sh | 15 +++++++++++++++\n> 2 files changed, 17 insertions(+), 0 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index a5ef3c6..e04e4d4 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -153,6 +153,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\t\tusage(git_usage_string);\n> \t\t\t}\n> \t\t\tgit_config_push_parameter((*argv)[1]);\n> +\t\t\tif (envchanged)\n> +\t\t\t\t*envchanged = 1;\n> \t\t\t(*argv)++;\n> \t\t\t(*argc)--;\n> \t\t} else {\n\nSorry, but I have to say that you are solving a wrong problem with a wrong\napproach.\n\nThe realloc() failure you saw is caused by this sequence:\n\n option_count = handle_options(&new_argv, &count, &envchanged);\n ...\n new_argv -= option_count;\n ...\n new_argv = xrealloc(new_argv, sizeof(char *) * (count + *argcp));\n\nThe \"handle_options()\" function advances new_argv, trying to strip the\nearlier items it recognized, and indicates how many it stripped by its\nreturn value. When the caller wants to resize the arguments array, it\nno longer points at the beginning of the buffer returned by malloc(),\nand compensates what handle_options() did by subtracting option_count\nbefore calling realloc().\n\nThe immediate cause of the problem is that handle_options() returns a\nwrong count when it sees a \"-c \". If you look at its\nimplementation, you see \"handled++\" paired with \"(*argv)++\" everywhere but\nthe place it parsed \"-c\".\n\nThere is no inherent reason to forbid \"-c \" in\nthe alias, e.g. \"-c ui.color=no diff\". Your patch may avoid the failure\nfrom the realloc, but breaks uses of such aliases.\n\nThe \"envchanged\" thing is there to avoid a situation like this:\n\n * You have an alias \"foo\" in .git/config;\n\n * You run \"git --git-dir=/some/where/else foo\";\n\n * We may read that \"foo\" alias from .git/config in your current\n directory, or we may notice the \"--git-dir\" on the command line, and\n may not to even read from that particular .git/config file that has\n \"foo\". Depending on the implementation of git (and its vintage), this\n will lead to an unexpected behaviour.\n\nSo a patch to fix the \"immediate cause\" would probably look like this\ninstead.\n\n-- >8 --\nSubject: handle_options(): do not miscount how many arguments were used\n\nThe handle_options() function advances the base of the argument array and\nreturns the number of arguments it used. The caller in handle_alias()\nwants to reallocate the argv array it passes to this function, and\nattempts to do so by subtracting the returned value to compensate for the\nchange handle_options() makes to the new_argv.\n\nBut handle_options() did not correctly count when \"-c \" is\ngiven, causing a wrong pointer to be passed to realloc().\n\nFix it by saving the original argv at the beginning of handle_options(),\nand return the difference between the final value of argv, which will\nrelieve the places that move the array pointer from the additional burden\nof keeping track of \"handled\" counter.\n\nSigned-off-by: Junio C Hamano \n---\n git.c | 6 ++----\n 1 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex ef598c3..df4306d 100644\n--- a/git.c\n+++ b/git.c\n@@ -66,7 +66,7 @@ static void commit_pager_choice(void) {\n \n static int handle_options(const char ***argv, int *argc, int *envchanged)\n {\n-\tint handled = 0;\n+\tconst char **orig_argv = *argv;\n \n \twhile (*argc > 0) {\n \t\tconst char *cmd = (*argv)[0];\n@@ -116,7 +116,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n-\t\t\thandled++;\n \t\t} else if (!prefixcmp(cmd, \"--git-dir=\")) {\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n \t\t\tif (envchanged)\n@@ -156,9 +155,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \n \t\t(*argv)++;\n \t\t(*argc)--;\n-\t\thandled++;\n \t}\n-\treturn handled;\n+\treturn (*argv) - orig_argv;\n }\n \n static int handle_alias(int *argcp, const char ***argv)\n"},{"id":"168454","messageId":"7v4o4my11c.fsf@alter.siamese.dyndns.org","threadId":"27419","inReplyTo":"7v8vtyy1ft.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] realloc failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-22T19:42:55Z","receivedAt":"2011-05-22T19:42:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The immediate cause of the problem is that handle_options() returns a\n> wrong count when it sees a \"-c \". If you look at its\n> implementation, you see \"handled++\" paired with \"(*argv)++\" everywhere but\n> the place it parsed \"-c\".\n\nI said \"immediate\" above, because I think the \"real\" reason why we\nsuffered is because handle_options() helper function was created by\nrefactoring existing code with a poor taste in designing an API.\n\nIt is understandable that it is more convenent for one callsite, which is\ngit.c::main(), to make the helper automatically move the argc/argv\nvariables of the caller, and it certainly would have made the initial\npatch look smaller when the helper was introduced, but as an API for a\nmore general callers, it is easier to follow the resulting code of the\ncaller if it didn't.\n\nRefactored in a more reasonable way, the result may look like this, on top\nof the previous patch.\n\n*CAUTION* I was not very careful checking sites that increment these\nvariables and rely on the result when I was doing this, so there may be\nplaces that need to be adjusted by adding or subtracting option_count)\n\n\n git.c | 51 +++++++++++++++++++++++++++------------------------\n 1 files changed, 27 insertions(+), 24 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex df4306d..3596f5b 100644\n--- a/git.c\n+++ b/git.c\n@@ -64,12 +64,12 @@ static void commit_pager_choice(void) {\n \t}\n }\n \n-static int handle_options(const char ***argv, int *argc, int *envchanged)\n+static int handle_options(int argc, const char **argv, int *envchanged)\n {\n-\tconst char **orig_argv = *argv;\n+\tconst char **orig_argv = argv;\n \n-\twhile (*argc > 0) {\n-\t\tconst char *cmd = (*argv)[0];\n+\twhile (argc > 0) {\n+\t\tconst char *cmd = argv[0];\n \t\tif (cmd[0] != '-')\n \t\t\tbreak;\n \n@@ -107,29 +107,29 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--git-dir\")) {\n-\t\t\tif (*argc < 2) {\n+\t\t\tif (argc < 2) {\n \t\t\t\tfprintf(stderr, \"No directory given for --git-dir.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\tsetenv(GIT_DIR_ENVIRONMENT, argv[1], 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n-\t\t\t(*argv)++;\n-\t\t\t(*argc)--;\n+\t\t\targv++;\n+\t\t\targc--;\n \t\t} else if (!prefixcmp(cmd, \"--git-dir=\")) {\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--work-tree\")) {\n-\t\t\tif (*argc < 2) {\n+\t\t\tif (argc < 2) {\n \t\t\t\tfprintf(stderr, \"No directory given for --work-tree.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, argv[1], 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n-\t\t\t(*argv)++;\n-\t\t\t(*argc)--;\n+\t\t\targv++;\n+\t\t\targc--;\n \t\t} else if (!prefixcmp(cmd, \"--work-tree=\")) {\n \t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);\n \t\t\tif (envchanged)\n@@ -141,22 +141,22 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"-c\")) {\n-\t\t\tif (*argc < 2) {\n+\t\t\tif (argc < 2) {\n \t\t\t\tfprintf(stderr, \"-c expects a configuration string\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tgit_config_push_parameter((*argv)[1]);\n-\t\t\t(*argv)++;\n-\t\t\t(*argc)--;\n+\t\t\tgit_config_push_parameter(argv[1]);\n+\t\t\targv++;\n+\t\t\targc--;\n \t\t} else {\n \t\t\tfprintf(stderr, \"Unknown option: %s\\n\", cmd);\n \t\t\tusage(git_usage_string);\n \t\t}\n \n-\t\t(*argv)++;\n-\t\t(*argc)--;\n+\t\targv++;\n+\t\targc--;\n \t}\n-\treturn (*argv) - orig_argv;\n+\treturn argv - orig_argv;\n }\n \n static int handle_alias(int *argcp, const char ***argv)\n@@ -198,14 +198,14 @@ static int handle_alias(int *argcp, const char ***argv)\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-\t\toption_count = handle_options(&new_argv, &count, &envchanged);\n+\t\toption_count = handle_options(count, new_argv, &envchanged);\n+\t\tcount -= option_count;\n \t\tif (envchanged)\n \t\t\tdie(\"alias '%s' changes environment variables\\n\"\n \t\t\t\t \"You can use '!git' in the alias to do this.\",\n \t\t\t\t alias_command);\n-\t\tmemmove(new_argv - option_count, new_argv,\n-\t\t\t\tcount * sizeof(char *));\n-\t\tnew_argv -= option_count;\n+\t\tmemmove(new_argv, new_argv + option_count,\n+\t\t\tcount * sizeof(char *));\n \n \t\tif (count < 1)\n \t\t\tdie(\"empty alias for %s\", alias_command);\n@@ -508,6 +508,7 @@ static int run_argv(int *argcp, const char ***argv)\n int main(int argc, const char **argv)\n {\n \tconst char *cmd;\n+\tint option_count;\n \n \tstartup_info = &git_startup_info;\n \n@@ -535,7 +536,9 @@ int main(int argc, const char **argv)\n \t/* Look for flags.. */\n \targv++;\n \targc--;\n-\thandle_options(&argv, &argc, NULL);\n+\toption_count = handle_options(argc, argv, NULL);\n+\targv += option_count;\n+\targc -= option_count;\n \tif (argc > 0) {\n \t\tif (!prefixcmp(argv[0], \"--\"))\n \t\t\targv[0] += 2;\n"},{"id":"168874","messageId":"20110527225913.075B.BA9123DE@callcc.net","threadId":"27419","inReplyTo":"7v4o4my11c.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] realloc failed","fromName":"Kazuki Tsujimoto","fromEmail":"kazuki@callcc.net","sentAt":"2011-05-27T13:59:15Z","receivedAt":"2011-05-27T13:59:15Z","isPatch":false,"sender":{"key":"kazuki@callcc.net","avatar":"https://gravatar.com/avatar/4d88894b5eb851a4542b02acb4775858bcc8fb2125607c65b7c6fba0a6286465?d=mp&s=160"},"body":"Subject: Re: [BUG] realloc failed\nFrom: Junio C Hamano \nDate: Sun, 22 May 2011 12:34:14 -0700\n\n> Just for your future reference.\n> ...snip...\n\nThanks for useful information, and reviewing the patch.\nI got it.\n\n-- \nKazuki Tsujimoto\n"}]}