@matheusbsbs / matheus-bagliano / commits / 75301f9015
Fork 0

Windows: avoid the "dup dance" when spawning a child process

When stdin, stdout, or stderr must be redirected for a child process that on Windows is spawned using one of the spawn() functions of Microsoft's C runtime, then there is no choice other than to 1. make a backup copy of fd 0,1,2 with dup 2. dup2 the redirection source fd into 0,1,2 3. spawn 4. dup2 the backup back into 0,1,2 5. close the backup copy and the redirection source We used this idiom as well -- but we are not using the spawn() functions anymore! Instead, we have our own implementation. We had hardcoded that stdin, stdout, and stderr of the child process were inherited from the parent's fds 0, 1, and 2. But we can actually specify any fd. With this patch, the fds to inherit are passed from start_command()'s WIN32 section to our spawn implementation. This way, we can avoid the backup copies of the fds. The backup copies were a bug waiting to surface: The OS handles underlying the dup()ed fds were inherited by the child process (but were not associated with a file descriptor in the child). Consequently, the file or pipe represented by the OS handle remained open even after the backup copy was closed in the parent process until the child exited. Since our implementation of pipe() creates non-inheritable OS handles, we still dup() file descriptors in start_command() because dup() happens to create inheritable duplicates. (A nice side effect is that the fd cleanup in start_command is the same for Windows and Unix and remains unchanged.) Signed-off-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Johannes Sixt committed Jan 15, 2010 at 21:12 UTC 75301f90159a505f3683a5eba10174928dc30fb1
3 files changed +50 -49
compat/mingw.c
+17 -8
@@ -615,8 +615,8 @@ static int env_compare(const void *a, const void *b)
615 return strcasecmp(*ea, *eb);
616 }
617
618 -static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
619 - int prepend_cmd)
618 +static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env,
619 + int prepend_cmd, int fhin, int fhout, int fherr)
620 {
621 STARTUPINFO si;
622 PROCESS_INFORMATION pi;
@@ -652,9 +652,9 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
652 memset(&si, 0, sizeof(si));
653 si.cb = sizeof(si);
654 si.dwFlags = STARTF_USESTDHANDLES;
655 - si.hStdInput = (HANDLE) _get_osfhandle(0);
656 - si.hStdOutput = (HANDLE) _get_osfhandle(1);
657 - si.hStdError = (HANDLE) _get_osfhandle(2);
655 + si.hStdInput = (HANDLE) _get_osfhandle(fhin);
656 + si.hStdOutput = (HANDLE) _get_osfhandle(fhout);
657 + si.hStdError = (HANDLE) _get_osfhandle(fherr);
658
659 /* concatenate argv, quoting args as we go */
660 strbuf_init(&args, 0);
@@ -709,7 +709,14 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
709 return (pid_t)pi.hProcess;
710 }
711
712 -pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)
712 +static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
713 + int prepend_cmd)
714 +{
715 + return mingw_spawnve_fd(cmd, argv, env, prepend_cmd, 0, 1, 2);
716 +}
717 +
718 +pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,
719 + int fhin, int fhout, int fherr)
720 {
721 pid_t pid;
722 char **path = get_path_split();
@@ -731,13 +738,15 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)
738 pid = -1;
739 }
740 else {
734 - pid = mingw_spawnve(iprog, argv, env, 1);
741 + pid = mingw_spawnve_fd(iprog, argv, env, 1,
742 + fhin, fhout, fherr);
743 free(iprog);
744 }
745 argv[0] = argv0;
746 }
747 else
740 - pid = mingw_spawnve(prog, argv, env, 0);
748 + pid = mingw_spawnve_fd(prog, argv, env, 0,
749 + fhin, fhout, fherr);
750 free(prog);
751 }
752 free_path_split(path);
compat/mingw.h
+2 -1
@@ -220,7 +220,8 @@ int mingw_fstat(int fd, struct stat *buf);
220 int mingw_utime(const char *file_name, const struct utimbuf *times);
221 #define utime mingw_utime
222
223 -pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);
223 +pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,
224 + int fhin, int fhout, int fherr);
225 void mingw_execvp(const char *cmd, char *const *argv);
226 #define execvp mingw_execvp
227
run-command.c
+31 -40
@@ -8,12 +8,14 @@ static inline void close_pair(int fd[2])
8 close(fd[1]);
9 }
10
11 +#ifndef WIN32
12 static inline void dup_devnull(int to)
13 {
14 int fd = open("/dev/null", O_RDWR);
15 dup2(fd, to);
16 close(fd);
17 }
18 +#endif
19
20 int start_command(struct child_process *cmd)
21 {
@@ -135,42 +137,30 @@ fail_pipe:
137 strerror(failed_errno = errno));
138 #else
139 {
138 - int s0 = -1, s1 = -1, s2 = -1; /* backups of stdin, stdout, stderr */
140 + int fhin = 0, fhout = 1, fherr = 2;
141 const char **sargv = cmd->argv;
142 char **env = environ;
143
142 - if (cmd->no_stdin) {
143 - s0 = dup(0);
144 - dup_devnull(0);
145 - } else if (need_in) {
146 - s0 = dup(0);
147 - dup2(fdin[0], 0);
148 - } else if (cmd->in) {
149 - s0 = dup(0);
150 - dup2(cmd->in, 0);
151 - }
152 -
153 - if (cmd->no_stderr) {
154 - s2 = dup(2);
155 - dup_devnull(2);
156 - } else if (need_err) {
157 - s2 = dup(2);
158 - dup2(fderr[1], 2);
159 - }
160 -
161 - if (cmd->no_stdout) {
162 - s1 = dup(1);
163 - dup_devnull(1);
164 - } else if (cmd->stdout_to_stderr) {
165 - s1 = dup(1);
166 - dup2(2, 1);
167 - } else if (need_out) {
168 - s1 = dup(1);
169 - dup2(fdout[1], 1);
170 - } else if (cmd->out > 1) {
171 - s1 = dup(1);
172 - dup2(cmd->out, 1);
173 - }
144 + if (cmd->no_stdin)
145 + fhin = open("/dev/null", O_RDWR);
146 + else if (need_in)
147 + fhin = dup(fdin[0]);
148 + else if (cmd->in)
149 + fhin = dup(cmd->in);
150 +
151 + if (cmd->no_stderr)
152 + fherr = open("/dev/null", O_RDWR);
153 + else if (need_err)
154 + fherr = dup(fderr[1]);
155 +
156 + if (cmd->no_stdout)
157 + fhout = open("/dev/null", O_RDWR);
158 + else if (cmd->stdout_to_stderr)
159 + fhout = dup(fherr);
160 + else if (need_out)
161 + fhout = dup(fdout[1]);
162 + else if (cmd->out > 1)
163 + fhout = dup(cmd->out);
164
165 if (cmd->dir)
166 die("chdir in start_command() not implemented");
@@ -181,7 +171,8 @@ fail_pipe:
171 cmd->argv = prepare_git_cmd(cmd->argv);
172 }
173
184 - cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
174 + cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env,
175 + fhin, fhout, fherr);
176 failed_errno = errno;
177 if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
178 error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
@@ -192,12 +183,12 @@ fail_pipe:
183 free(cmd->argv);
184
185 cmd->argv = sargv;
195 - if (s0 >= 0)
196 - dup2(s0, 0), close(s0);
197 - if (s1 >= 0)
198 - dup2(s1, 1), close(s1);
199 - if (s2 >= 0)
200 - dup2(s2, 2), close(s2);
186 + if (fhin != 0)
187 + close(fhin);
188 + if (fhout != 1)
189 + close(fhout);
190 + if (fherr != 2)
191 + close(fherr);
192 }
193 #endif
194