Skip to content

Commit 55d607d

Browse files
gitstergitster
committed
Merge branch 'js/mingw-inherit-only-std-handles'
Work around a issue where a FD that is left open when spawning a child process and is kept open in the child can interfere with the operation in the parent process on Windows. * js/mingw-inherit-only-std-handles: mingw: forbid translating ERROR_SUCCESS to an errno value mingw: do set `errno` correctly when trying to restrict handle inheritance mingw: restrict file handle inheritance only on Windows 7 and later mingw: spawned processes need to inherit only standard handles mingw: work around incorrect standard handles mingw: demonstrate that all file handles are inherited by child processes
2 parents 7c88714 + 3ba3720 commit 55d607d

File tree

5 files changed

+199
-12
lines changed

5 files changed

+199
-12
lines changed

‎Documentation/config/core.txt‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -559,6 +559,12 @@ core.unsetenvvars::
559559
Defaults to `PERL5LIB` to account for the fact that Git for
560560
Windows insists on using its own Perl interpreter.
561561

562+
core.restrictinheritedhandles::
563+
Windows-only: override whether spawned processes inherit only standard
564+
file handles (`stdin`, `stdout` and `stderr`) or all handles. Can be
565+
`auto`, `true` or `false`. Defaults to `auto`, which means `true` on
566+
Windows 7 and later, and `false` on older Windows versions.
567+
562568
core.createObject::
563569
You can set this to 'link', in which case a hardlink followed by
564570
a delete of the source are used to make sure that object creation

‎compat/mingw.c‎

Lines changed: 134 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ int err_win_to_posix(DWORD winerr)
114114
case ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;
115115
case ERROR_SHARING_VIOLATION: error = EACCES; break;
116116
case ERROR_STACK_OVERFLOW: error = ENOMEM; break;
117+
case ERROR_SUCCESS: BUG("err_win_to_posix() called without an error!");
117118
case ERROR_SWAPERROR: error = ENOENT; break;
118119
case ERROR_TOO_MANY_MODULES: error = EMFILE; break;
119120
case ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;
@@ -212,6 +213,7 @@ enum hide_dotfiles_type {
212213
HIDE_DOTFILES_DOTGITONLY
213214
};
214215

216+
static int core_restrict_inherited_handles = -1;
215217
static enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;
216218
static char *unset_environment_variables;
217219

@@ -231,6 +233,15 @@ int mingw_core_config(const char *var, const char *value, void *cb)
231233
return 0;
232234
}
233235

236+
if (!strcmp(var, "core.restrictinheritedhandles")) {
237+
if (value && !strcasecmp(value, "auto"))
238+
core_restrict_inherited_handles = -1;
239+
else
240+
core_restrict_inherited_handles =
241+
git_config_bool(var, value);
242+
return 0;
243+
}
244+
234245
return 0;
235246
}
236247

@@ -1436,8 +1447,13 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
14361447
const char *dir,
14371448
int prepend_cmd, int fhin, int fhout, int fherr)
14381449
{
1439-
STARTUPINFOW si;
1450+
static int restrict_handle_inheritance = -1;
1451+
STARTUPINFOEXW si;
14401452
PROCESS_INFORMATION pi;
1453+
LPPROC_THREAD_ATTRIBUTE_LIST attr_list = NULL;
1454+
HANDLE stdhandles[3];
1455+
DWORD stdhandles_count = 0;
1456+
SIZE_T size;
14411457
struct strbuf args;
14421458
wchar_t wcmd[MAX_PATH], wdir[MAX_PATH], *wargs, *wenvblk = NULL;
14431459
unsigned flags = CREATE_UNICODE_ENVIRONMENT;
@@ -1447,6 +1463,19 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
14471463
is_msys2_sh(cmd ? cmd : *argv) ?
14481464
quote_arg_msys2 : quote_arg_msvc;
14491465

1466+
/* Make sure to override previous errors, if any */
1467+
errno = 0;
1468+
1469+
if (restrict_handle_inheritance < 0)
1470+
restrict_handle_inheritance = core_restrict_inherited_handles;
1471+
/*
1472+
* The following code to restrict which handles are inherited seems
1473+
* to work properly only on Windows 7 and later, so let's disable it
1474+
* on Windows Vista and 2008.
1475+
*/
1476+
if (restrict_handle_inheritance < 0)
1477+
restrict_handle_inheritance = GetVersion() >> 16 >= 7601;
1478+
14501479
do_unset_environment_variables();
14511480

14521481
/* Determine whether or not we are associated to a console */
@@ -1474,11 +1503,23 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
14741503
CloseHandle(cons);
14751504
}
14761505
memset(&si, 0, sizeof(si));
1477-
si.cb = sizeof(si);
1478-
si.dwFlags = STARTF_USESTDHANDLES;
1479-
si.hStdInput = winansi_get_osfhandle(fhin);
1480-
si.hStdOutput = winansi_get_osfhandle(fhout);
1481-
si.hStdError = winansi_get_osfhandle(fherr);
1506+
si.StartupInfo.cb = sizeof(si);
1507+
si.StartupInfo.hStdInput = winansi_get_osfhandle(fhin);
1508+
si.StartupInfo.hStdOutput = winansi_get_osfhandle(fhout);
1509+
si.StartupInfo.hStdError = winansi_get_osfhandle(fherr);
1510+
1511+
/* The list of handles cannot contain duplicates */
1512+
if (si.StartupInfo.hStdInput != INVALID_HANDLE_VALUE)
1513+
stdhandles[stdhandles_count++] = si.StartupInfo.hStdInput;
1514+
if (si.StartupInfo.hStdOutput != INVALID_HANDLE_VALUE &&
1515+
si.StartupInfo.hStdOutput != si.StartupInfo.hStdInput)
1516+
stdhandles[stdhandles_count++] = si.StartupInfo.hStdOutput;
1517+
if (si.StartupInfo.hStdError != INVALID_HANDLE_VALUE &&
1518+
si.StartupInfo.hStdError != si.StartupInfo.hStdInput &&
1519+
si.StartupInfo.hStdError != si.StartupInfo.hStdOutput)
1520+
stdhandles[stdhandles_count++] = si.StartupInfo.hStdError;
1521+
if (stdhandles_count)
1522+
si.StartupInfo.dwFlags |= STARTF_USESTDHANDLES;
14821523

14831524
if (*argv && !strcmp(cmd, *argv))
14841525
wcmd[0] = L'\0';
@@ -1511,16 +1552,98 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
15111552
wenvblk = make_environment_block(deltaenv);
15121553

15131554
memset(&pi, 0, sizeof(pi));
1514-
ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL, TRUE,
1515-
flags, wenvblk, dir ? wdir : NULL, &si, &pi);
1555+
if (restrict_handle_inheritance && stdhandles_count &&
1556+
(InitializeProcThreadAttributeList(NULL, 1, 0, &size) ||
1557+
GetLastError() == ERROR_INSUFFICIENT_BUFFER) &&
1558+
(attr_list = (LPPROC_THREAD_ATTRIBUTE_LIST)
1559+
(HeapAlloc(GetProcessHeap(), 0, size))) &&
1560+
InitializeProcThreadAttributeList(attr_list, 1, 0, &size) &&
1561+
UpdateProcThreadAttribute(attr_list, 0,
1562+
PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
1563+
stdhandles,
1564+
stdhandles_count * sizeof(HANDLE),
1565+
NULL, NULL)) {
1566+
si.lpAttributeList = attr_list;
1567+
flags |= EXTENDED_STARTUPINFO_PRESENT;
1568+
}
1569+
1570+
ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,
1571+
stdhandles_count ? TRUE : FALSE,
1572+
flags, wenvblk, dir ? wdir : NULL,
1573+
&si.StartupInfo, &pi);
1574+
1575+
/*
1576+
* On Windows 2008 R2, it seems that specifying certain types of handles
1577+
* (such as FILE_TYPE_CHAR or FILE_TYPE_PIPE) will always produce an
1578+
* error. Rather than playing finicky and fragile games, let's just try
1579+
* to detect this situation and simply try again without restricting any
1580+
* handle inheritance. This is still better than failing to create
1581+
* processes.
1582+
*/
1583+
if (!ret && restrict_handle_inheritance && stdhandles_count) {
1584+
DWORD err = GetLastError();
1585+
struct strbuf buf = STRBUF_INIT;
1586+
1587+
if (err != ERROR_NO_SYSTEM_RESOURCES &&
1588+
/*
1589+
* On Windows 7 and earlier, handles on pipes and character
1590+
* devices are inherited automatically, and cannot be
1591+
* specified in the thread handle list. Rather than trying
1592+
* to catch each and every corner case (and running the
1593+
* chance of *still* forgetting a few), let's just fall
1594+
* back to creating the process without trying to limit the
1595+
* handle inheritance.
1596+
*/
1597+
!(err == ERROR_INVALID_PARAMETER &&
1598+
GetVersion() >> 16 < 9200) &&
1599+
!getenv("SUPPRESS_HANDLE_INHERITANCE_WARNING")) {
1600+
DWORD fl = 0;
1601+
int i;
1602+
1603+
setenv("SUPPRESS_HANDLE_INHERITANCE_WARNING", "1", 1);
1604+
1605+
for (i = 0; i < stdhandles_count; i++) {
1606+
HANDLE h = stdhandles[i];
1607+
strbuf_addf(&buf, "handle #%d: %p (type %lx, "
1608+
"handle info (%d) %lx\n", i, h,
1609+
GetFileType(h),
1610+
GetHandleInformation(h, &fl),
1611+
fl);
1612+
}
1613+
strbuf_addstr(&buf, "\nThis is a bug; please report it "
1614+
"at\nhttps://github.com/git-for-windows/"
1615+
"git/issues/new\n\n"
1616+
"To suppress this warning, please set "
1617+
"the environment variable\n\n"
1618+
"\tSUPPRESS_HANDLE_INHERITANCE_WARNING=1"
1619+
"\n");
1620+
}
1621+
restrict_handle_inheritance = 0;
1622+
flags &= ~EXTENDED_STARTUPINFO_PRESENT;
1623+
ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,
1624+
TRUE, flags, wenvblk, dir ? wdir : NULL,
1625+
&si.StartupInfo, &pi);
1626+
if (!ret)
1627+
errno = err_win_to_posix(GetLastError());
1628+
if (ret && buf.len) {
1629+
warning("failed to restrict file handles (%ld)\n\n%s",
1630+
err, buf.buf);
1631+
}
1632+
strbuf_release(&buf);
1633+
} else if (!ret)
1634+
errno = err_win_to_posix(GetLastError());
1635+
1636+
if (si.lpAttributeList)
1637+
DeleteProcThreadAttributeList(si.lpAttributeList);
1638+
if (attr_list)
1639+
HeapFree(GetProcessHeap(), 0, attr_list);
15161640

15171641
free(wenvblk);
15181642
free(wargs);
15191643

1520-
if (!ret) {
1521-
errno = ENOENT;
1644+
if (!ret)
15221645
return -1;
1523-
}
1646+
15241647
CloseHandle(pi.hThread);
15251648

15261649
/*

‎compat/winansi.c‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -662,10 +662,20 @@ void winansi_init(void)
662662
*/
663663
HANDLE winansi_get_osfhandle(int fd)
664664
{
665+
HANDLE ret;
666+
665667
if (fd == 1 && (fd_is_interactive[1] & FD_SWAPPED))
666668
return hconsole1;
667669
if (fd == 2 && (fd_is_interactive[2] & FD_SWAPPED))
668670
return hconsole2;
669671

670-
return (HANDLE)_get_osfhandle(fd);
672+
ret = (HANDLE)_get_osfhandle(fd);
673+
674+
/*
675+
* There are obviously circumstances under which _get_osfhandle()
676+
* returns (HANDLE)-2. This is not documented anywhere, but that is so
677+
* clearly an invalid handle value that we can just work around this
678+
* and return the correct value for invalid handles.
679+
*/
680+
return ret == (HANDLE)-2 ? INVALID_HANDLE_VALUE : ret;
671681
}

‎t/helper/test-run-command.c‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,13 +328,57 @@ static int quote_echo(int argc, const char **argv)
328328
return 0;
329329
}
330330

331+
static int inherit_handle(const char *argv0)
332+
{
333+
struct child_process cp = CHILD_PROCESS_INIT;
334+
char path[PATH_MAX];
335+
int tmp;
336+
337+
/* First, open an inheritable handle */
338+
xsnprintf(path, sizeof(path), "out-XXXXXX");
339+
tmp = xmkstemp(path);
340+
341+
argv_array_pushl(&cp.args,
342+
"test-tool", argv0, "inherited-handle-child", NULL);
343+
cp.in = -1;
344+
cp.no_stdout = cp.no_stderr = 1;
345+
if (start_command(&cp) < 0)
346+
die("Could not start child process");
347+
348+
/* Then close it, and try to delete it. */
349+
close(tmp);
350+
if (unlink(path))
351+
die("Could not delete '%s'", path);
352+
353+
if (close(cp.in) < 0 || finish_command(&cp) < 0)
354+
die("Child did not finish");
355+
356+
return 0;
357+
}
358+
359+
static int inherit_handle_child(void)
360+
{
361+
struct strbuf buf = STRBUF_INIT;
362+
363+
if (strbuf_read(&buf, 0, 0) < 0)
364+
die("Could not read stdin");
365+
printf("Received %s\n", buf.buf);
366+
strbuf_release(&buf);
367+
368+
return 0;
369+
}
370+
331371
int cmd__run_command(int argc, const char **argv)
332372
{
333373
struct child_process proc = CHILD_PROCESS_INIT;
334374
int jobs;
335375

336376
if (argc > 1 && !strcmp(argv[1], "testsuite"))
337377
exit(testsuite(argc - 1, argv + 1));
378+
if (!strcmp(argv[1], "inherited-handle"))
379+
exit(inherit_handle(argv[0]));
380+
if (!strcmp(argv[1], "inherited-handle-child"))
381+
exit(inherit_handle_child());
338382

339383
if (argc >= 2 && !strcmp(argv[1], "quote-stress-test"))
340384
return !!quote_stress_test(argc - 1, argv + 1);

‎t/t0061-run-command.sh‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ cat >hello-script <<-EOF
1212
cat hello-script
1313
EOF
1414

15+
test_expect_success MINGW 'subprocess inherits only std handles' '
16+
test-tool run-command inherited-handle
17+
'
18+
1519
test_expect_success 'start_command reports ENOENT (slash)' '
1620
test-tool run-command start-command-ENOENT ./does-not-exist 2>err &&
1721
test_i18ngrep "\./does-not-exist" err

0 commit comments

Comments
 (0)