bpo-19764: Implemented support for subprocess.Popen(close_fds=True) on Windows (#1218)

Even though Python marks any handles it opens as non-inheritable there
is still a race when using `subprocess.Popen` since creating a process
with redirected stdio requires temporarily creating inheritable handles.
By implementing support for `subprocess.Popen(close_fds=True)` we fix
this race.

In order to implement this we use PROC_THREAD_ATTRIBUTE_HANDLE_LIST
which is available since Windows Vista. Which allows to pass an explicit
list of handles to inherit when creating a process.

This commit also adds `STARTUPINFO.lpAttributeList["handle_list"]`
which can be used to control PROC_THREAD_ATTRIBUTE_HANDLE_LIST
directly.
This commit is contained in:
Segev Finer 2017-12-18 11:28:19 +02:00 committed by Victor Stinner
parent 87010e85cb
commit b2a6083eb0
8 changed files with 425 additions and 54 deletions

View File

@ -452,17 +452,20 @@ functions.
common use of *preexec_fn* to call os.setsid() in the child.
If *close_fds* is true, all file descriptors except :const:`0`, :const:`1` and
:const:`2` will be closed before the child process is executed. (POSIX only).
The default varies by platform: Always true on POSIX. On Windows it is
true when *stdin*/*stdout*/*stderr* are :const:`None`, false otherwise.
:const:`2` will be closed before the child process is executed.
On Windows, if *close_fds* is true then no handles will be inherited by the
child process. Note that on Windows, you cannot set *close_fds* to true and
also redirect the standard handles by setting *stdin*, *stdout* or *stderr*.
child process unless explicitly passed in the ``handle_list`` element of
:attr:`STARTUPINFO.lpAttributeList`, or by standard handle redirection.
.. versionchanged:: 3.2
The default for *close_fds* was changed from :const:`False` to
what is described above.
.. versionchanged:: 3.7
On Windows the default for *close_fds* was changed from :const:`False` to
:const:`True` when redirecting the standard handles. It's now possible to
set *close_fds* to :const:`True` when redirecting the standard handles.
*pass_fds* is an optional sequence of file descriptors to keep open
between the parent and child. Providing any *pass_fds* forces
*close_fds* to be :const:`True`. (POSIX only)
@ -764,7 +767,7 @@ The :class:`STARTUPINFO` class and following constants are only available
on Windows.
.. class:: STARTUPINFO(*, dwFlags=0, hStdInput=None, hStdOutput=None, \
hStdError=None, wShowWindow=0)
hStdError=None, wShowWindow=0, lpAttributeList=None)
Partial support of the Windows
`STARTUPINFO <https://msdn.microsoft.com/en-us/library/ms686331(v=vs.85).aspx>`__
@ -814,6 +817,33 @@ on Windows.
:data:`SW_HIDE` is provided for this attribute. It is used when
:class:`Popen` is called with ``shell=True``.
.. attribute:: lpAttributeList
A dictionary of additional attributes for process creation as given in
``STARTUPINFOEX``, see
`UpdateProcThreadAttribute <https://msdn.microsoft.com/en-us/library/windows/desktop/ms686880(v=vs.85).aspx>`__.
Supported attributes:
**handle_list**
Sequence of handles that will be inherited. *close_fds* must be true if
non-empty.
The handles must be temporarily made inheritable by
:func:`os.set_handle_inheritable` when passed to the :class:`Popen`
constructor, else :class:`OSError` will be raised with Windows error
``ERROR_INVALID_PARAMETER`` (87).
.. warning::
In a multithreaded process, use caution to avoid leaking handles
that are marked inheritable when combining this feature with
concurrent calls to other process creation functions that inherit
all handles such as :func:`os.system`. This also applies to
standard handle redirection, which temporarily creates inheritable
handles.
.. versionadded:: 3.7
Windows Constants
^^^^^^^^^^^^^^^^^

View File

@ -437,6 +437,17 @@ string
expression pattern for braced placeholders and non-braced placeholders
separately. (Contributed by Barry Warsaw in :issue:`1198569`.)
subprocess
----------
On Windows the default for *close_fds* was changed from :const:`False` to
:const:`True` when redirecting the standard handles. It's now possible to set
*close_fds* to :const:`True` when redirecting the standard handles. See
:class:`subprocess.Popen`.
This means that *close_fds* now defaults to :const:`True` on all supported
platforms.
sys
---
@ -883,6 +894,14 @@ Changes in the Python API
.. _Unicode Technical Standard #18: https://unicode.org/reports/tr18/
* On Windows the default for the *close_fds* argument of
:class:`subprocess.Popen` was changed from :const:`False` to :const:`True`
when redirecting the standard handles. If you previously depended on handles
being inherited when using :class:`subprocess.Popen` with standard io
redirection, you will have to pass ``close_fds=False`` to preserve the
previous behaviour, or use
:attr:`STARTUPINFO.lpAttributeList <subprocess.STARTUPINFO.lpAttributeList>`.
Changes in the C API
--------------------

View File

@ -128,12 +128,13 @@ if _mswindows:
import _winapi
class STARTUPINFO:
def __init__(self, *, dwFlags=0, hStdInput=None, hStdOutput=None,
hStdError=None, wShowWindow=0):
hStdError=None, wShowWindow=0, lpAttributeList=None):
self.dwFlags = dwFlags
self.hStdInput = hStdInput
self.hStdOutput = hStdOutput
self.hStdError = hStdError
self.wShowWindow = wShowWindow
self.lpAttributeList = lpAttributeList or {"handle_list": []}
else:
import _posixsubprocess
import select
@ -577,9 +578,6 @@ def getoutput(cmd):
return getstatusoutput(cmd)[1]
_PLATFORM_DEFAULT_CLOSE_FDS = object()
class Popen(object):
""" Execute a child program in a new process.
@ -630,7 +628,7 @@ class Popen(object):
def __init__(self, args, bufsize=-1, executable=None,
stdin=None, stdout=None, stderr=None,
preexec_fn=None, close_fds=_PLATFORM_DEFAULT_CLOSE_FDS,
preexec_fn=None, close_fds=True,
shell=False, cwd=None, env=None, universal_newlines=None,
startupinfo=None, creationflags=0,
restore_signals=True, start_new_session=False,
@ -655,21 +653,8 @@ class Popen(object):
if preexec_fn is not None:
raise ValueError("preexec_fn is not supported on Windows "
"platforms")
any_stdio_set = (stdin is not None or stdout is not None or
stderr is not None)
if close_fds is _PLATFORM_DEFAULT_CLOSE_FDS:
if any_stdio_set:
close_fds = False
else:
close_fds = True
elif close_fds and any_stdio_set:
raise ValueError(
"close_fds is not supported on Windows platforms"
" if you redirect stdin/stdout/stderr")
else:
# POSIX
if close_fds is _PLATFORM_DEFAULT_CLOSE_FDS:
close_fds = True
if pass_fds and not close_fds:
warnings.warn("pass_fds overriding close_fds.", RuntimeWarning)
close_fds = True
@ -1019,6 +1004,19 @@ class Popen(object):
return Handle(h)
def _filter_handle_list(self, handle_list):
"""Filter out console handles that can't be used
in lpAttributeList["handle_list"] and make sure the list
isn't empty. This also removes duplicate handles."""
# An handle with it's lowest two bits set might be a special console
# handle that if passed in lpAttributeList["handle_list"], will
# cause it to fail.
return list({handle for handle in handle_list
if handle & 0x3 != 0x3
or _winapi.GetFileType(handle) !=
_winapi.FILE_TYPE_CHAR})
def _execute_child(self, args, executable, preexec_fn, close_fds,
pass_fds, cwd, env,
startupinfo, creationflags, shell,
@ -1036,12 +1034,41 @@ class Popen(object):
# Process startup details
if startupinfo is None:
startupinfo = STARTUPINFO()
if -1 not in (p2cread, c2pwrite, errwrite):
use_std_handles = -1 not in (p2cread, c2pwrite, errwrite)
if use_std_handles:
startupinfo.dwFlags |= _winapi.STARTF_USESTDHANDLES
startupinfo.hStdInput = p2cread
startupinfo.hStdOutput = c2pwrite
startupinfo.hStdError = errwrite
attribute_list = startupinfo.lpAttributeList
have_handle_list = bool(attribute_list and
"handle_list" in attribute_list and
attribute_list["handle_list"])
# If we were given an handle_list or need to create one
if have_handle_list or (use_std_handles and close_fds):
if attribute_list is None:
attribute_list = startupinfo.lpAttributeList = {}
handle_list = attribute_list["handle_list"] = \
list(attribute_list.get("handle_list", []))
if use_std_handles:
handle_list += [int(p2cread), int(c2pwrite), int(errwrite)]
handle_list[:] = self._filter_handle_list(handle_list)
if handle_list:
if not close_fds:
warnings.warn("startupinfo.lpAttributeList['handle_list'] "
"overriding close_fds", RuntimeWarning)
# When using the handle_list we always request to inherit
# handles but the only handles that will be inherited are
# the ones in the handle_list
close_fds = False
if shell:
startupinfo.dwFlags |= _winapi.STARTF_USESHOWWINDOW
startupinfo.wShowWindow = _winapi.SW_HIDE

View File

@ -2743,11 +2743,6 @@ class Win32ProcessTestCase(BaseTestCase):
[sys.executable, "-c",
"import sys; sys.exit(47)"],
preexec_fn=lambda: 1)
self.assertRaises(ValueError, subprocess.call,
[sys.executable, "-c",
"import sys; sys.exit(47)"],
stdout=subprocess.PIPE,
close_fds=True)
@support.cpython_only
def test_issue31471(self):
@ -2765,6 +2760,67 @@ class Win32ProcessTestCase(BaseTestCase):
close_fds=True)
self.assertEqual(rc, 47)
def test_close_fds_with_stdio(self):
import msvcrt
fds = os.pipe()
self.addCleanup(os.close, fds[0])
self.addCleanup(os.close, fds[1])
handles = []
for fd in fds:
os.set_inheritable(fd, True)
handles.append(msvcrt.get_osfhandle(fd))
p = subprocess.Popen([sys.executable, "-c",
"import msvcrt; print(msvcrt.open_osfhandle({}, 0))".format(handles[0])],
stdout=subprocess.PIPE, close_fds=False)
stdout, stderr = p.communicate()
self.assertEqual(p.returncode, 0)
int(stdout.strip()) # Check that stdout is an integer
p = subprocess.Popen([sys.executable, "-c",
"import msvcrt; print(msvcrt.open_osfhandle({}, 0))".format(handles[0])],
stdout=subprocess.PIPE, stderr=subprocess.PIPE, close_fds=True)
stdout, stderr = p.communicate()
self.assertEqual(p.returncode, 1)
self.assertIn(b"OSError", stderr)
# The same as the previous call, but with an empty handle_list
handle_list = []
startupinfo = subprocess.STARTUPINFO()
startupinfo.lpAttributeList = {"handle_list": handle_list}
p = subprocess.Popen([sys.executable, "-c",
"import msvcrt; print(msvcrt.open_osfhandle({}, 0))".format(handles[0])],
stdout=subprocess.PIPE, stderr=subprocess.PIPE,
startupinfo=startupinfo, close_fds=True)
stdout, stderr = p.communicate()
self.assertEqual(p.returncode, 1)
self.assertIn(b"OSError", stderr)
# Check for a warning due to using handle_list and close_fds=False
with support.check_warnings((".*overriding close_fds", RuntimeWarning)):
startupinfo = subprocess.STARTUPINFO()
startupinfo.lpAttributeList = {"handle_list": handles[:]}
p = subprocess.Popen([sys.executable, "-c",
"import msvcrt; print(msvcrt.open_osfhandle({}, 0))".format(handles[0])],
stdout=subprocess.PIPE, stderr=subprocess.PIPE,
startupinfo=startupinfo, close_fds=False)
stdout, stderr = p.communicate()
self.assertEqual(p.returncode, 0)
def test_empty_attribute_list(self):
startupinfo = subprocess.STARTUPINFO()
startupinfo.lpAttributeList = {}
subprocess.call([sys.executable, "-c", "import sys; sys.exit(0)"],
startupinfo=startupinfo)
def test_empty_handle_list(self):
startupinfo = subprocess.STARTUPINFO()
startupinfo.lpAttributeList = {"handle_list": []}
subprocess.call([sys.executable, "-c", "import sys; sys.exit(0)"],
startupinfo=startupinfo)
def test_shell_sequence(self):
# Run command through the shell (sequence)
newenv = os.environ.copy()

View File

@ -466,6 +466,7 @@ Carl Feynman
Vincent Fiack
Anastasia Filatova
Tomer Filiba
Segev Finer
Jeffrey Finkelstein
Russell Finn
Dan Finnie

View File

@ -0,0 +1,2 @@
Implement support for `subprocess.Popen(close_fds=True)` on Windows. Patch
by Segev Finer.

View File

@ -811,6 +811,165 @@ getenvironment(PyObject* environment)
return NULL;
}
static LPHANDLE
gethandlelist(PyObject *mapping, const char *name, Py_ssize_t *size)
{
LPHANDLE ret = NULL;
PyObject *value_fast = NULL;
PyObject *value;
Py_ssize_t i;
value = PyMapping_GetItemString(mapping, name);
if (!value) {
PyErr_Clear();
return NULL;
}
if (value == Py_None) {
goto cleanup;
}
value_fast = PySequence_Fast(value, "handle_list must be a sequence or None");
if (value_fast == NULL)
goto cleanup;
*size = PySequence_Fast_GET_SIZE(value_fast) * sizeof(HANDLE);
/* Passing an empty array causes CreateProcess to fail so just don't set it */
if (*size == 0) {
goto cleanup;
}
ret = PyMem_Malloc(*size);
if (ret == NULL)
goto cleanup;
for (i = 0; i < PySequence_Fast_GET_SIZE(value_fast); i++) {
ret[i] = PYNUM_TO_HANDLE(PySequence_Fast_GET_ITEM(value_fast, i));
if (ret[i] == (HANDLE)-1 && PyErr_Occurred()) {
PyMem_Free(ret);
ret = NULL;
goto cleanup;
}
}
cleanup:
Py_DECREF(value);
Py_XDECREF(value_fast);
return ret;
}
typedef struct {
LPPROC_THREAD_ATTRIBUTE_LIST attribute_list;
LPHANDLE handle_list;
} AttributeList;
static void
freeattributelist(AttributeList *attribute_list)
{
if (attribute_list->attribute_list != NULL) {
DeleteProcThreadAttributeList(attribute_list->attribute_list);
PyMem_Free(attribute_list->attribute_list);
}
PyMem_Free(attribute_list->handle_list);
memset(attribute_list, 0, sizeof(*attribute_list));
}
static int
getattributelist(PyObject *obj, const char *name, AttributeList *attribute_list)
{
int ret = 0;
DWORD err;
BOOL result;
PyObject *value;
Py_ssize_t handle_list_size;
DWORD attribute_count = 0;
SIZE_T attribute_list_size = 0;
value = PyObject_GetAttrString(obj, name);
if (!value) {
PyErr_Clear(); /* FIXME: propagate error? */
return 0;
}
if (value == Py_None) {
ret = 0;
goto cleanup;
}
if (!PyMapping_Check(value)) {
ret = -1;
PyErr_Format(PyExc_TypeError, "%s must be a mapping or None", name);
goto cleanup;
}
attribute_list->handle_list = gethandlelist(value, "handle_list", &handle_list_size);
if (attribute_list->handle_list == NULL && PyErr_Occurred()) {
ret = -1;
goto cleanup;
}
if (attribute_list->handle_list != NULL)
++attribute_count;
/* Get how many bytes we need for the attribute list */
result = InitializeProcThreadAttributeList(NULL, attribute_count, 0, &attribute_list_size);
if (result || GetLastError() != ERROR_INSUFFICIENT_BUFFER) {
ret = -1;
PyErr_SetFromWindowsErr(GetLastError());
goto cleanup;
}
attribute_list->attribute_list = PyMem_Malloc(attribute_list_size);
if (attribute_list->attribute_list == NULL) {
ret = -1;
goto cleanup;
}
result = InitializeProcThreadAttributeList(
attribute_list->attribute_list,
attribute_count,
0,
&attribute_list_size);
if (!result) {
err = GetLastError();
/* So that we won't call DeleteProcThreadAttributeList */
PyMem_Free(attribute_list->attribute_list);
attribute_list->attribute_list = NULL;
ret = -1;
PyErr_SetFromWindowsErr(err);
goto cleanup;
}
if (attribute_list->handle_list != NULL) {
result = UpdateProcThreadAttribute(
attribute_list->attribute_list,
0,
PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
attribute_list->handle_list,
handle_list_size,
NULL,
NULL);
if (!result) {
ret = -1;
PyErr_SetFromWindowsErr(GetLastError());
goto cleanup;
}
}
cleanup:
Py_DECREF(value);
if (ret < 0)
freeattributelist(attribute_list);
return ret;
}
/*[clinic input]
_winapi.CreateProcess
@ -842,34 +1001,35 @@ _winapi_CreateProcess_impl(PyObject *module, Py_UNICODE *application_name,
PyObject *startup_info)
/*[clinic end generated code: output=4652a33aff4b0ae1 input=4a43b05038d639bb]*/
{
PyObject *ret = NULL;
BOOL result;
PROCESS_INFORMATION pi;
STARTUPINFOW si;
PyObject* environment;
STARTUPINFOEXW si;
PyObject *environment = NULL;
wchar_t *wenvironment;
AttributeList attribute_list = {0};
ZeroMemory(&si, sizeof(si));
si.cb = sizeof(si);
si.StartupInfo.cb = sizeof(si);
/* note: we only support a small subset of all SI attributes */
si.dwFlags = getulong(startup_info, "dwFlags");
si.wShowWindow = (WORD)getulong(startup_info, "wShowWindow");
si.hStdInput = gethandle(startup_info, "hStdInput");
si.hStdOutput = gethandle(startup_info, "hStdOutput");
si.hStdError = gethandle(startup_info, "hStdError");
si.StartupInfo.dwFlags = getulong(startup_info, "dwFlags");
si.StartupInfo.wShowWindow = (WORD)getulong(startup_info, "wShowWindow");
si.StartupInfo.hStdInput = gethandle(startup_info, "hStdInput");
si.StartupInfo.hStdOutput = gethandle(startup_info, "hStdOutput");
si.StartupInfo.hStdError = gethandle(startup_info, "hStdError");
if (PyErr_Occurred())
return NULL;
goto cleanup;
if (env_mapping != Py_None) {
environment = getenvironment(env_mapping);
if (environment == NULL) {
return NULL;
goto cleanup;
}
/* contains embedded null characters */
wenvironment = PyUnicode_AsUnicode(environment);
if (wenvironment == NULL) {
Py_DECREF(environment);
return NULL;
goto cleanup;
}
}
else {
@ -877,29 +1037,41 @@ _winapi_CreateProcess_impl(PyObject *module, Py_UNICODE *application_name,
wenvironment = NULL;
}
if (getattributelist(startup_info, "lpAttributeList", &attribute_list) < 0)
goto cleanup;
si.lpAttributeList = attribute_list.attribute_list;
Py_BEGIN_ALLOW_THREADS
result = CreateProcessW(application_name,
command_line,
NULL,
NULL,
inherit_handles,
creation_flags | CREATE_UNICODE_ENVIRONMENT,
creation_flags | EXTENDED_STARTUPINFO_PRESENT |
CREATE_UNICODE_ENVIRONMENT,
wenvironment,
current_directory,
&si,
(LPSTARTUPINFOW)&si,
&pi);
Py_END_ALLOW_THREADS
Py_XDECREF(environment);
if (!result) {
PyErr_SetFromWindowsErr(GetLastError());
goto cleanup;
}
if (! result)
return PyErr_SetFromWindowsErr(GetLastError());
return Py_BuildValue("NNkk",
ret = Py_BuildValue("NNkk",
HANDLE_TO_PYNUM(pi.hProcess),
HANDLE_TO_PYNUM(pi.hThread),
pi.dwProcessId,
pi.dwThreadId);
cleanup:
Py_XDECREF(environment);
freeattributelist(&attribute_list);
return ret;
}
/*[clinic input]
@ -1489,7 +1661,6 @@ _winapi_WriteFile_impl(PyObject *module, HANDLE handle, PyObject *buffer,
return Py_BuildValue("II", written, err);
}
/*[clinic input]
_winapi.GetACP
@ -1503,6 +1674,30 @@ _winapi_GetACP_impl(PyObject *module)
return PyLong_FromUnsignedLong(GetACP());
}
/*[clinic input]
_winapi.GetFileType -> DWORD
handle: HANDLE
[clinic start generated code]*/
static DWORD
_winapi_GetFileType_impl(PyObject *module, HANDLE handle)
/*[clinic end generated code: output=92b8466ac76ecc17 input=0058366bc40bbfbf]*/
{
DWORD result;
Py_BEGIN_ALLOW_THREADS
result = GetFileType(handle);
Py_END_ALLOW_THREADS
if (result == FILE_TYPE_UNKNOWN && GetLastError() != NO_ERROR) {
PyErr_SetFromWindowsErr(0);
return -1;
}
return result;
}
static PyMethodDef winapi_functions[] = {
_WINAPI_CLOSEHANDLE_METHODDEF
@ -1530,6 +1725,7 @@ static PyMethodDef winapi_functions[] = {
_WINAPI_WAITFORSINGLEOBJECT_METHODDEF
_WINAPI_WRITEFILE_METHODDEF
_WINAPI_GETACP_METHODDEF
_WINAPI_GETFILETYPE_METHODDEF
{NULL, NULL}
};
@ -1623,6 +1819,12 @@ PyInit__winapi(void)
WINAPI_CONSTANT(F_DWORD, CREATE_DEFAULT_ERROR_MODE);
WINAPI_CONSTANT(F_DWORD, CREATE_BREAKAWAY_FROM_JOB);
WINAPI_CONSTANT(F_DWORD, FILE_TYPE_UNKNOWN);
WINAPI_CONSTANT(F_DWORD, FILE_TYPE_DISK);
WINAPI_CONSTANT(F_DWORD, FILE_TYPE_CHAR);
WINAPI_CONSTANT(F_DWORD, FILE_TYPE_PIPE);
WINAPI_CONSTANT(F_DWORD, FILE_TYPE_REMOTE);
WINAPI_CONSTANT("i", NULL);
return m;

View File

@ -907,4 +907,38 @@ _winapi_GetACP(PyObject *module, PyObject *Py_UNUSED(ignored))
{
return _winapi_GetACP_impl(module);
}
/*[clinic end generated code: output=b9c00c323c9f4944 input=a9049054013a1b77]*/
PyDoc_STRVAR(_winapi_GetFileType__doc__,
"GetFileType($module, /, handle)\n"
"--\n"
"\n");
#define _WINAPI_GETFILETYPE_METHODDEF \
{"GetFileType", (PyCFunction)_winapi_GetFileType, METH_FASTCALL|METH_KEYWORDS, _winapi_GetFileType__doc__},
static DWORD
_winapi_GetFileType_impl(PyObject *module, HANDLE handle);
static PyObject *
_winapi_GetFileType(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames)
{
PyObject *return_value = NULL;
static const char * const _keywords[] = {"handle", NULL};
static _PyArg_Parser _parser = {"" F_HANDLE ":GetFileType", _keywords, 0};
HANDLE handle;
DWORD _return_value;
if (!_PyArg_ParseStackAndKeywords(args, nargs, kwnames, &_parser,
&handle)) {
goto exit;
}
_return_value = _winapi_GetFileType_impl(module, handle);
if ((_return_value == PY_DWORD_MAX) && PyErr_Occurred()) {
goto exit;
}
return_value = Py_BuildValue("k", _return_value);
exit:
return return_value;
}
/*[clinic end generated code: output=ec7f36eb7efc9d00 input=a9049054013a1b77]*/