{"thread":{"id":"41259","subject":"[BUG] typo DWIMery with alias broken (cd to random dir)","startedAt":"2016-01-26T07:37:25Z","lastAt":"2016-02-03T02:19:32Z","messageCount":22,"participants":["Michael J Gruber","Duy Nguyen","Nguyễn Thái Ngọc Duy","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"276763","messageId":"56A72235.9080602@drmicha.warpmail.net","threadId":"41259","inReplyTo":null,"subject":"[BUG] typo DWIMery with alias broken (cd to random dir)","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-01-26T07:37:25Z","receivedAt":"2016-01-26T07:37:25Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Hi there,\n\nwith current next (989ee58 plus local additions) it seems that typo\nDWIMery with aliases is broken, see below.\n\nIt appears that the typo DWIMery is broken only when there is a unique\nautomatic DWIM substitution for a mistyped alias.\n\nI haven't bisected yet, but I suspect this to be related to recent\nchanges regarding the environment in which commands/aliases are started\n(though this happens without extra work trees), so I'm cc'ing an expert\nin that area. Funny, though, that my user name shows up...\n\nI think the reason is that git.c's handle_alias() (or something else)\ncalls restore_env() multiple times, and restore_env frees orig_cwd such\nthat subsequent restore_env(0) with external_alias=0 tries to cd to a\nrandom location.\n\nI have no idea whether orig_cwd=0 after freeing or something else would\nbe the proper fix.\n\nMichael\n\nLANG=C git sss\nWARNING: You called a Git command named 'sss', which does not exist.\nContinuing under the assumption that you meant 'ss'\nin 2.0 seconds automatically...\nfatal: could not move to git@drmicha.warpmail.net: No such file or directory\n[mjg@skimbleshanks git]✗ LANG=C git ss\n## HEAD (no branch)\n?? a\n?? a.patch\n?? c2d.sh\n[mjg@skimbleshanks git]✓ LANG=C git statu -sb\ngit: 'statu' is not a git command. See 'git --help'.\n\nDid you mean one of these?\n        status\n        stage\n        stash\n[mjg@skimbleshanks git]✗ LANG=C git statuss -sb\nWARNING: You called a Git command named 'statuss', which does not exist.\nContinuing under the assumption that you meant 'status'\nin 2.0 seconds automatically...\n## HEAD (no branch)\n?? a\n?? a.patch\n?? c2d.sh\n"},{"id":"276789","messageId":"CACsJy8C2ucbGxM0pJUA84bk=_uJaD7TsRr+tDOjuMpHOmXTk+w@mail.gmail.com","threadId":"41259","inReplyTo":"56A72235.9080602@drmicha.warpmail.net","subject":"Re: [BUG] typo DWIMery with alias broken (cd to random dir)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-01-26T12:50:41Z","receivedAt":"2016-01-26T12:50:41Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 26, 2016 at 2:37 PM, Michael J Gruber\n<git@drmicha.warpmail.net> wrote:\n> Hi there,\n>\n> with current next (989ee58 plus local additions) it seems that typo\n> DWIMery with aliases is broken, see below.\n>\n> It appears that the typo DWIMery is broken only when there is a unique\n> automatic DWIM substitution for a mistyped alias.\n>\n> I haven't bisected yet, but I suspect this to be related to recent\n> changes regarding the environment in which commands/aliases are started\n> (though this happens without extra work trees), so I'm cc'ing an expert\n> in that area. Funny, though, that my user name shows up...\n>\n> I think the reason is that git.c's handle_alias() (or something else)\n> calls restore_env() multiple times, and restore_env frees orig_cwd such\n> that subsequent restore_env(0) with external_alias=0 tries to cd to a\n> random location.\n>\n> I have no idea whether orig_cwd=0 after freeing or something else would\n> be the proper fix.\n\nI think the key is to reset saved_env_before_alias to zero in\nrestore_env(). With that flag remains set, the second save_env turns\nno-op. orig_cwd should be reset too, but that's minor. It's working\nfor me. I'll send a patch later.\n-- \nDuy\n"},{"id":"276792","messageId":"1453814801-1925-1-git-send-email-pclouds@gmail.com","threadId":"41259","inReplyTo":"56A72235.9080602@drmicha.warpmail.net","subject":"[PATCH] git.c: fix help.autocorrect after 57ea712 breaks it","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-01-26T13:26:41Z","receivedAt":"2016-01-26T13:26:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit 57ea712 (git.c: make sure we do not leak GIT_* to alias scripts -\n2015-12-20) does not realize that handle_alias() can be called multiple\ntimes because of the forever loop in run_argv(). The commit breaks alias\nchains.\n\nSuppose you have an alias \"abc\" that resolves to another alias \"def\",\nwhich finally resolve to \"git status\". handle_alias() is called twice.\nsave_env() and restore_env() are also called twice. But because of the\ncheck save_env_before_alias in save_env(), we save once while trying to\nrestore twice. Consequences are left for the reader's imagination.\n\nFortunately, you cannot make an alias of another alias. At least not\nyet. Unfortunately it can still happen with help.autocorrect, where your\nalias typo is treated as the first \"alias\", and it can be resolved to\nthe second alias. Then boom.\n\nMake sure we call save_env() and restore_env() in pairs. While at there,\nset orig_cwd to NULL after freeing it for hygiene.\n\nReported-by: Michael J Gruber <git@drmicha.warpmail.net>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n git.c                  | 11 ++++++++---\n t/t1300-repo-config.sh |  8 ++++++++\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex da278c3..cd733f7 100644\n--- a/git.c\n+++ b/git.c\n@@ -25,14 +25,15 @@ static const char *env_names[] = {\n \tGIT_PREFIX_ENVIRONMENT\n };\n static char *orig_env[4];\n-static int saved_env_before_alias;\n+static int saved_env_before_alias, saved;\n \n static void save_env_before_alias(void)\n {\n \tint i;\n-\tif (saved_env_before_alias)\n-\t\treturn;\n+\tif (saved)\n+\t\tdie(\"BUG: uneven pair of save_env/restore_env calls\");\n \tsaved_env_before_alias = 1;\n+\tsaved = 1;\n \torig_cwd = xgetcwd();\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\torig_env[i] = getenv(env_names[i]);\n@@ -44,9 +45,13 @@ static void save_env_before_alias(void)\n static void restore_env(int external_alias)\n {\n \tint i;\n+\tif (saved != 1)\n+\t\tdie(\"BUG: uneven pair of save_env/restore_env calls\");\n \tif (!external_alias && orig_cwd && chdir(orig_cwd))\n \t\tdie_errno(\"could not move to %s\", orig_cwd);\n \tfree(orig_cwd);\n+\torig_cwd = NULL;\n+\tsaved = 0;\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\tif (external_alias &&\n \t\t    !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 52678e7..3f95285 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1201,4 +1201,12 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '\n \t  \"die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600\"\n '\n \n+test_expect_success 'autocorrect and save_env/restore_env' '\n+\tgit config alias.ss status &&\n+\tgit config help.autocorrect 1 &&\n+\tgit sss --porcelain | grep actual >actual &&\n+\techo \"?? actual\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.7.0.288.g1d8ad15\n"},{"id":"276813","messageId":"56A7850D.6060805@drmicha.warpmail.net","threadId":"41259","inReplyTo":"1453814801-1925-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] git.c: fix help.autocorrect after 57ea712 breaks it","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-01-26T14:39:09Z","receivedAt":"2016-01-26T14:39:09Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Nguyễn Thái Ngọc Duy venit, vidit, dixit 26.01.2016 14:26:\n> Commit 57ea712 (git.c: make sure we do not leak GIT_* to alias scripts -\n> 2015-12-20) does not realize that handle_alias() can be called multiple\n> times because of the forever loop in run_argv(). The commit breaks alias\n> chains.\n> \n> Suppose you have an alias \"abc\" that resolves to another alias \"def\",\n> which finally resolve to \"git status\". handle_alias() is called twice.\n> save_env() and restore_env() are also called twice. But because of the\n> check save_env_before_alias in save_env(), we save once while trying to\n> restore twice. Consequences are left for the reader's imagination.\n> \n> Fortunately, you cannot make an alias of another alias. At least not\n> yet. Unfortunately it can still happen with help.autocorrect, where your\n> alias typo is treated as the first \"alias\", and it can be resolved to\n> the second alias. Then boom.\n> \n> Make sure we call save_env() and restore_env() in pairs. While at there,\n> set orig_cwd to NULL after freeing it for hygiene.\n> \n> Reported-by: Michael J Gruber <git@drmicha.warpmail.net>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n\nThe patch fixes it for me, thanks!\n\nMichael\n"},{"id":"276823","messageId":"xmqqsi1kck18.fsf@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"56A7850D.6060805@drmicha.warpmail.net","subject":"Re: [PATCH] git.c: fix help.autocorrect after 57ea712 breaks it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-26T17:44:51Z","receivedAt":"2016-01-26T17:44:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> Nguyễn Thái Ngọc Duy venit, vidit, dixit 26.01.2016 14:26:\n>> Commit 57ea712 (git.c: make sure we do not leak GIT_* to alias scripts -\n>> 2015-12-20) does not realize that handle_alias() can be called multiple\n>> times because of the forever loop in run_argv(). The commit breaks alias\n>> chains.\n>> \n>> Suppose you have an alias \"abc\" that resolves to another alias \"def\",\n>> which finally resolve to \"git status\". handle_alias() is called twice.\n>> save_env() and restore_env() are also called twice. But because of the\n>> check save_env_before_alias in save_env(), we save once while trying to\n>> restore twice. Consequences are left for the reader's imagination.\n>> \n>> Fortunately, you cannot make an alias of another alias. At least not\n>> yet. Unfortunately it can still happen with help.autocorrect, where your\n>> alias typo is treated as the first \"alias\", and it can be resolved to\n>> the second alias. Then boom.\n>> \n>> Make sure we call save_env() and restore_env() in pairs. While at there,\n>> set orig_cwd to NULL after freeing it for hygiene.\n>> \n>> Reported-by: Michael J Gruber <git@drmicha.warpmail.net>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>\n> The patch fixes it for me, thanks!\n>\n> Michael\n\nThanks for noticing, and thanks for describing the problem very\nclearly and fixing it quickly.\n"},{"id":"276827","messageId":"xmqqbn88cgfl.fsf@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"1453814801-1925-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] git.c: fix help.autocorrect after 57ea712 breaks it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-26T19:02:38Z","receivedAt":"2016-01-26T19:02:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> diff --git a/git.c b/git.c\n> index da278c3..cd733f7 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -25,14 +25,15 @@ static const char *env_names[] = {\n>  \tGIT_PREFIX_ENVIRONMENT\n>  };\n>  static char *orig_env[4];\n> -static int saved_env_before_alias;\n> +static int saved_env_before_alias, saved;\n\nEven for a file-local static, this name is a bit too generic, isn't\nit?  saved_env_count or something perhaps?\n\n\n>  static void save_env_before_alias(void)\n>  {\n>  \tint i;\n> -\tif (saved_env_before_alias)\n> -\t\treturn;\n> +\tif (saved)\n> +\t\tdie(\"BUG: uneven pair of save_env/restore_env calls\");\n>  \tsaved_env_before_alias = 1;\n> +\tsaved = 1;\n>  \torig_cwd = xgetcwd();\n>  \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n>  \t\torig_env[i] = getenv(env_names[i]);\n> @@ -44,9 +45,13 @@ static void save_env_before_alias(void)\n>  static void restore_env(int external_alias)\n>  {\n>  \tint i;\n> +\tif (saved != 1)\n> +\t\tdie(\"BUG: uneven pair of save_env/restore_env calls\");\n>  \tif (!external_alias && orig_cwd && chdir(orig_cwd))\n>  \t\tdie_errno(\"could not move to %s\", orig_cwd);\n>  \tfree(orig_cwd);\n> +\torig_cwd = NULL;\n> +\tsaved = 0;\n>  \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n>  \t\tif (external_alias &&\n>  \t\t    !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))\n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 52678e7..3f95285 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n> @@ -1201,4 +1201,12 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '\n>  \t  \"die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600\"\n>  '\n>  \n> +test_expect_success 'autocorrect and save_env/restore_env' '\n> +\tgit config alias.ss status &&\n> +\tgit config help.autocorrect 1 &&\n> +\tgit sss --porcelain | grep actual >actual &&\n> +\techo \"?? actual\" >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_done\n"},{"id":"276829","messageId":"xmqq60ygcd9a.fsf@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"1453814801-1925-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] git.c: fix help.autocorrect after 57ea712 breaks it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-26T20:11:13Z","receivedAt":"2016-01-26T20:11:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Commit 57ea712 (git.c: make sure we do not leak GIT_* to alias scripts -\n> 2015-12-20) does not realize that handle_alias() can be called multiple\n> times because of the forever loop in run_argv(). The commit breaks alias\n> chains.\n>\n> Suppose you have an alias \"abc\" that resolves to another alias \"def\",\n> which finally resolve to \"git status\". handle_alias() is called twice.\n> save_env() and restore_env() are also called twice. But because of the\n> check save_env_before_alias in save_env(), we save once while trying to\n> restore twice. Consequences are left for the reader's imagination.\n>\n> Fortunately, you cannot make an alias of another alias. At least not\n> yet. Unfortunately it can still happen with help.autocorrect, where your\n> alias typo is treated as the first \"alias\", and it can be resolved to\n> the second alias. Then boom.\n>\n> Make sure we call save_env() and restore_env() in pairs. While at there,\n> set orig_cwd to NULL after freeing it for hygiene.\n>\n> Reported-by: Michael J Gruber <git@drmicha.warpmail.net>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n\nI spoke too soon, I am afraid.\n\nThe log message talks about \"we save once while trying to restore\ntwice\" being bad, and the fix is to make sure we save and restore in\npairs.  Before or after the patch, however, there is no change in\nthe callers of save_env_before_alias() and restore_env(), and it is\nunclear how that is ensured.\n\nIt turns out that the callers are doing the right thing (assuming\nthat calling save and restore in pairs is the right solution), and\nthe culprit is in the save_env_before_alias() function that returns\nwithout saving when we were called.  That is mentioned in the log,\nbut the description of last paragraph leaves the impression that it\nwas callers that needed fixing, which is misleading.\n\nAfter the patch, we have \"saved\" and \"saved_env_before_alias\".\n\n - The former is a protection against committing similar programming\n   errors in the future (i.e. glorified assert()).  Can we have it\n   in a separate commit, perhaps on top of the real fix, to make the\n   change for the real fix easier to understand?\n\n - Do we still need the latter?  saved_env_before_alias is set to 1\n   if we called save_env_before_alias() even once, which happens\n   always (and only) at the beginning of handle_alias(), so that is\n   equivalent to saying \"have we ever called handle_alias()?\"\n\n   And there is only one caller of handle_alias, in run_argv(), and\n   it maintains yet another variable \"done_alias\" to keep track of\n   the same information.\n\n   The only code that cares about saved_env_before_alias is\n   handle_builtin(), but it is a glorified no-op if\n   saved_env_before_alias() is set.  And the only caller of this\n   handle_builtin(), for which saved_env_before_alias matters, is\n   the same run_argv().\n\nI wonder if this would be better done as a multi-part series that\ngoes like this:\n\n [1/3] git: remove an early return from save_env_before_alias()\n\n       whose description would be the first two paragraphs of your\n       patch, with two line removal, i.e.\n\n        diff --git a/git.c b/git.c\n        index 98d4412..a57a4cb 100644\n        --- a/git.c\n        +++ b/git.c\n        @@ -30,8 +30,6 @@ static int saved_env_before_alias;\n         static void save_env_before_alias(void)\n         {\n                int i;\n        -\tif (saved_env_before_alias)\n        -\t\treturn;\n                saved_env_before_alias = 1;\n                orig_cwd = xgetcwd();\n                for (i = 0; i < ARRAY_SIZE(env_names); i++) {\n\n       that should be a sufficient \"fix\" to the issue.\n\n [2/3] git: protect against unbalanced calls to {save,restore}_env()\n\n       We made sure that save_env_before_alias() does not skip\n       saving the environment when asked to (which led to double\n       free of orig_cwd in restore_env()) with the previous step.\n       Protect against future breakage where somebody adds new\n       callers of these functions in an unbalanced fashion.\n       \n        diff --git a/git.c b/git.c\n        index a57a4cb..e39b972 100644\n        --- a/git.c\n        +++ b/git.c\n        @@ -26,11 +26,15 @@ static const char *env_names[] = {\n         };\n         static char *orig_env[4];\n         static int saved_env_before_alias;\n        +static int save_restore_env_balance;\n\n         static void save_env_before_alias(void)\n         {\n                int i;\n                saved_env_before_alias = 1;\n        +\n        +\tassert(save_restore_env_balance == 0);\n        +\tsave_restore_env_balance = 1;\n                orig_cwd = xgetcwd();\n                for (i = 0; i < ARRAY_SIZE(env_names); i++) {\n                        orig_env[i] = getenv(env_names[i]);\n        @@ -42,6 +46,9 @@ static void save_env_before_alias(void)\n         static void restore_env(int external_alias)\n         {\n                int i;\n        +\n        +\tassert(save_restore_env_balance == 1);\n        +\tsave_restore_env_balance = 0;\n                if (!external_alias && orig_cwd && chdir(orig_cwd))\n                        die_errno(\"could not move to %s\", orig_cwd);\n                free(orig_cwd);\n\n [3/3] git: simplify environment save/restore logic\n\n       The only code that cares about the value of the global\n       variable saved_env_before_alias after the previous fix is\n       handle_builtin() that turns into a glorified no-op when the\n       variable is true, so the logic could safely be lifted to its\n       caller, i.e. the caller can refrain from calling it when the\n       variable is set.\n\n       This variable tells us if save_env_before_alias() was called\n       (with or without matching restore_env()), but the sole caller\n       of the function, handle_alias(), always calls it the first as\n       thing, so it essentially keeps track of the fact that\n       handle_alias() has ever been called.\n\n       It turns out that handle_builtin() and handle_alias() are\n       called only from one function in a way that the value of the\n       variable matters, which is run_argv(), and it already keeps\n       track of the fact that it called handle_alias().\n\n       So we can simplify the whole thing by:\n\n       - Change handle_builtin() to always make a direct call to the\n         builtin implementation it finds, and make sure the caller\n         refrains from calling it if handle_alias() has ever been\n         called;\n\n       - Remove saved_env_before_alias variable, and instead use the\n         local \"done_alias\" variable maintained inside run_argv() to\n         make the above decision.\n\n [4/3] git: plug leaks of saved environments\n\n       I think orig_env[] members if save_env() is called twice are\n       leaked, so they would need to be free'd just like orig_cwd is\n       freed in restore_env().  Optionally, you can assign NULL to\n       them and also to orig_cwd after freeing.\n\n\n\nThe patch text for [3/3] would look like this.\n\n git.c | 29 ++++++++++++++---------------\n 1 file changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex e39b972..c8d7b56 100644\n--- a/git.c\n+++ b/git.c\n@@ -25,13 +25,11 @@ static const char *env_names[] = {\n \tGIT_PREFIX_ENVIRONMENT\n };\n static char *orig_env[4];\n-static int saved_env_before_alias;\n static int save_restore_env_balance;\n \n static void save_env_before_alias(void)\n {\n \tint i;\n-\tsaved_env_before_alias = 1;\n \n \tassert(save_restore_env_balance == 0);\n \tsave_restore_env_balance = 1;\n@@ -533,16 +531,8 @@ static void handle_builtin(int argc, const char **argv)\n \t}\n \n \tbuiltin = get_builtin(cmd);\n-\tif (builtin) {\n-\t\t/*\n-\t\t * XXX: if we can figure out cases where it is _safe_\n-\t\t * to do, we can avoid spawning a new process when\n-\t\t * saved_env_before_alias is true\n-\t\t * (i.e. setup_git_dir* has been run once)\n-\t\t */\n-\t\tif (!saved_env_before_alias)\n-\t\t\texit(run_builtin(builtin, argc, argv));\n-\t}\n+\tif (builtin)\n+\t\texit(run_builtin(builtin, argc, argv));\n }\n \n static void execv_dashed_external(const char **argv)\n@@ -586,8 +576,17 @@ static int run_argv(int *argcp, const char ***argv)\n \tint done_alias = 0;\n \n \twhile (1) {\n-\t\t/* See if it's a builtin */\n-\t\thandle_builtin(*argcp, *argv);\n+\t\t/*\n+\t\t * If we tried alias and futzed with our environment,\n+\t\t * it no longer is safe to invoke builtins directly in\n+\t\t * general.  We have to spawn them as dashed externals.\n+\t\t *\n+\t\t * NEEDSWORK: if we can figure out cases\n+\t\t * where it is safe to do, we can avoid spawning a new\n+\t\t * process.\n+\t\t */\n+\t\tif (!done_alias)\n+\t\t\thandle_builtin(*argcp, *argv);\n \n \t\t/* .. then try the external ones */\n \t\texecv_dashed_external(*argv);\n@@ -598,9 +597,9 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t */\n \t\tif (done_alias)\n \t\t\tbreak;\n+\t\tdone_alias = 1;\n \t\tif (!handle_alias(argcp, argv))\n \t\t\tbreak;\n-\t\tdone_alias = 1;\n \t}\n \n \treturn done_alias;\n"},{"id":"276850","messageId":"xmqqbn87a54v.fsf@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqq60ygcd9a.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git.c: fix help.autocorrect after 57ea712 breaks it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T06:49:36Z","receivedAt":"2016-01-27T06:49:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I spoke too soon, I am afraid.\n> ...\n> I wonder if this would be better done as a multi-part series that\n> goes like this:\n> ...\n\nSo here is the first of the three-patch series to replace it.\n\n-- >8 --\nFrom: Junio C Hamano <gitster@pobox.com>\nDate: Tue, 26 Jan 2016 11:46:53 -0800\nSubject: [PATCH 1/3] git: remove an early return from save_env_before_alias()\n\nWhen help.autocorrect is in effect, an attempt to auto-execute an\nuniquely corrected result of a misspelled alias will result in an\nirrelevant error message.  The codepath that causes this issue calls\nsave_env_before_alias() and restore_env() as a pair in handle_alias(),\nand that happens twice.  A global variable orig_cwd is allocated and\nholds the return value of getcwd() in save_env_before_alias(), which\nis used in restore_env() to go back to that directory and freed.\n\nHowever, save_env_before_alias() is not prepared to be called twice.\nIt returns early when it knows it has already been called, leaving\norig_cwd undefined, which is then checked in the second call to\nrestore_env(), and by that time, the memory that used to hold the\ncontents of orig_cwd is either freed or reused to hold something else,\nand this is fed to chdir(), causing it to fail.\n\nFix this by making sure save_env() does do the saving when called.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 98d4412..a57a4cb 100644\n--- a/git.c\n+++ b/git.c\n@@ -30,8 +30,6 @@ static int saved_env_before_alias;\n static void save_env_before_alias(void)\n {\n \tint i;\n-\tif (saved_env_before_alias)\n-\t\treturn;\n \tsaved_env_before_alias = 1;\n \torig_cwd = xgetcwd();\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n-- \n2.7.0-366-g065efda\n"},{"id":"276851","messageId":"xmqq7fiva53g.fsf_-_@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqqbn87a54v.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 2/3] git: protect against unbalanced calls to {save,restore}_env()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T06:50:27Z","receivedAt":"2016-01-27T06:50:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We made sure that save_env_before_alias() does not skip saving the\nenvironment when asked to (which led to double free of orig_cwd in\nrestore_env()) with the previous step.  Protect against future\nbreakage where somebody adds new callers of these functions in an\nunbalanced fashion.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n git.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex a57a4cb..e39b972 100644\n--- a/git.c\n+++ b/git.c\n@@ -26,11 +26,15 @@ static const char *env_names[] = {\n };\n static char *orig_env[4];\n static int saved_env_before_alias;\n+static int save_restore_env_balance;\n \n static void save_env_before_alias(void)\n {\n \tint i;\n \tsaved_env_before_alias = 1;\n+\n+\tassert(save_restore_env_balance == 0);\n+\tsave_restore_env_balance = 1;\n \torig_cwd = xgetcwd();\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\torig_env[i] = getenv(env_names[i]);\n@@ -42,6 +46,9 @@ static void save_env_before_alias(void)\n static void restore_env(int external_alias)\n {\n \tint i;\n+\n+\tassert(save_restore_env_balance == 1);\n+\tsave_restore_env_balance = 0;\n \tif (!external_alias && orig_cwd && chdir(orig_cwd))\n \t\tdie_errno(\"could not move to %s\", orig_cwd);\n \tfree(orig_cwd);\n-- \n2.7.0-368-g4610598\n"},{"id":"276852","messageId":"xmqq37tja50t.fsf_-_@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqqbn87a54v.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 3/3] git: simplify environment save/restore logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T06:52:02Z","receivedAt":"2016-01-27T06:52:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The only code that cares about the value of the global variable\nsaved_env_before_alias after the previous fix is handle_builtin()\nthat turns into a glorified no-op when the variable is true, so the\nlogic could safely be lifted to its caller, i.e. the caller can\nrefrain from calling it when the variable is set.\n\nThis variable tells us if save_env_before_alias() was called (with\nor without matching restore_env()), but the sole caller of the\nfunction, handle_alias(), always calls it the first as thing, so it\nessentially keeps track of the fact that handle_alias() has ever\nbeen called.\n\nIt turns out that handle_builtin() and handle_alias() are called\nonly from one function in a way that the value of the variable\nmatters, which is run_argv(), and it already keeps track of the fact\nthat it called handle_alias().\n\nSo we can simplify the whole thing by:\n\n- Change handle_builtin() to always make a direct call to the\n  builtin implementation it finds, and make sure the caller\n  refrains from calling it if handle_alias() has ever been\n  called;\n\n- Remove saved_env_before_alias variable, and instead use the\n  local \"done_alias\" variable maintained inside run_argv() to\n  make the above decision.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * I do not mean to say that it is unnecessary to plug the leak,\n   which should be the fourth patch in this three-patch series, but\n   it should rather be obvious to implement, so I omitted from this\n   series, which is primarily meant to be \"how about doing it this\n   way\" illustration.\n\n git.c | 29 ++++++++++++++---------------\n 1 file changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex e39b972..c8d7b56 100644\n--- a/git.c\n+++ b/git.c\n@@ -25,13 +25,11 @@ static const char *env_names[] = {\n \tGIT_PREFIX_ENVIRONMENT\n };\n static char *orig_env[4];\n-static int saved_env_before_alias;\n static int save_restore_env_balance;\n \n static void save_env_before_alias(void)\n {\n \tint i;\n-\tsaved_env_before_alias = 1;\n \n \tassert(save_restore_env_balance == 0);\n \tsave_restore_env_balance = 1;\n@@ -533,16 +531,8 @@ static void handle_builtin(int argc, const char **argv)\n \t}\n \n \tbuiltin = get_builtin(cmd);\n-\tif (builtin) {\n-\t\t/*\n-\t\t * XXX: if we can figure out cases where it is _safe_\n-\t\t * to do, we can avoid spawning a new process when\n-\t\t * saved_env_before_alias is true\n-\t\t * (i.e. setup_git_dir* has been run once)\n-\t\t */\n-\t\tif (!saved_env_before_alias)\n-\t\t\texit(run_builtin(builtin, argc, argv));\n-\t}\n+\tif (builtin)\n+\t\texit(run_builtin(builtin, argc, argv));\n }\n \n static void execv_dashed_external(const char **argv)\n@@ -586,8 +576,17 @@ static int run_argv(int *argcp, const char ***argv)\n \tint done_alias = 0;\n \n \twhile (1) {\n-\t\t/* See if it's a builtin */\n-\t\thandle_builtin(*argcp, *argv);\n+\t\t/*\n+\t\t * If we tried alias and futzed with our environment,\n+\t\t * it no longer is safe to invoke builtins directly in\n+\t\t * general.  We have to spawn them as dashed externals.\n+\t\t *\n+\t\t * NEEDSWORK: if we can figure out cases\n+\t\t * where it is safe to do, we can avoid spawning a new\n+\t\t * process.\n+\t\t */\n+\t\tif (!done_alias)\n+\t\t\thandle_builtin(*argcp, *argv);\n \n \t\t/* .. then try the external ones */\n \t\texecv_dashed_external(*argv);\n@@ -598,9 +597,9 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t */\n \t\tif (done_alias)\n \t\t\tbreak;\n+\t\tdone_alias = 1;\n \t\tif (!handle_alias(argcp, argv))\n \t\t\tbreak;\n-\t\tdone_alias = 1;\n \t}\n \n \treturn done_alias;\n-- \n2.7.0-368-g4610598\n"},{"id":"276878","messageId":"CACsJy8DzHYpw3TT3i17W-8eiR9J9DOQUO6mkxffUEnGqT1u96Q@mail.gmail.com","threadId":"41259","inReplyTo":"xmqqbn87a54v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git.c: fix help.autocorrect after 57ea712 breaks it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-01-27T09:14:52Z","receivedAt":"2016-01-27T09:14:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 27, 2016 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> I spoke too soon, I am afraid.\n>> ...\n>> I wonder if this would be better done as a multi-part series that\n>> goes like this:\n>> ...\n>\n> So here is the first of the three-patch series to replace it.\n\nThis is much better (the whole series, not just the first patch). We\nprobably should add a test about help.autocorrect though, maybe in the\nfirst patch, because it's not tested at all in the test suite.\n-- \nDuy\n"},{"id":"276887","messageId":"xmqqtwlz8c4w.fsf@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"CACsJy8DzHYpw3TT3i17W-8eiR9J9DOQUO6mkxffUEnGqT1u96Q@mail.gmail.com","subject":"Re: [PATCH] git.c: fix help.autocorrect after 57ea712 breaks it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T12:01:19Z","receivedAt":"2016-01-27T12:01:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Wed, Jan 27, 2016 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> I spoke too soon, I am afraid.\n>>> ...\n>>> I wonder if this would be better done as a multi-part series that\n>>> goes like this:\n>>> ...\n>>\n>> So here is the first of the three-patch series to replace it.\n>\n> This is much better (the whole series, not just the first patch). We\n> probably should add a test about help.autocorrect though, maybe in the\n> first patch, because it's not tested at all in the test suite.\n\nPatches welcome.  Thanks.\n"},{"id":"276943","messageId":"xmqqy4ba627n.fsf_-_@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqqtwlz8c4w.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 1/3] git: remove an early return from save_env_before_alias()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T23:18:36Z","receivedAt":"2016-01-27T23:18:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> On Wed, Jan 27, 2016 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Junio C Hamano <gitster@pobox.com> writes:\n>>>\n>>>> I spoke too soon, I am afraid.\n>>>> ...\n>>>> I wonder if this would be better done as a multi-part series that\n>>>> goes like this:\n>>>> ...\n>>>\n>>> So here is the first of the three-patch series to replace it.\n>>\n>> This is much better (the whole series, not just the first patch). We\n>> probably should add a test about help.autocorrect though, maybe in the\n>> first patch, because it's not tested at all in the test suite.\n>\n> Patches welcome.  Thanks.\n\nHere is an updated 1/3; this feature does not fit very well to any\ncategory (it is certainly not basic, or command that is primarily\nabout objects or worktree; I just picked \"Git tools\" t9xxx as that\nis the closest thing to \"direct end user support\").\n\n-- >8 --\nWhen help.autocorrect is in effect, an attempt to auto-execute an\nuniquely corrected result of a misspelt alias will result in an\nirrelevant error message.  The codepath that causes this calls\nsave_env_before_alias() and restore_env() in handle_alias(), and\nthat happens twice.  A global variable orig_cwd is allocated to hold\nthe return value of getcwd() in save_env_before_alias(), which is\nthen used in restore_env() to go back to that directory and finally\nfree(3)'d there.\n\nHowever, save_env_before_alias() is not prepared to be called twice.\nIt returns early when it knows it has already been called, leaving\norig_cwd undefined, which is then checked in the second call to\nrestore_env(), and by that time, the memory that used to hold the\ncontents of orig_cwd is either freed or reused to hold something\nelse, and this is fed to chdir(2), causing it to fail.  Even if it\ndid not fail (i.e. reading of the already free'd piece of memory\nyielded a directory path that we can chdir(2) to), it then gets\nfree(3)'d.\n\nFix this by making sure save_env() does do the saving when called.\n\nWhile at it, add a minimal test for help.autocorrect facility.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git.c                       |  2 --\n t/t9003-help-autocorrect.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 2 deletions(-)\n create mode 100755 t/t9003-help-autocorrect.sh\n\ndiff --git a/git.c b/git.c\nindex 98d4412..a57a4cb 100644\n--- a/git.c\n+++ b/git.c\n@@ -30,8 +30,6 @@ static int saved_env_before_alias;\n static void save_env_before_alias(void)\n {\n \tint i;\n-\tif (saved_env_before_alias)\n-\t\treturn;\n \tsaved_env_before_alias = 1;\n \torig_cwd = xgetcwd();\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\ndiff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh\nnew file mode 100755\nindex 0000000..dfe95c9\n--- /dev/null\n+++ b/t/t9003-help-autocorrect.sh\n@@ -0,0 +1,52 @@\n+#!/bin/sh\n+\n+test_description='help.autocorrect finding a match'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\t# An alias\n+\tgit config alias.lgf \"log --format=%s --first-parent\" &&\n+\n+\t# A random user-defined command\n+\twrite_script git-distimdistim <<-EOF &&\n+\t\techo distimdistim was called\n+\tEOF\n+\n+\tPATH=\"$PATH:.\" &&\n+\texport PATH &&\n+\n+\tgit commit --allow-empty -m \"a single log entry\" &&\n+\n+\t# Sanity check\n+\tgit lgf >actual &&\n+\techo \"a single log entry\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit distimdistim >actual &&\n+\techo \"distimdistim was called\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'autocorrect showing candidates' '\n+\tgit config help.autocorrect 0 &&\n+\n+\ttest_must_fail git lfg 2>actual &&\n+\tsed -e \"1,/^Did you mean this/d\" actual | grep lgf &&\n+\n+\ttest_must_fail git distimdist 2>actual &&\n+\tsed -e \"1,/^Did you mean this/d\" actual | grep distimdistim\n+'\n+\n+test_expect_success 'autocorrect running commands' '\n+\tgit config help.autocorrect -1 &&\n+\n+\tgit lfg >actual &&\n+\techo \"a single log entry\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit distimdist >actual &&\n+\techo \"distimdistim was called\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.7.0-368-gb6e04f9\n"},{"id":"276944","messageId":"xmqqtwly625m.fsf_-_@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqq7fiva53g.fsf_-_@gitster.mtv.corp.google.com","subject":"[PATCH v2 2/3] git: protect against unbalanced calls to {save,restore}_env()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T23:19:49Z","receivedAt":"2016-01-27T23:19:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We made sure that save_env_before_alias() does not skip saving the\nenvironment when asked to (which led to use-after-free of orig_cwd\nin restore_env() in the buggy version) with the previous step.\n\nProtect against future breakage where somebody adds new callers of\nthese functions in an unbalanced fashion.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex a57a4cb..e39b972 100644\n--- a/git.c\n+++ b/git.c\n@@ -26,11 +26,15 @@ static const char *env_names[] = {\n };\n static char *orig_env[4];\n static int saved_env_before_alias;\n+static int save_restore_env_balance;\n \n static void save_env_before_alias(void)\n {\n \tint i;\n \tsaved_env_before_alias = 1;\n+\n+\tassert(save_restore_env_balance == 0);\n+\tsave_restore_env_balance = 1;\n \torig_cwd = xgetcwd();\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\torig_env[i] = getenv(env_names[i]);\n@@ -42,6 +46,9 @@ static void save_env_before_alias(void)\n static void restore_env(int external_alias)\n {\n \tint i;\n+\n+\tassert(save_restore_env_balance == 1);\n+\tsave_restore_env_balance = 0;\n \tif (!external_alias && orig_cwd && chdir(orig_cwd))\n \t\tdie_errno(\"could not move to %s\", orig_cwd);\n \tfree(orig_cwd);\n-- \n2.7.0-368-gb6e04f9\n"},{"id":"276946","messageId":"xmqqoac6621o.fsf_-_@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqq37tja50t.fsf_-_@gitster.mtv.corp.google.com","subject":"[PATCH v2 3/3] git: simplify environment save/restore logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T23:22:11Z","receivedAt":"2016-01-27T23:22:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The only code that cares about the value of the global variable\nsaved_env_before_alias after the previous fix is handle_builtin()\nthat turns into a glorified no-op when the variable is true, so the\nlogic could safely be lifted to its caller, i.e. the caller can\nrefrain from calling it when the variable is set.\n\nThis variable tells us if save_env_before_alias() was called (with\nor without matching restore_env()), but the sole caller of the\nfunction, handle_alias(), always calls it as the first thing, so we\ncan consider that the variable essentially keeps track of the fact\nthat handle_alias() has ever been called.\n\nIt turns out that handle_builtin() and handle_alias() are called\nonly from one function in a way that the value of the variable\nmatters, which is run_argv(), and it already keeps track of the\nfact that it already called handle_alias().\n\nSo we can simplify the whole thing by:\n\n- Change handle_builtin() to always make a direct call to the\n  builtin implementation it finds, and make sure the caller\n  refrains from calling it if handle_alias() has ever been\n  called;\n\n- Remove saved_env_before_alias variable, and instead use the\n  local \"done_alias\" variable maintained inside run_argv() to\n  make the same decision.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Patches 2 and 3 are with updated log messages; their patch text\n   did not change.\n\n git.c | 29 ++++++++++++++---------------\n 1 file changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex e39b972..c8d7b56 100644\n--- a/git.c\n+++ b/git.c\n@@ -25,13 +25,11 @@ static const char *env_names[] = {\n \tGIT_PREFIX_ENVIRONMENT\n };\n static char *orig_env[4];\n-static int saved_env_before_alias;\n static int save_restore_env_balance;\n \n static void save_env_before_alias(void)\n {\n \tint i;\n-\tsaved_env_before_alias = 1;\n \n \tassert(save_restore_env_balance == 0);\n \tsave_restore_env_balance = 1;\n@@ -533,16 +531,8 @@ static void handle_builtin(int argc, const char **argv)\n \t}\n \n \tbuiltin = get_builtin(cmd);\n-\tif (builtin) {\n-\t\t/*\n-\t\t * XXX: if we can figure out cases where it is _safe_\n-\t\t * to do, we can avoid spawning a new process when\n-\t\t * saved_env_before_alias is true\n-\t\t * (i.e. setup_git_dir* has been run once)\n-\t\t */\n-\t\tif (!saved_env_before_alias)\n-\t\t\texit(run_builtin(builtin, argc, argv));\n-\t}\n+\tif (builtin)\n+\t\texit(run_builtin(builtin, argc, argv));\n }\n \n static void execv_dashed_external(const char **argv)\n@@ -586,8 +576,17 @@ static int run_argv(int *argcp, const char ***argv)\n \tint done_alias = 0;\n \n \twhile (1) {\n-\t\t/* See if it's a builtin */\n-\t\thandle_builtin(*argcp, *argv);\n+\t\t/*\n+\t\t * If we tried alias and futzed with our environment,\n+\t\t * it no longer is safe to invoke builtins directly in\n+\t\t * general.  We have to spawn them as dashed externals.\n+\t\t *\n+\t\t * NEEDSWORK: if we can figure out cases\n+\t\t * where it is safe to do, we can avoid spawning a new\n+\t\t * process.\n+\t\t */\n+\t\tif (!done_alias)\n+\t\t\thandle_builtin(*argcp, *argv);\n \n \t\t/* .. then try the external ones */\n \t\texecv_dashed_external(*argv);\n@@ -598,9 +597,9 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t */\n \t\tif (done_alias)\n \t\t\tbreak;\n+\t\tdone_alias = 1;\n \t\tif (!handle_alias(argcp, argv))\n \t\t\tbreak;\n-\t\tdone_alias = 1;\n \t}\n \n \treturn done_alias;\n-- \n2.7.0-368-gb6e04f9\n"},{"id":"276948","messageId":"xmqqk2mu60vl.fsf@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqqoac6621o.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/3] git: simplify environment save/restore logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T23:47:26Z","receivedAt":"2016-01-27T23:47:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> @@ -598,9 +597,9 @@ static int run_argv(int *argcp, const char ***argv)\n>  \t\t */\n>  \t\tif (done_alias)\n>  \t\t\tbreak;\n> +\t\tdone_alias = 1;\n>  \t\tif (!handle_alias(argcp, argv))\n>  \t\t\tbreak;\n> -\t\tdone_alias = 1;\n>  \t}\n>  \n>  \treturn done_alias;\n\nThis hunk shouldn't be there; it changes the return value from this\nfunction and breaks the whole thing.\n\nWhat I will push out as part of 'pu' will have this fixed.\n"},{"id":"277285","messageId":"xmqqvb66ptdd.fsf_-_@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqqy4ba627n.fsf_-_@gitster.mtv.corp.google.com","subject":"[PATCH v3 1/4] git: remove an early return from save_env_before_alias()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-02T23:47:26Z","receivedAt":"2016-02-02T23:47:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When help.autocorrect is in effect, an attempt to auto-execute an\nuniquely corrected result of a misspelt alias will result in an\nirrelevant error message.  The codepath that causes this calls\nsave_env_before_alias() and restore_env() in handle_alias(), and\nthat happens twice.  A global variable orig_cwd is allocated to hold\nthe return value of getcwd() in save_env_before_alias(), which is\nthen used in restore_env() to go back to that directory and finally\nfree(3)'d there.\n\nHowever, save_env_before_alias() is not prepared to be called twice.\nIt returns early when it knows it has already been called, leaving\norig_cwd undefined, which is then checked in the second call to\nrestore_env(), and by that time, the memory that used to hold the\ncontents of orig_cwd is either freed or reused to hold something\nelse, and this is fed to chdir(2), causing it to fail.  Even if it\ndid not fail (i.e. reading of the already free'd piece of memory\nyielded a directory path that we can chdir(2) to), it then gets\nfree(3)'d.\n\nFix this by making sure save_env() does do the saving when called.\n\nWhile at it, add a minimal test for help.autocorrect facility.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * For another review round before deciding what to write in the\n   upcoming \"What's cooking\" report.\n\n git.c                       |  2 --\n t/t9003-help-autocorrect.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 2 deletions(-)\n create mode 100755 t/t9003-help-autocorrect.sh\n\ndiff --git a/git.c b/git.c\nindex 98d4412..a57a4cb 100644\n--- a/git.c\n+++ b/git.c\n@@ -30,8 +30,6 @@ static int saved_env_before_alias;\n static void save_env_before_alias(void)\n {\n \tint i;\n-\tif (saved_env_before_alias)\n-\t\treturn;\n \tsaved_env_before_alias = 1;\n \torig_cwd = xgetcwd();\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\ndiff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh\nnew file mode 100755\nindex 0000000..dfe95c9\n--- /dev/null\n+++ b/t/t9003-help-autocorrect.sh\n@@ -0,0 +1,52 @@\n+#!/bin/sh\n+\n+test_description='help.autocorrect finding a match'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\t# An alias\n+\tgit config alias.lgf \"log --format=%s --first-parent\" &&\n+\n+\t# A random user-defined command\n+\twrite_script git-distimdistim <<-EOF &&\n+\t\techo distimdistim was called\n+\tEOF\n+\n+\tPATH=\"$PATH:.\" &&\n+\texport PATH &&\n+\n+\tgit commit --allow-empty -m \"a single log entry\" &&\n+\n+\t# Sanity check\n+\tgit lgf >actual &&\n+\techo \"a single log entry\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit distimdistim >actual &&\n+\techo \"distimdistim was called\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'autocorrect showing candidates' '\n+\tgit config help.autocorrect 0 &&\n+\n+\ttest_must_fail git lfg 2>actual &&\n+\tsed -e \"1,/^Did you mean this/d\" actual | grep lgf &&\n+\n+\ttest_must_fail git distimdist 2>actual &&\n+\tsed -e \"1,/^Did you mean this/d\" actual | grep distimdistim\n+'\n+\n+test_expect_success 'autocorrect running commands' '\n+\tgit config help.autocorrect -1 &&\n+\n+\tgit lfg >actual &&\n+\techo \"a single log entry\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit distimdist >actual &&\n+\techo \"distimdistim was called\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.7.0-391-gcd29568\n"},{"id":"277286","messageId":"xmqqr3guptck.fsf_-_@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqqy4ba627n.fsf_-_@gitster.mtv.corp.google.com","subject":"[PATCH v3 2/4] git: protect against unbalanced calls to {save,restore}_env()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-02T23:47:55Z","receivedAt":"2016-02-02T23:47:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We made sure that save_env_before_alias() does not skip saving the\nenvironment when asked to (which led to use-after-free of orig_cwd\nin restore_env() in the buggy version) with the previous step.\n\nProtect against future breakage where somebody adds new callers of\nthese functions in an unbalanced fashion.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex a57a4cb..e39b972 100644\n--- a/git.c\n+++ b/git.c\n@@ -26,11 +26,15 @@ static const char *env_names[] = {\n };\n static char *orig_env[4];\n static int saved_env_before_alias;\n+static int save_restore_env_balance;\n \n static void save_env_before_alias(void)\n {\n \tint i;\n \tsaved_env_before_alias = 1;\n+\n+\tassert(save_restore_env_balance == 0);\n+\tsave_restore_env_balance = 1;\n \torig_cwd = xgetcwd();\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\torig_env[i] = getenv(env_names[i]);\n@@ -42,6 +46,9 @@ static void save_env_before_alias(void)\n static void restore_env(int external_alias)\n {\n \tint i;\n+\n+\tassert(save_restore_env_balance == 1);\n+\tsave_restore_env_balance = 0;\n \tif (!external_alias && orig_cwd && chdir(orig_cwd))\n \t\tdie_errno(\"could not move to %s\", orig_cwd);\n \tfree(orig_cwd);\n-- \n2.7.0-391-gcd29568\n"},{"id":"277287","messageId":"xmqqmvriptaw.fsf_-_@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqqy4ba627n.fsf_-_@gitster.mtv.corp.google.com","subject":"[PATCH v3 3/4] git: simplify environment save/restore logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-02T23:48:55Z","receivedAt":"2016-02-02T23:48:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The only code that cares about the value of the global variable\nsaved_env_before_alias after the previous fix is handle_builtin()\nthat turns into a glorified no-op when the variable is true, so the\nlogic could safely be lifted to its caller, i.e. the caller can\nrefrain from calling it when the variable is set.\n\nThis variable tells us if save_env_before_alias() was called (with\nor without matching restore_env()), but the sole caller of the\nfunction, handle_alias(), always calls it as the first thing, so we\ncan consider that the variable essentially keeps track of the fact\nthat handle_alias() has ever been called.\n\nIt turns out that handle_builtin() and handle_alias() are called\nonly from one function in a way that the value of the variable\nmatters, which is run_argv(), and it already keeps track of the\nfact that it already called handle_alias().\n\nSo we can simplify the whole thing by:\n\n- Change handle_builtin() to always make a direct call to the\n  builtin implementation it finds, and make sure the caller\n  refrains from calling it if handle_alias() has ever been\n  called;\n\n- Remove saved_env_before_alias variable, and instead use the\n  local \"done_alias\" variable maintained inside run_argv() to\n  make the same decision.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex e39b972..1cbe267 100644\n--- a/git.c\n+++ b/git.c\n@@ -25,13 +25,11 @@ static const char *env_names[] = {\n \tGIT_PREFIX_ENVIRONMENT\n };\n static char *orig_env[4];\n-static int saved_env_before_alias;\n static int save_restore_env_balance;\n \n static void save_env_before_alias(void)\n {\n \tint i;\n-\tsaved_env_before_alias = 1;\n \n \tassert(save_restore_env_balance == 0);\n \tsave_restore_env_balance = 1;\n@@ -533,16 +531,8 @@ static void handle_builtin(int argc, const char **argv)\n \t}\n \n \tbuiltin = get_builtin(cmd);\n-\tif (builtin) {\n-\t\t/*\n-\t\t * XXX: if we can figure out cases where it is _safe_\n-\t\t * to do, we can avoid spawning a new process when\n-\t\t * saved_env_before_alias is true\n-\t\t * (i.e. setup_git_dir* has been run once)\n-\t\t */\n-\t\tif (!saved_env_before_alias)\n-\t\t\texit(run_builtin(builtin, argc, argv));\n-\t}\n+\tif (builtin)\n+\t\texit(run_builtin(builtin, argc, argv));\n }\n \n static void execv_dashed_external(const char **argv)\n@@ -586,8 +576,17 @@ static int run_argv(int *argcp, const char ***argv)\n \tint done_alias = 0;\n \n \twhile (1) {\n-\t\t/* See if it's a builtin */\n-\t\thandle_builtin(*argcp, *argv);\n+\t\t/*\n+\t\t * If we tried alias and futzed with our environment,\n+\t\t * it no longer is safe to invoke builtins directly in\n+\t\t * general.  We have to spawn them as dashed externals.\n+\t\t *\n+\t\t * NEEDSWORK: if we can figure out cases\n+\t\t * where it is safe to do, we can avoid spawning a new\n+\t\t * process.\n+\t\t */\n+\t\tif (!done_alias)\n+\t\t\thandle_builtin(*argcp, *argv);\n \n \t\t/* .. then try the external ones */\n \t\texecv_dashed_external(*argv);\n-- \n2.7.0-391-gcd29568\n"},{"id":"277288","messageId":"xmqqio26pt91.fsf_-_@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"xmqqy4ba627n.fsf_-_@gitster.mtv.corp.google.com","subject":"[PATCH v3 4/4] restore_env(): free the saved environment variable once we are done","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-02T23:50:02Z","receivedAt":"2016-02-02T23:50:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Just like we free orig_cwd, which is the value of the original\nworking directory saved in save_env_before_alias(), once we are\ndone with it, the contents of orig_env[] array, saved in the\nsave_env_before_alias() function should be freed; otherwise,\nthe second and subsequent calls to save/restore pair will leak\nthe memory allocated in save_env_before_alias().\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This is new.\n\n git.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 1cbe267..93f569d 100644\n--- a/git.c\n+++ b/git.c\n@@ -54,10 +54,12 @@ static void restore_env(int external_alias)\n \t\tif (external_alias &&\n \t\t    !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))\n \t\t\tcontinue;\n-\t\tif (orig_env[i])\n+\t\tif (orig_env[i]) {\n \t\t\tsetenv(env_names[i], orig_env[i], 1);\n-\t\telse\n+\t\t\tfree(orig_env[i]);\n+\t\t} else {\n \t\t\tunsetenv(env_names[i]);\n+\t\t}\n \t}\n }\n \n-- \n2.7.0-391-gcd29568\n"},{"id":"277292","messageId":"CAPig+cT8PhJU=9aS8NvkN9wx6imtACOS9rkgDJeJpN=CGikd7Q@mail.gmail.com","threadId":"41259","inReplyTo":"xmqqio26pt91.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] restore_env(): free the saved environment variable once we are done","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-03T01:47:14Z","receivedAt":"2016-02-03T01:47:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 2, 2016 at 6:50 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Just like we free orig_cwd, which is the value of the original\n> working directory saved in save_env_before_alias(), once we are\n> done with it, the contents of orig_env[] array, saved in the\n> save_env_before_alias() function should be freed; otherwise,\n> the second and subsequent calls to save/restore pair will leak\n> the memory allocated in save_env_before_alias().\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> diff --git a/git.c b/git.c\n> @@ -54,10 +54,12 @@ static void restore_env(int external_alias)\n>                 if (external_alias &&\n>                     !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))\n>                         continue;\n> -               if (orig_env[i])\n> +               if (orig_env[i]) {\n>                         setenv(env_names[i], orig_env[i], 1);\n> -               else\n> +                       free(orig_env[i]);\n\nNow that this is \"well-protected\"[1] against incorrect nesting, you\ndon't worry about the dangling pointers in static orig_env[], right?\n(The same for the dangling pointer in static 'orig_cwd' after being\nfreed a bit earlier in this function, correct?)\n\n[1]: via assert()\n\n> +               } else {\n>                         unsetenv(env_names[i]);\n> +               }\n>         }\n>  }\n"},{"id":"277293","messageId":"xmqqy4b2o7rf.fsf@gitster.mtv.corp.google.com","threadId":"41259","inReplyTo":"CAPig+cT8PhJU=9aS8NvkN9wx6imtACOS9rkgDJeJpN=CGikd7Q@mail.gmail.com","subject":"Re: [PATCH v3 4/4] restore_env(): free the saved environment variable once we are done","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-03T02:19:32Z","receivedAt":"2016-02-03T02:19:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> diff --git a/git.c b/git.c\n>> @@ -54,10 +54,12 @@ static void restore_env(int external_alias)\n>>                 if (external_alias &&\n>>                     !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))\n>>                         continue;\n>> -               if (orig_env[i])\n>> +               if (orig_env[i]) {\n>>                         setenv(env_names[i], orig_env[i], 1);\n>> -               else\n>> +                       free(orig_env[i]);\n>\n> Now that this is \"well-protected\"[1] against incorrect nesting, you\n> don't worry about the dangling pointers in static orig_env[], right?\n> (The same for the dangling pointer in static 'orig_cwd' after being\n> freed a bit earlier in this function, correct?)\n\nCorrect.\n\nI do not think we follow a style that requires \"after freeing memory\npointed at by a variable, the variable must be assigned NULL\".\nWould it be a good idea?  I do not see a point in it.\n\nThanks.\n"}]}