Skip to content

Commit 75301f9

Browse files
j6tgitster
authored andcommitted
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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 3e34d66 commit 75301f9

File tree

3 files changed

+50
-49
lines changed

3 files changed

+50
-49
lines changed

compat/mingw.c

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -615,8 +615,8 @@ static int env_compare(const void *a, const void *b)
615615
return strcasecmp(*ea, *eb);
616616
}
617617

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)
620620
{
621621
STARTUPINFO si;
622622
PROCESS_INFORMATION pi;
@@ -652,9 +652,9 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
652652
memset(&si, 0, sizeof(si));
653653
si.cb = sizeof(si);
654654
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);
658658

659659
/* concatenate argv, quoting args as we go */
660660
strbuf_init(&args, 0);
@@ -709,7 +709,14 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
709709
return (pid_t)pi.hProcess;
710710
}
711711

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)
713720
{
714721
pid_t pid;
715722
char **path = get_path_split();
@@ -731,13 +738,15 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)
731738
pid = -1;
732739
}
733740
else {
734-
pid = mingw_spawnve(iprog, argv, env, 1);
741+
pid = mingw_spawnve_fd(iprog, argv, env, 1,
742+
fhin, fhout, fherr);
735743
free(iprog);
736744
}
737745
argv[0] = argv0;
738746
}
739747
else
740-
pid = mingw_spawnve(prog, argv, env, 0);
748+
pid = mingw_spawnve_fd(prog, argv, env, 0,
749+
fhin, fhout, fherr);
741750
free(prog);
742751
}
743752
free_path_split(path);

compat/mingw.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,8 @@ int mingw_fstat(int fd, struct stat *buf);
220220
int mingw_utime(const char *file_name, const struct utimbuf *times);
221221
#define utime mingw_utime
222222

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);
224225
void mingw_execvp(const char *cmd, char *const *argv);
225226
#define execvp mingw_execvp
226227

run-command.c

Lines changed: 31 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,14 @@ static inline void close_pair(int fd[2])
88
close(fd[1]);
99
}
1010

11+
#ifndef WIN32
1112
static inline void dup_devnull(int to)
1213
{
1314
int fd = open("/dev/null", O_RDWR);
1415
dup2(fd, to);
1516
close(fd);
1617
}
18+
#endif
1719

1820
int start_command(struct child_process *cmd)
1921
{
@@ -135,42 +137,30 @@ int start_command(struct child_process *cmd)
135137
strerror(failed_errno = errno));
136138
#else
137139
{
138-
int s0 = -1, s1 = -1, s2 = -1; /* backups of stdin, stdout, stderr */
140+
int fhin = 0, fhout = 1, fherr = 2;
139141
const char **sargv = cmd->argv;
140142
char **env = environ;
141143

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);
174164

175165
if (cmd->dir)
176166
die("chdir in start_command() not implemented");
@@ -181,7 +171,8 @@ int start_command(struct child_process *cmd)
181171
cmd->argv = prepare_git_cmd(cmd->argv);
182172
}
183173

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);
185176
failed_errno = errno;
186177
if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
187178
error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
@@ -192,12 +183,12 @@ int start_command(struct child_process *cmd)
192183
free(cmd->argv);
193184

194185
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);
201192
}
202193
#endif
203194

0 commit comments

Comments
 (0)