From 9286ef390f8211d6a3e500117a58c358d843a324 Mon Sep 17 00:00:00 2001 From: Karsten Blees Date: Sun, 15 Jan 2012 00:57:14 +0100 Subject: [PATCH] Win32: don't copy the environment twice when spawning child processes When spawning child processes via start_command(), the environment and all environment entries are copied twice. First by make_augmented_environ / copy_environ to merge with child_process.env. Then a second time by make_environment_block to create a sorted environment block string as required by CreateProcess. Move the merge logic to make_environment_block so that we only need to copy the environment once. This changes semantics of the env parameter: it now expects a delta (such as child_process.env) rather than a full environment. This is not a problem as the parameter is only used by start_command() (all other callers previously passed char **environ, and now pass NULL). The merge logic no longer xstrdup()s the environment strings, so do_putenv must not free them. Add a parameter to distinguish this from normal putenv. Remove the now unused make_augmented_environ / free_environ API. Signed-off-by: Karsten Blees --- compat/mingw.c | 74 +++++++++++++++++++--------------------------------------- compat/mingw.h | 6 ----- run-command.c | 10 ++------ 3 files changed, 26 insertions(+), 64 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index be381573f1..9a0b39a47e 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -759,7 +759,7 @@ static int lookupenv(char **env, const char *name, size_t nmln) /* * If name contains '=', then sets the variable, otherwise it unsets it */ -static char **do_putenv(char **env, const char *name) +static char **do_putenv(char **env, const char *name, int free_old) { char *eq = strchrnul(name, '='); int i = lookupenv(env, name, eq-name); @@ -774,7 +774,8 @@ static char **do_putenv(char **env, const char *name) } } else { - free(env[i]); + if (free_old) + free(env[i]); if (*eq) env[i] = (char*) name; else @@ -803,7 +804,7 @@ char *mingw_getenv(const char *name) int mingw_putenv(const char *namevalue) { - environ = do_putenv(environ, namevalue); + environ = do_putenv(environ, namevalue, 1); return 0; } @@ -990,21 +991,30 @@ static char *path_lookup(const char *cmd, char **path, int exe_only) } /* - * Create environment block suitable for CreateProcess. + * Create environment block suitable for CreateProcess. Merges current + * process environment and the supplied environment changes. */ -static wchar_t *make_environment_block(char **env) +static wchar_t *make_environment_block(char **deltaenv) { wchar_t *wenvblk = NULL; int count = 0; char **e, **tmpenv; int size = 0, wenvsz = 0, wenvpos = 0; - for (e = env; *e; e++) + while (environ[count]) count++; - /* environment must be sorted */ + /* copy the environment */ tmpenv = xmalloc(sizeof(*tmpenv) * (count + 1)); - memcpy(tmpenv, env, sizeof(*tmpenv) * (count + 1)); + memcpy(tmpenv, environ, sizeof(*tmpenv) * (count + 1)); + + /* merge supplied environment changes into the temporary environment */ + for (e = deltaenv; e && *e; e++) + tmpenv = do_putenv(tmpenv, *e, 0); + + /* environment must be sorted */ + for (count = 0; tmpenv[count]; ) + count++; qsort(tmpenv, count, sizeof(*tmpenv), compareenv); /* create environment block from temporary environment */ @@ -1027,7 +1037,7 @@ struct pinfo_t { struct pinfo_t *pinfo = NULL; CRITICAL_SECTION pinfo_cs; -static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env, +static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaenv, const char *dir, int prepend_cmd, int fhin, int fhout, int fherr) { @@ -1095,8 +1105,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env, xutftowcs(wargs, args.buf, 2 * args.len + 1); strbuf_release(&args); - if (env) - wenvblk = make_environment_block(env); + wenvblk = make_environment_block(deltaenv); memset(&pi, 0, sizeof(pi)); ret = CreateProcessW(wcmd, wargs, NULL, NULL, TRUE, flags, @@ -1134,10 +1143,10 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env, static pid_t mingw_spawnv(const char *cmd, const char **argv, int prepend_cmd) { - return mingw_spawnve_fd(cmd, argv, environ, NULL, prepend_cmd, 0, 1, 2); + return mingw_spawnve_fd(cmd, argv, NULL, NULL, prepend_cmd, 0, 1, 2); } -pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env, +pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv, const char *dir, int fhin, int fhout, int fherr) { @@ -1161,14 +1170,14 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env, pid = -1; } else { - pid = mingw_spawnve_fd(iprog, argv, env, dir, 1, + pid = mingw_spawnve_fd(iprog, argv, deltaenv, dir, 1, fhin, fhout, fherr); free(iprog); } argv[0] = argv0; } else - pid = mingw_spawnve_fd(prog, argv, env, dir, 0, + pid = mingw_spawnve_fd(prog, argv, deltaenv, dir, 0, fhin, fhout, fherr); free(prog); } @@ -1257,41 +1266,6 @@ int mingw_kill(pid_t pid, int sig) return -1; } -static char **copy_environ(void) -{ - char **env; - int i = 0; - while (environ[i]) - i++; - env = xmalloc((i+1)*sizeof(*env)); - for (i = 0; environ[i]; i++) - env[i] = xstrdup(environ[i]); - env[i] = NULL; - return env; -} - -void free_environ(char **env) -{ - int i; - for (i = 0; env[i]; i++) - free(env[i]); - free(env); -} - -/* - * Copies global environ and adjusts variables as specified by vars. - */ -char **make_augmented_environ(const char *const *vars) -{ - char **env = copy_environ(); - - while (*vars) { - const char *v = *vars++; - env = do_putenv(env, strchr(v, '=') ? xstrdup(v) : v); - } - return env; -} - /* * Note, this isn't a complete replacement for getaddrinfo. It assumes * that service contains a numerical port, or that it is null. It diff --git a/compat/mingw.h b/compat/mingw.h index ba21474515..04b6523255 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -335,12 +335,6 @@ void mingw_open_html(const char *path); void mingw_mark_as_git_dir(const char *dir); #define mark_as_git_dir mingw_mark_as_git_dir -/* - * helpers - */ - -char **make_augmented_environ(const char *const *vars); -void free_environ(char **env); /** * Converts UTF-8 encoded string to UTF-16LE. diff --git a/run-command.c b/run-command.c index 1db8abf984..6d0dc3da91 100644 --- a/run-command.c +++ b/run-command.c @@ -381,7 +381,6 @@ fail_pipe: { int fhin = 0, fhout = 1, fherr = 2; const char **sargv = cmd->argv; - char **env = environ; if (cmd->no_stdin) fhin = open("/dev/null", O_RDWR); @@ -406,25 +405,20 @@ fail_pipe: else if (cmd->out > 1) fhout = dup(cmd->out); - if (cmd->env) - env = make_augmented_environ(cmd->env); - if (cmd->git_cmd) { cmd->argv = prepare_git_cmd(cmd->argv); } else if (cmd->use_shell) { cmd->argv = prepare_shell_cmd(cmd->argv); } - cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env, cmd->dir, - fhin, fhout, fherr); + cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, (char**) cmd->env, + cmd->dir, fhin, fhout, fherr); failed_errno = errno; if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT)) error("cannot spawn %s: %s", cmd->argv[0], strerror(errno)); if (cmd->clean_on_exit && cmd->pid >= 0) mark_child_for_cleanup(cmd->pid); - if (cmd->env) - free_environ(env); if (cmd->git_cmd) free(cmd->argv); -- 2.11.4.GIT