From 9d1d2b7fad9bec6320a2058c625787c835864960 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 26 Jan 2016 11:46:53 -0800 Subject: [PATCH 1/4] git: remove an early return from save_env_before_alias() When help.autocorrect is in effect, an attempt to auto-execute an uniquely corrected result of a misspelt alias will result in an irrelevant error message. The codepath that causes this calls save_env_before_alias() and restore_env() in handle_alias(), and that happens twice. A global variable orig_cwd is allocated to hold the return value of getcwd() in save_env_before_alias(), which is then used in restore_env() to go back to that directory and finally free(3)'d there. However, save_env_before_alias() is not prepared to be called twice. It returns early when it knows it has already been called, leaving orig_cwd undefined, which is then checked in the second call to restore_env(), and by that time, the memory that used to hold the contents of orig_cwd is either freed or reused to hold something else, and this is fed to chdir(2), causing it to fail. Even if it did not fail (i.e. reading of the already free'd piece of memory yielded a directory path that we can chdir(2) to), it then gets free(3)'d. Fix this by making sure save_env() does do the saving when called. While at it, add a minimal test for help.autocorrect facility. Signed-off-by: Junio C Hamano --- git.c | 2 -- t/t9003-help-autocorrect.sh | 52 +++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) create mode 100755 t/t9003-help-autocorrect.sh diff --git a/git.c b/git.c index 98d441220..a57a4cb46 100644 --- a/git.c +++ b/git.c @@ -30,8 +30,6 @@ static int saved_env_before_alias; static void save_env_before_alias(void) { int i; - if (saved_env_before_alias) - return; saved_env_before_alias = 1; orig_cwd = xgetcwd(); for (i = 0; i < ARRAY_SIZE(env_names); i++) { diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh new file mode 100755 index 000000000..dfe95c923 --- /dev/null +++ b/t/t9003-help-autocorrect.sh @@ -0,0 +1,52 @@ +#!/bin/sh + +test_description='help.autocorrect finding a match' +. ./test-lib.sh + +test_expect_success 'setup' ' + # An alias + git config alias.lgf "log --format=%s --first-parent" && + + # A random user-defined command + write_script git-distimdistim <<-EOF && + echo distimdistim was called + EOF + + PATH="$PATH:." && + export PATH && + + git commit --allow-empty -m "a single log entry" && + + # Sanity check + git lgf >actual && + echo "a single log entry" >expect && + test_cmp expect actual && + + git distimdistim >actual && + echo "distimdistim was called" >expect && + test_cmp expect actual +' + +test_expect_success 'autocorrect showing candidates' ' + git config help.autocorrect 0 && + + test_must_fail git lfg 2>actual && + sed -e "1,/^Did you mean this/d" actual | grep lgf && + + test_must_fail git distimdist 2>actual && + sed -e "1,/^Did you mean this/d" actual | grep distimdistim +' + +test_expect_success 'autocorrect running commands' ' + git config help.autocorrect -1 && + + git lfg >actual && + echo "a single log entry" >expect && + test_cmp expect actual && + + git distimdist >actual && + echo "distimdistim was called" >expect && + test_cmp expect actual +' + +test_done From 2e1175d43d05e83fe836e1c8c8e7c25b7ee659ae Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 26 Jan 2016 22:50:27 -0800 Subject: [PATCH 2/4] git: protect against unbalanced calls to {save,restore}_env() We made sure that save_env_before_alias() does not skip saving the environment when asked to (which led to use-after-free of orig_cwd in restore_env() in the buggy version) with the previous step. Protect against future breakage where somebody adds new callers of these functions in an unbalanced fashion. Signed-off-by: Junio C Hamano --- git.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/git.c b/git.c index a57a4cb46..e39b97235 100644 --- a/git.c +++ b/git.c @@ -26,11 +26,15 @@ static const char *env_names[] = { }; static char *orig_env[4]; static int saved_env_before_alias; +static int save_restore_env_balance; static void save_env_before_alias(void) { int i; saved_env_before_alias = 1; + + assert(save_restore_env_balance == 0); + save_restore_env_balance = 1; orig_cwd = xgetcwd(); for (i = 0; i < ARRAY_SIZE(env_names); i++) { orig_env[i] = getenv(env_names[i]); @@ -42,6 +46,9 @@ static void save_env_before_alias(void) static void restore_env(int external_alias) { int i; + + assert(save_restore_env_balance == 1); + save_restore_env_balance = 0; if (!external_alias && orig_cwd && chdir(orig_cwd)) die_errno("could not move to %s", orig_cwd); free(orig_cwd); From 441981bc85ea2b648d7ffb2515b371071208e657 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 26 Jan 2016 22:52:02 -0800 Subject: [PATCH 3/4] git: simplify environment save/restore logic The only code that cares about the value of the global variable saved_env_before_alias after the previous fix is handle_builtin() that turns into a glorified no-op when the variable is true, so the logic could safely be lifted to its caller, i.e. the caller can refrain from calling it when the variable is set. This variable tells us if save_env_before_alias() was called (with or without matching restore_env()), but the sole caller of the function, handle_alias(), always calls it as the first thing, so we can consider that the variable essentially keeps track of the fact that handle_alias() has ever been called. It turns out that handle_builtin() and handle_alias() are called only from one function in a way that the value of the variable matters, which is run_argv(), and it already keeps track of the fact that it already called handle_alias(). So we can simplify the whole thing by: - Change handle_builtin() to always make a direct call to the builtin implementation it finds, and make sure the caller refrains from calling it if handle_alias() has ever been called; - Remove saved_env_before_alias variable, and instead use the local "done_alias" variable maintained inside run_argv() to make the same decision. Signed-off-by: Junio C Hamano --- git.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/git.c b/git.c index e39b97235..1cbe2676f 100644 --- a/git.c +++ b/git.c @@ -25,13 +25,11 @@ static const char *env_names[] = { GIT_PREFIX_ENVIRONMENT }; static char *orig_env[4]; -static int saved_env_before_alias; static int save_restore_env_balance; static void save_env_before_alias(void) { int i; - saved_env_before_alias = 1; assert(save_restore_env_balance == 0); save_restore_env_balance = 1; @@ -533,16 +531,8 @@ static void handle_builtin(int argc, const char **argv) } builtin = get_builtin(cmd); - if (builtin) { - /* - * XXX: if we can figure out cases where it is _safe_ - * to do, we can avoid spawning a new process when - * saved_env_before_alias is true - * (i.e. setup_git_dir* has been run once) - */ - if (!saved_env_before_alias) - exit(run_builtin(builtin, argc, argv)); - } + if (builtin) + exit(run_builtin(builtin, argc, argv)); } static void execv_dashed_external(const char **argv) @@ -586,8 +576,17 @@ static int run_argv(int *argcp, const char ***argv) int done_alias = 0; while (1) { - /* See if it's a builtin */ - handle_builtin(*argcp, *argv); + /* + * If we tried alias and futzed with our environment, + * it no longer is safe to invoke builtins directly in + * general. We have to spawn them as dashed externals. + * + * NEEDSWORK: if we can figure out cases + * where it is safe to do, we can avoid spawning a new + * process. + */ + if (!done_alias) + handle_builtin(*argcp, *argv); /* .. then try the external ones */ execv_dashed_external(*argv); From 8384c139cb9409fb3cf5ef70afff263917581258 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 2 Feb 2016 15:42:59 -0800 Subject: [PATCH 4/4] restore_env(): free the saved environment variable once we are done Just like we free orig_cwd, which is the value of the original working directory saved in save_env_before_alias(), once we are done with it, the contents of orig_env[] array, saved in the save_env_before_alias() function should be freed; otherwise, the second and subsequent calls to save/restore pair will leak the memory allocated in save_env_before_alias(). Signed-off-by: Junio C Hamano --- git.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/git.c b/git.c index 1cbe2676f..93f569d06 100644 --- a/git.c +++ b/git.c @@ -54,10 +54,12 @@ static void restore_env(int external_alias) if (external_alias && !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT)) continue; - if (orig_env[i]) + if (orig_env[i]) { setenv(env_names[i], orig_env[i], 1); - else + free(orig_env[i]); + } else { unsetenv(env_names[i]); + } } }