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.

2 files changed, 260 insertions, 24 deletions

diff --git a/Modules/_winapi.c b/Modules/_winapi.c
index 604c05d..c596cba 100644
--- a/Modules/_winapi.c
+++ b/Modules/_winapi.c
@@ -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());
+    ret = Py_BuildValue("NNkk",
+                        HANDLE_TO_PYNUM(pi.hProcess),
+                        HANDLE_TO_PYNUM(pi.hThread),
+                        pi.dwProcessId,
+                        pi.dwThreadId);
 
-    return 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;
diff --git a/Modules/clinic/_winapi.c.h b/Modules/clinic/_winapi.c.h
index 80edbf5..b14f087 100644
--- a/Modules/clinic/_winapi.c.h
+++ b/Modules/clinic/_winapi.c.h
@@ -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]*/