From 7e680162690327c35ddde943c6b065e686d464d3 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Thu, 20 Apr 2017 22:36:41 +0300 Subject: [PATCH 01/14] bpo-19764: Implemented support for subprocess.Popen(close_fds=True) on Windows 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. --- Doc/library/subprocess.rst | 26 +++- Lib/subprocess.py | 60 ++++++--- Lib/test/test_subprocess.py | 55 ++++++++- Modules/_winapi.c | 240 ++++++++++++++++++++++++++++++++---- Modules/clinic/_winapi.c.h | 36 +++++- 5 files changed, 368 insertions(+), 49 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index afa1e86c05f6af..ee79c340717131 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -447,17 +447,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`. 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) @@ -797,6 +800,19 @@ 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 `__. + + Supported attributes: + + **handle_list** + When *close_fds* is :const:`True`, supplies a list of + handles that will be inherited. + + .. versionadded:: 3.7 Constants ^^^^^^^^^ diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 551aad342b8304..68ce6fc8c99bd2 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -134,6 +134,7 @@ def __init__(self, *, dwFlags=0, hStdInput=None, hStdOutput=None, self.hStdOutput = hStdOutput self.hStdError = hStdError self.wShowWindow = wShowWindow + self.lpAttributeList = {"handle_list": []} else: import _posixsubprocess import select @@ -608,25 +609,15 @@ def __init__(self, args, bufsize=-1, executable=None, if not isinstance(bufsize, int): raise TypeError("bufsize must be an integer") + if close_fds is _PLATFORM_DEFAULT_CLOSE_FDS: + close_fds = True + if _mswindows: 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 @@ -951,6 +942,19 @@ def _make_inheritable(self, handle): 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""" + # 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. We use a set comprehension to remove duplicates. + 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, @@ -968,12 +972,40 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, # 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 = (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.setdefault("handle_list", []) + + if use_std_handles: + handle_list += [p2cread, c2pwrite, errwrite] + + handle_list[:] = self._filter_handle_list(handle_list) + + if handle_list: + if have_handle_list and 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 diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index f01bd1a6951865..0e8880e76a172e 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2601,11 +2601,6 @@ def test_invalid_args(self): [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) def test_close_fds(self): # close file descriptors @@ -2614,6 +2609,56 @@ def test_close_fds(self): 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) + + 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() diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 8d01b376f2597c..21658bc57fa876 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -795,6 +795,157 @@ 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)); + } + +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; + BOOL result; + PyObject *value; + Py_ssize_t handle_list_size; + DWORD attribute_count = 0; + SIZE_T attribute_list_size; + + 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) { + /* So that we won't call DeleteProcThreadAttributeList */ + PyMem_Free(attribute_list->attribute_list); + attribute_list->attribute_list = NULL; + + ret = -1; + PyErr_SetFromWindowsErr(GetLastError()); + 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 @@ -826,63 +977,74 @@ _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) - return NULL; + goto cleanup; wenvironment = PyUnicode_AsUnicode(environment); if (wenvironment == NULL) - { - Py_XDECREF(environment); - return NULL; - } + goto cleanup; } else { environment = NULL; 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()); + 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 Py_BuildValue("NNkk", - HANDLE_TO_PYNUM(pi.hProcess), - HANDLE_TO_PYNUM(pi.hThread), - pi.dwProcessId, - pi.dwThreadId); + return ret; } /*[clinic input] @@ -1472,6 +1634,30 @@ _winapi_WriteFile_impl(PyObject *module, HANDLE handle, PyObject *buffer, return Py_BuildValue("II", written, err); } +/*[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 @@ -1498,6 +1684,7 @@ static PyMethodDef winapi_functions[] = { _WINAPI_WAITFORMULTIPLEOBJECTS_METHODDEF _WINAPI_WAITFORSINGLEOBJECT_METHODDEF _WINAPI_WRITEFILE_METHODDEF + _WINAPI_GETFILETYPE_METHODDEF {NULL, NULL} }; @@ -1578,6 +1765,11 @@ PyInit__winapi(void) WINAPI_CONSTANT(F_DWORD, WAIT_OBJECT_0); WINAPI_CONSTANT(F_DWORD, WAIT_ABANDONED_0); WINAPI_CONSTANT(F_DWORD, WAIT_TIMEOUT); + 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); diff --git a/Modules/clinic/_winapi.c.h b/Modules/clinic/_winapi.c.h index 431580293ed144..c625a4d73d1c24 100644 --- a/Modules/clinic/_winapi.c.h +++ b/Modules/clinic/_winapi.c.h @@ -941,4 +941,38 @@ _winapi_WriteFile(PyObject *module, PyObject **args, Py_ssize_t nargs, PyObject exit: return return_value; } -/*[clinic end generated code: output=2beb984508fb040a input=a9049054013a1b77]*/ + +PyDoc_STRVAR(_winapi_GetFileType__doc__, +"GetFileType($module, /, handle)\n" +"--\n" +"\n"); + +#define _WINAPI_GETFILETYPE_METHODDEF \ + {"GetFileType", (PyCFunction)_winapi_GetFileType, METH_FASTCALL, _winapi_GetFileType__doc__}, + +static DWORD +_winapi_GetFileType_impl(PyObject *module, HANDLE handle); + +static PyObject * +_winapi_GetFileType(PyObject *module, PyObject **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 == DWORD_MAX) && PyErr_Occurred()) { + goto exit; + } + return_value = Py_BuildValue("k", _return_value); + +exit: + return return_value; +} +/*[clinic end generated code: output=90c10be97410f00e input=a9049054013a1b77]*/ From ad07b107b825ad7b70897143cba3ed615312dcf1 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Fri, 21 Apr 2017 01:50:39 +0300 Subject: [PATCH 02/14] bpo-19764: Convert subprocess.Handle to int in handle_list --- Lib/subprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 68ce6fc8c99bd2..581b8cfa1a1bf4 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -992,7 +992,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, handle_list = attribute_list.setdefault("handle_list", []) if use_std_handles: - handle_list += [p2cread, c2pwrite, errwrite] + handle_list += [int(p2cread), int(c2pwrite), int(errwrite)] handle_list[:] = self._filter_handle_list(handle_list) From 9ad94f36432bb83d5e83548870b895f6af461bac Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Fri, 21 Apr 2017 13:02:08 +0300 Subject: [PATCH 03/14] bpo-19764: Fixed the handle_list warning with close_fds=True Also fixed the documentation for handle_list. --- Doc/library/subprocess.rst | 2 +- Lib/subprocess.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index ee79c340717131..e3ca488dc8ba88 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -809,7 +809,7 @@ on Windows. Supported attributes: **handle_list** - When *close_fds* is :const:`True`, supplies a list of + When *close_fds* is :const:`False`, supplies a list of handles that will be inherited. .. versionadded:: 3.7 diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 581b8cfa1a1bf4..27438be1088fa0 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -981,9 +981,9 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, startupinfo.hStdError = errwrite attribute_list = startupinfo.lpAttributeList - have_handle_list = (attribute_list and - "handle_list" in attribute_list and - attribute_list["handle_list"]) + 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): @@ -997,7 +997,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, handle_list[:] = self._filter_handle_list(handle_list) if handle_list: - if have_handle_list and not close_fds: + if have_handle_list and close_fds: warnings.warn("startupinfo.lpAttributeList['handle_list'] " "overriding close_fds", RuntimeWarning) From 24f3f3920613f73b7a2cbe3f6a06833d5a5f9857 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Fri, 21 Apr 2017 13:35:16 +0300 Subject: [PATCH 04/14] bpo-19764: Added a test for the overriding close_fds warning --- Lib/test/test_subprocess.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 0e8880e76a172e..1f5e77a0f80e1f 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2647,6 +2647,17 @@ def test_close_fds_with_stdio(self): self.assertEqual(p.returncode, 1) self.assertIn(b"OSError", stderr) + # And with a non empty handle_list + 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=True) + stdout, stderr = p.communicate() + self.assertEqual(p.returncode, 0) + def test_empty_attribute_list(self): startupinfo = subprocess.STARTUPINFO() startupinfo.lpAttributeList = {} From 1eab24290b9005e0abf63dc52ae2206cfa053ab2 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Fri, 21 Apr 2017 19:39:31 +0300 Subject: [PATCH 05/14] bpo-19764: The warning should be emitted for handle_list and close_fds=True --- Doc/library/subprocess.rst | 2 +- Lib/subprocess.py | 2 +- Lib/test/test_subprocess.py | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index e3ca488dc8ba88..ee79c340717131 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -809,7 +809,7 @@ on Windows. Supported attributes: **handle_list** - When *close_fds* is :const:`False`, supplies a list of + When *close_fds* is :const:`True`, supplies a list of handles that will be inherited. .. versionadded:: 3.7 diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 27438be1088fa0..e3df1ec94a7200 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -997,7 +997,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, handle_list[:] = self._filter_handle_list(handle_list) if handle_list: - if have_handle_list and close_fds: + if not close_fds: warnings.warn("startupinfo.lpAttributeList['handle_list'] " "overriding close_fds", RuntimeWarning) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 1f5e77a0f80e1f..f753da5ae36c51 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2647,14 +2647,14 @@ def test_close_fds_with_stdio(self): self.assertEqual(p.returncode, 1) self.assertIn(b"OSError", stderr) - # And with a non empty handle_list + # 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=True) + startupinfo=startupinfo, close_fds=False) stdout, stderr = p.communicate() self.assertEqual(p.returncode, 0) From 5b3cccc0e1333db6b6140f15bdf07b89d7972404 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Wed, 31 May 2017 19:40:01 +0300 Subject: [PATCH 06/14] Minor fixes in _winapi.c --- Modules/_winapi.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 21658bc57fa876..f263ee9d17c7d0 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -830,6 +830,11 @@ gethandlelist(PyObject *mapping, const char *name, Py_ssize_t *size) 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: @@ -860,11 +865,12 @@ 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; + SIZE_T attribute_list_size = 0; value = PyObject_GetAttrString(obj, name); if (!value) { @@ -912,12 +918,14 @@ getattributelist(PyObject *obj, const char *name, AttributeList *attribute_list) 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(GetLastError()); + PyErr_SetFromWindowsErr(err); goto cleanup; } From c84449c179fe59a94d4f3e80aa529c3cb8a97ae2 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Tue, 6 Jun 2017 21:28:55 +0300 Subject: [PATCH 07/14] bpo-19764: Addressed review comments by haypo --- Doc/library/subprocess.rst | 8 ++++---- Doc/whatsnew/3.7.rst | 18 ++++++++++++++++++ Lib/subprocess.py | 17 ++++++----------- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index ee79c340717131..2c8bb329dc4558 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -458,8 +458,8 @@ functions. .. versionchanged:: 3.7 On Windows the default for *close_fds* was changed from :const:`False` to - :const:`True`. It's now possible to set *close_fds* to :const:`True` when - redirecting the standard handles. + :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 @@ -809,8 +809,8 @@ on Windows. Supported attributes: **handle_list** - When *close_fds* is :const:`True`, supplies a list of - handles that will be inherited. + Sequence of handles that will be inherited. *close_fds* must be true if + non-empty. .. versionadded:: 3.7 diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst index 875fc556912cae..5e27039e7a5a28 100644 --- a/Doc/whatsnew/3.7.rst +++ b/Doc/whatsnew/3.7.rst @@ -132,6 +132,17 @@ Serhiy Storchaka in :issue:`28682`.) Added support for :ref:`file descriptors ` in :func:`~os.scandir` on Unix. (Contributed by Serhiy Storchaka in :issue:`25996`.) +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. + unittest.mock ------------- @@ -294,6 +305,13 @@ Changes in the Python API ``makedirs()``. (Contributed by Serhiy Storchaka in :issue:`19930`.) +* 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 `. CPython bytecode changes ------------------------ diff --git a/Lib/subprocess.py b/Lib/subprocess.py index e3df1ec94a7200..fa7e40deb31017 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -128,13 +128,13 @@ def stdout(self, value): 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 = {"handle_list": []} + self.lpAttributeList = lpAttributeList or {"handle_list": []} else: import _posixsubprocess import select @@ -537,9 +537,6 @@ def getoutput(cmd): return getstatusoutput(cmd)[1] -_PLATFORM_DEFAULT_CLOSE_FDS = object() - - class Popen(object): """ Execute a child program in a new process. @@ -588,7 +585,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=False, startupinfo=None, creationflags=0, restore_signals=True, start_new_session=False, @@ -609,9 +606,6 @@ def __init__(self, args, bufsize=-1, executable=None, if not isinstance(bufsize, int): raise TypeError("bufsize must be an integer") - if close_fds is _PLATFORM_DEFAULT_CLOSE_FDS: - close_fds = True - if _mswindows: if preexec_fn is not None: raise ValueError("preexec_fn is not supported on Windows " @@ -945,10 +939,10 @@ def _make_inheritable(self, handle): 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""" + 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. We use a set comprehension to remove duplicates. + # cause it to fail. return list({handle for handle in handle_list if handle & 0x3 != 0x3 or _winapi.GetFileType(handle) != @@ -990,6 +984,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, if attribute_list is None: attribute_list = startupinfo.lpAttributeList = {} handle_list = attribute_list.setdefault("handle_list", []) + handle_list = attribute_list["handle_list"] = list(handle_list) if use_std_handles: handle_list += [int(p2cread), int(c2pwrite), int(errwrite)] From 0e60d28d4c1f50dec9608aeecf833f242ed0d8cd Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Tue, 6 Jun 2017 22:23:00 +0300 Subject: [PATCH 08/14] bpo-19764: Simplified the handle_list cast to list --- Lib/subprocess.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index fa7e40deb31017..bfc46268d474f5 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -983,8 +983,8 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, if have_handle_list or (use_std_handles and close_fds): if attribute_list is None: attribute_list = startupinfo.lpAttributeList = {} - handle_list = attribute_list.setdefault("handle_list", []) - handle_list = attribute_list["handle_list"] = list(handle_list) + handle_list = attribute_list["handle_list"] = \ + list(attribute_list.get("handle_list", [])) if use_std_handles: handle_list += [int(p2cread), int(c2pwrite), int(errwrite)] From 167590b4f20d226e31925eed75dedfb2a4aba5e2 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Wed, 7 Jun 2017 20:50:00 +0300 Subject: [PATCH 09/14] bpo-19764: Add some extra documentation about handle_list --- Doc/library/subprocess.rst | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 8b8e459b84a8d2..e1cf180b482878 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -812,6 +812,19 @@ on Windows. Sequence of handles that will be inherited. *close_fds* must be true if non-empty. + The handles must be made inheritable by + :func:`os.set_handle_inheritable` or :class:`OSError` will be raised + with Windows error `ERROR_INVALID_PARAMETER` (87). They only have to be + inheritable during the process creation and can be made non-inheritable + right afterwards. + + Note that inheritable handles can be inherited by any process created + that is set to inherit all handles. Other process creation functions + might not be using an explicit handle list to avoid inheriting all + handles like subprocess does, so you should be careful in using them + when you have handles marked as inheritable. Also note that standard + handles redirection requires creating inheritable handles temporarily. + .. versionadded:: 3.7 Constants From 59af91f1aa87d509e5274cf20ced97258c92405d Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Thu, 8 Jun 2017 00:25:22 +0300 Subject: [PATCH 10/14] bpo-19764: Documentation update --- Doc/library/subprocess.rst | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index e1cf180b482878..9764f0063b1ae7 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -750,7 +750,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 `__ @@ -812,18 +812,19 @@ on Windows. Sequence of handles that will be inherited. *close_fds* must be true if non-empty. - The handles must be made inheritable by - :func:`os.set_handle_inheritable` or :class:`OSError` will be raised - with Windows error `ERROR_INVALID_PARAMETER` (87). They only have to be - inheritable during the process creation and can be made non-inheritable - right afterwards. - - Note that inheritable handles can be inherited by any process created - that is set to inherit all handles. Other process creation functions - might not be using an explicit handle list to avoid inheriting all - handles like subprocess does, so you should be careful in using them - when you have handles marked as inheritable. Also note that standard - handles redirection requires creating inheritable handles temporarily. + 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 From d4a9cdbcebb2ebf050297549d264638c20d61e18 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Thu, 8 Jun 2017 07:11:04 +0300 Subject: [PATCH 11/14] bpo-19764: Fixed bad reStructuredText markup --- Doc/library/subprocess.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 9764f0063b1ae7..7e992cf323e7f0 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -815,7 +815,7 @@ on Windows. 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). + ``ERROR_INVALID_PARAMETER`` (87). .. warning:: From 8e5d21f1cf5f4c982bd8a8d09497fadb5e2e565f Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Fri, 18 Aug 2017 17:56:05 +0300 Subject: [PATCH 12/14] bpo-19764: Fix whitespace --- Doc/whatsnew/3.7.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst index 83788ed7d18a9c..60a7c10d2c0ff0 100644 --- a/Doc/whatsnew/3.7.rst +++ b/Doc/whatsnew/3.7.rst @@ -465,7 +465,7 @@ Changes in the Python API * Due to internal changes in :mod:`socket` you won't be able to :func:`socket.fromshare` a socket :func:`~socket.socket.share`-ed in older Python versions. - + * ``repr`` for :class:`datetime.timedelta` has changed to include keyword arguments in the output. (Contributed by Utkarsh Upadhyay in :issue:`30302`.) From 208274c8df26b5dca9cbc332500316023529fcf5 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Fri, 18 Aug 2017 18:00:36 +0300 Subject: [PATCH 13/14] bpo-19764: Add NEWS.d entry --- .../next/Windows/2017-08-18-18-00-24.bpo-19764.ODpc9y.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Windows/2017-08-18-18-00-24.bpo-19764.ODpc9y.rst diff --git a/Misc/NEWS.d/next/Windows/2017-08-18-18-00-24.bpo-19764.ODpc9y.rst b/Misc/NEWS.d/next/Windows/2017-08-18-18-00-24.bpo-19764.ODpc9y.rst new file mode 100644 index 00000000000000..b5f5cae83230fd --- /dev/null +++ b/Misc/NEWS.d/next/Windows/2017-08-18-18-00-24.bpo-19764.ODpc9y.rst @@ -0,0 +1,2 @@ +Implement support for `subprocess.Popen(close_fds=True)` on Windows. Patch +by Segev Finer. From 3b0426e188575ef5fe7a8e63072f7897ce70e169 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Sat, 16 Dec 2017 12:07:29 +0200 Subject: [PATCH 14/14] Add Segev Finer to Misc/ACKS --- Misc/ACKS | 1 + 1 file changed, 1 insertion(+) diff --git a/Misc/ACKS b/Misc/ACKS index 8303ce80050d2e..e5343899640fe6 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -466,6 +466,7 @@ Carl Feynman Vincent Fiack Anastasia Filatova Tomer Filiba +Segev Finer Jeffrey Finkelstein Russell Finn Dan Finnie