Commit 9bbb9a0c authored by Mark Hammond's avatar Mark Hammond

Patch #101032, from David Bolen:

This is an enhancement to a prior patch (100941) ...
[T]his patch removes the risk of deadlock waiting for the child previously present in certain cases. It adds tracking of all file handles returned from an os.popen* call and only waits for the child process, returning the exit code, on the closure of the final file handle to that child.
parent f2d678e4
...@@ -2098,7 +2098,7 @@ posix_popen(PyObject *self, PyObject *args) ...@@ -2098,7 +2098,7 @@ posix_popen(PyObject *self, PyObject *args)
* *
* Written by Bill Tutt <billtut@microsoft.com>. Minor tweaks * Written by Bill Tutt <billtut@microsoft.com>. Minor tweaks
* and 2.0 integration by Fredrik Lundh <fredrik@pythonware.com> * and 2.0 integration by Fredrik Lundh <fredrik@pythonware.com>
* Return code handling by David Bolen. * Return code handling by David Bolen <db3l@fitlinxx.com>.
*/ */
#include <malloc.h> #include <malloc.h>
...@@ -2116,8 +2116,8 @@ static int _PyPclose(FILE *file); ...@@ -2116,8 +2116,8 @@ static int _PyPclose(FILE *file);
/* /*
* Internal dictionary mapping popen* file pointers to process handles, * Internal dictionary mapping popen* file pointers to process handles,
* in order to maintain a link to the process handle until the file is * for use when retrieving the process exit code. See _PyPclose() below
* closed, at which point the process exit code is returned to the caller. * for more information on this dictionary's use.
*/ */
static PyObject *_PyPopenProcs = NULL; static PyObject *_PyPopenProcs = NULL;
...@@ -2276,10 +2276,11 @@ win32_popen4(PyObject *self, PyObject *args) ...@@ -2276,10 +2276,11 @@ win32_popen4(PyObject *self, PyObject *args)
} }
static int static int
_PyPopenCreateProcess(char *cmdstring, FILE *file, _PyPopenCreateProcess(char *cmdstring,
HANDLE hStdin, HANDLE hStdin,
HANDLE hStdout, HANDLE hStdout,
HANDLE hStderr) HANDLE hStderr,
HANDLE *hProcess)
{ {
PROCESS_INFORMATION piProcInfo; PROCESS_INFORMATION piProcInfo;
STARTUPINFO siStartInfo; STARTUPINFO siStartInfo;
...@@ -2354,26 +2355,8 @@ _PyPopenCreateProcess(char *cmdstring, FILE *file, ...@@ -2354,26 +2355,8 @@ _PyPopenCreateProcess(char *cmdstring, FILE *file,
/* Close the handles now so anyone waiting is woken. */ /* Close the handles now so anyone waiting is woken. */
CloseHandle(piProcInfo.hThread); CloseHandle(piProcInfo.hThread);
/* /* Return process handle */
* Try to insert our process handle into the internal *hProcess = piProcInfo.hProcess;
* dictionary so we can find it later when trying
* to close this file.
*/
if (!_PyPopenProcs)
_PyPopenProcs = PyDict_New();
if (_PyPopenProcs) {
PyObject *hProcessObj, *fileObj;
hProcessObj = PyLong_FromVoidPtr(piProcInfo.hProcess);
fileObj = PyLong_FromVoidPtr(file);
if (!hProcessObj || !fileObj ||
PyDict_SetItem(_PyPopenProcs,
fileObj, hProcessObj) < 0) {
/* Insert failure - close handle to prevent leak */
CloseHandle(piProcInfo.hProcess);
}
}
return TRUE; return TRUE;
} }
return FALSE; return FALSE;
...@@ -2386,12 +2369,13 @@ _PyPopen(char *cmdstring, int mode, int n) ...@@ -2386,12 +2369,13 @@ _PyPopen(char *cmdstring, int mode, int n)
{ {
HANDLE hChildStdinRd, hChildStdinWr, hChildStdoutRd, hChildStdoutWr, HANDLE hChildStdinRd, hChildStdinWr, hChildStdoutRd, hChildStdoutWr,
hChildStderrRd, hChildStderrWr, hChildStdinWrDup, hChildStdoutRdDup, hChildStderrRd, hChildStderrWr, hChildStdinWrDup, hChildStdoutRdDup,
hChildStderrRdDup; /* hChildStdoutWrDup; */ hChildStderrRdDup, hProcess; /* hChildStdoutWrDup; */
SECURITY_ATTRIBUTES saAttr; SECURITY_ATTRIBUTES saAttr;
BOOL fSuccess; BOOL fSuccess;
int fd1, fd2, fd3; int fd1, fd2, fd3;
FILE *f1, *f2, *f3; FILE *f1, *f2, *f3;
long file_count;
PyObject *f; PyObject *f;
saAttr.nLength = sizeof(SECURITY_ATTRIBUTES); saAttr.nLength = sizeof(SECURITY_ATTRIBUTES);
...@@ -2490,6 +2474,7 @@ _PyPopen(char *cmdstring, int mode, int n) ...@@ -2490,6 +2474,7 @@ _PyPopen(char *cmdstring, int mode, int n)
CloseHandle(hChildStderrRdDup); CloseHandle(hChildStderrRdDup);
break; break;
} }
file_count = 1;
break; break;
case POPEN_2: case POPEN_2:
...@@ -2512,13 +2497,14 @@ _PyPopen(char *cmdstring, int mode, int n) ...@@ -2512,13 +2497,14 @@ _PyPopen(char *cmdstring, int mode, int n)
f2 = _fdopen(fd2, m1); f2 = _fdopen(fd2, m1);
p1 = PyFile_FromFile(f1, cmdstring, m2, _PyPclose); p1 = PyFile_FromFile(f1, cmdstring, m2, _PyPclose);
PyFile_SetBufSize(p1, 0); PyFile_SetBufSize(p1, 0);
p2 = PyFile_FromFile(f2, cmdstring, m1, fclose); p2 = PyFile_FromFile(f2, cmdstring, m1, _PyPclose);
PyFile_SetBufSize(p2, 0); PyFile_SetBufSize(p2, 0);
if (n != 4) if (n != 4)
CloseHandle(hChildStderrRdDup); CloseHandle(hChildStderrRdDup);
f = Py_BuildValue("OO",p1,p2); f = Py_BuildValue("OO",p1,p2);
file_count = 2;
break; break;
} }
...@@ -2542,33 +2528,119 @@ _PyPopen(char *cmdstring, int mode, int n) ...@@ -2542,33 +2528,119 @@ _PyPopen(char *cmdstring, int mode, int n)
fd3 = _open_osfhandle((long)hChildStderrRdDup, mode); fd3 = _open_osfhandle((long)hChildStderrRdDup, mode);
f3 = _fdopen(fd3, m1); f3 = _fdopen(fd3, m1);
p1 = PyFile_FromFile(f1, cmdstring, m2, _PyPclose); p1 = PyFile_FromFile(f1, cmdstring, m2, _PyPclose);
p2 = PyFile_FromFile(f2, cmdstring, m1, fclose); p2 = PyFile_FromFile(f2, cmdstring, m1, _PyPclose);
p3 = PyFile_FromFile(f3, cmdstring, m1, fclose); p3 = PyFile_FromFile(f3, cmdstring, m1, _PyPclose);
PyFile_SetBufSize(p1, 0); PyFile_SetBufSize(p1, 0);
PyFile_SetBufSize(p2, 0); PyFile_SetBufSize(p2, 0);
PyFile_SetBufSize(p3, 0); PyFile_SetBufSize(p3, 0);
f = Py_BuildValue("OOO",p1,p2,p3); f = Py_BuildValue("OOO",p1,p2,p3);
file_count = 3;
break; break;
} }
} }
if (n == POPEN_4) { if (n == POPEN_4) {
if (!_PyPopenCreateProcess(cmdstring, if (!_PyPopenCreateProcess(cmdstring,
f1,
hChildStdinRd, hChildStdinRd,
hChildStdoutWr, hChildStdoutWr,
hChildStdoutWr)) hChildStdoutWr,
&hProcess))
return win32_error("CreateProcess", NULL); return win32_error("CreateProcess", NULL);
} }
else { else {
if (!_PyPopenCreateProcess(cmdstring, if (!_PyPopenCreateProcess(cmdstring,
f1,
hChildStdinRd, hChildStdinRd,
hChildStdoutWr, hChildStdoutWr,
hChildStderrWr)) hChildStderrWr,
&hProcess))
return win32_error("CreateProcess", NULL); return win32_error("CreateProcess", NULL);
} }
/*
* Insert the files we've created into the process dictionary
* all referencing the list with the process handle and the
* initial number of files (see description below in _PyPclose).
* Since if _PyPclose later tried to wait on a process when all
* handles weren't closed, it could create a deadlock with the
* child, we spend some energy here to try to ensure that we
* either insert all file handles into the dictionary or none
* at all. It's a little clumsy with the various popen modes
* and variable number of files involved.
*/
if (!_PyPopenProcs) {
_PyPopenProcs = PyDict_New();
}
if (_PyPopenProcs) {
PyObject *procObj, *hProcessObj, *intObj, *fileObj[3];
int ins_rc[3];
fileObj[0] = fileObj[1] = fileObj[2] = NULL;
ins_rc[0] = ins_rc[1] = ins_rc[2] = 0;
procObj = PyList_New(2);
hProcessObj = PyLong_FromVoidPtr(hProcess);
intObj = PyInt_FromLong(file_count);
if (procObj && hProcessObj && intObj) {
PyList_SetItem(procObj,0,hProcessObj);
PyList_SetItem(procObj,1,intObj);
fileObj[0] = PyLong_FromVoidPtr(f1);
if (fileObj[0]) {
ins_rc[0] = PyDict_SetItem(_PyPopenProcs,
fileObj[0],
procObj);
}
if (file_count >= 2) {
fileObj[1] = PyLong_FromVoidPtr(f2);
if (fileObj[1]) {
ins_rc[1] = PyDict_SetItem(_PyPopenProcs,
fileObj[1],
procObj);
}
}
if (file_count >= 3) {
fileObj[2] = PyLong_FromVoidPtr(f3);
if (fileObj[2]) {
ins_rc[2] = PyDict_SetItem(_PyPopenProcs,
fileObj[2],
procObj);
}
}
if (ins_rc[0] < 0 || !fileObj[0] ||
ins_rc[1] < 0 || (file_count > 1 && !fileObj[1]) ||
ins_rc[2] < 0 || (file_count > 2 && !fileObj[2])) {
/* Something failed - remove any dictionary
* entries that did make it.
*/
if (!ins_rc[0] && fileObj[0]) {
PyDict_DelItem(_PyPopenProcs,
fileObj[0]);
}
if (!ins_rc[1] && fileObj[1]) {
PyDict_DelItem(_PyPopenProcs,
fileObj[1]);
}
if (!ins_rc[2] && fileObj[2]) {
PyDict_DelItem(_PyPopenProcs,
fileObj[2]);
}
}
}
/*
* Clean up our localized references for the dictionary keys
* and value since PyDict_SetItem will Py_INCREF any copies
* that got placed in the dictionary.
*/
Py_XDECREF(procObj);
Py_XDECREF(fileObj[0]);
Py_XDECREF(fileObj[1]);
Py_XDECREF(fileObj[2]);
}
/* Child is launched. Close the parents copy of those pipe /* Child is launched. Close the parents copy of those pipe
* handles that only the child should have open. You need to * handles that only the child should have open. You need to
* make sure that no handles to the write end of the output pipe * make sure that no handles to the write end of the output pipe
...@@ -2590,25 +2662,55 @@ _PyPopen(char *cmdstring, int mode, int n) ...@@ -2590,25 +2662,55 @@ _PyPopen(char *cmdstring, int mode, int n)
/* /*
* Wrapper for fclose() to use for popen* files, so we can retrieve the * Wrapper for fclose() to use for popen* files, so we can retrieve the
* exit code for the child process and return as a result of the close. * exit code for the child process and return as a result of the close.
*
* This function uses the _PyPopenProcs dictionary in order to map the
* input file pointer to information about the process that was
* originally created by the popen* call that created the file pointer.
* The dictionary uses the file pointer as a key (with one entry
* inserted for each file returned by the original popen* call) and a
* single list object as the value for all files from a single call.
* The list object contains the Win32 process handle at [0], and a file
* count at [1], which is initialized to the total number of file
* handles using that list.
*
* This function closes whichever handle it is passed, and decrements
* the file count in the dictionary for the process handle pointed to
* by this file. On the last close (when the file count reaches zero),
* this function will wait for the child process and then return its
* exit code as the result of the close() operation. This permits the
* files to be closed in any order - it is always the close() of the
* final handle that will return the exit code.
*/ */
static int _PyPclose(FILE *file) static int _PyPclose(FILE *file)
{ {
int result; int result;
DWORD exit_code; DWORD exit_code;
HANDLE hProcess; HANDLE hProcess;
PyObject *hProcessObj, *fileObj; PyObject *procObj, *hProcessObj, *intObj, *fileObj;
long file_count;
/* Close the file handle first, to ensure it can't block the /* Close the file handle first, to ensure it can't block the
* child from exiting when we wait for it below. * child from exiting if it's the last handle.
*/ */
result = fclose(file); result = fclose(file);
if (_PyPopenProcs) { if (_PyPopenProcs) {
fileObj = PyLong_FromVoidPtr(file); if ((fileObj = PyLong_FromVoidPtr(file)) != NULL &&
if (fileObj) { (procObj = PyDict_GetItem(_PyPopenProcs,
hProcessObj = PyDict_GetItem(_PyPopenProcs, fileObj); fileObj)) != NULL &&
if (hProcessObj) { (hProcessObj = PyList_GetItem(procObj,0)) != NULL &&
(intObj = PyList_GetItem(procObj,1)) != NULL) {
hProcess = PyLong_AsVoidPtr(hProcessObj); hProcess = PyLong_AsVoidPtr(hProcessObj);
file_count = PyInt_AsLong(intObj);
if (file_count > 1) {
/* Still other files referencing process */
file_count--;
PyList_SetItem(procObj,1,
PyInt_FromLong(file_count));
} else {
/* Last file for this process */
if (result != EOF && if (result != EOF &&
WaitForSingleObject(hProcess, INFINITE) != WAIT_FAILED && WaitForSingleObject(hProcess, INFINITE) != WAIT_FAILED &&
GetExitCodeProcess(hProcess, &exit_code)) { GetExitCodeProcess(hProcess, &exit_code)) {
...@@ -2634,15 +2736,19 @@ static int _PyPclose(FILE *file) ...@@ -2634,15 +2736,19 @@ static int _PyPclose(FILE *file)
/* Free up the native handle at this point */ /* Free up the native handle at this point */
CloseHandle(hProcess); CloseHandle(hProcess);
}
/* Remove from dictionary and flush dictionary if empty */ /* Remove this file pointer from dictionary */
PyDict_DelItem(_PyPopenProcs, fileObj); PyDict_DelItem(_PyPopenProcs, fileObj);
if (PyDict_Size(_PyPopenProcs) == 0) { if (PyDict_Size(_PyPopenProcs) == 0) {
Py_DECREF(_PyPopenProcs); Py_DECREF(_PyPopenProcs);
_PyPopenProcs = NULL; _PyPopenProcs = NULL;
} }
} /* if hProcessObj */
} /* if fileObj */ } /* if object retrieval ok */
Py_XDECREF(fileObj);
} /* if _PyPopenProcs */ } /* if _PyPopenProcs */
return result; return result;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment