giampaolo · GitHub

So this is kind of a big one and the repercussions are hard to describe because it's subtle. This test case:

def assert_gone(pid):
# XXX
if WINDOWS:
return
assert not psutil.pid_exists(pid), pid
assert pid not in psutil.pids(), pid
try:
p = psutil.Process(pid)
assert not p.is_running(), pid
except psutil.NoSuchProcess:
pass
else:
assert 0, "pid %s is not gone" % pid

...which was disabled on Windows, shows that processes unexpectedly stick around after being terminate()d and wait()ed on. The problem originates from OpenProcess() Windows API.
OpenProcess() is unreliable in that it can return a handle for a process PID which no longer exists, so we assume that GetExitCodeProcess returning STILL_ACTIVE means it's running, else it isn't. It turns out this is not always the case. This had repercussions on psutil.pid_exists() API which returned an incorrect value, and also on other psutil APIs relying on it internally (cmdline(), environ(), cwd(), connections()). This PR fixes this logic and assumes a process handle is actually running if:

  1. OpenProcess doesn't return ERROR_INVALID_PARAMETER
  2. GetExitCodeProcess must return STILL_ACTIVE
  3. if not then iterate over all PIDs, which is slower but safe (this is the part which was missing)
    Full logic here:
    /*
    * Given a process HANDLE checks whether it's actually running.
    * Returns:
    * - 1: running
    * - 0: not running
    * - -1: WindowsError
    * - -2: AssertionError
    */
    int
    psutil_is_phandle_running(HANDLE hProcess, DWORD pid) {
    DWORD processExitCode = 0;
    if (hProcess == NULL) {
    if (GetLastError() == ERROR_INVALID_PARAMETER) {
    // Yeah, this is the actual error code in case of
    // "no such process".
    if (! psutil_assert_pid_not_exists(
    pid, "iphr: OpenProcess() -> ERROR_INVALID_PARAMETER")) {
    return -2;
    }
    return 0;
    }
    return -1;
    }
    if (GetExitCodeProcess(hProcess, &processExitCode)) {
    // XXX - maybe STILL_ACTIVE is not fully reliable as per:
    // http://stackoverflow.com/questions/1591342/#comment47830782_1591379
    if (processExitCode == STILL_ACTIVE) {
    if (! psutil_assert_pid_exists(
    pid, "iphr: GetExitCodeProcess() -> STILL_ACTIVE")) {
    return -2;
    }
    return 1;
    }
    else {
    // We can't be sure so we look into pids.
    if (psutil_pid_in_pids(pid) == 1) {
    return 1;
    }
    else {
    CloseHandle(hProcess);
    return 0;
    }
    }
    }
    CloseHandle(hProcess);
    if (! psutil_assert_pid_not_exists( pid, "iphr: exit fun")) {
    return -2;
    }
    return -1;
    }

The logic of pid_exists() changed in an almost identical manner:

int
psutil_pid_is_running(DWORD pid) {
HANDLE hProcess;
DWORD exitCode;
DWORD err;
// Special case for PID 0 System Idle Process
if (pid == 0)
return 1;
if (pid < 0)
return 0;
hProcess = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ,
FALSE, pid);
if (NULL == hProcess) {
err = GetLastError();
// Yeah, this is the actual error code in case of "no such process".
if (err == ERROR_INVALID_PARAMETER) {
if (! psutil_assert_pid_not_exists(
pid, "pir: OpenProcess() -> INVALID_PARAMETER")) {
return -1;
}
return 0;
}
// Access denied obviously means there's a process to deny access to.
else if (err == ERROR_ACCESS_DENIED) {
if (! psutil_assert_pid_exists(
pid, "pir: OpenProcess() ACCESS_DENIED")) {
return -1;
}
return 1;
}
// Be strict and raise an exception; the caller is supposed
// to take -1 into account.
else {
PyErr_SetFromWindowsErr(err);
return -1;
}
}
if (GetExitCodeProcess(hProcess, &exitCode)) {
CloseHandle(hProcess);
// XXX - maybe STILL_ACTIVE is not fully reliable as per:
// http://stackoverflow.com/questions/1591342/#comment47830782_1591379
if (exitCode == STILL_ACTIVE) {
if (! psutil_assert_pid_exists(
pid, "pir: GetExitCodeProcess() -> STILL_ACTIVE")) {
return -1;
}
return 1;
}
// We can't be sure so we look into pids.
else {
return psutil_pid_in_pids(pid);
}
}
else {
err = GetLastError();
CloseHandle(hProcess);
// Same as for OpenProcess, assume access denied means there's
// a process to deny access to.
if (err == ERROR_ACCESS_DENIED) {
if (! psutil_assert_pid_exists(
pid, "pir: GetExitCodeProcess() -> ERROR_ACCESS_DENIED")) {
return -1;
}
return 1;
}
else {
PyErr_SetFromWindowsErr(0);
return -1;
}
}
}

Read the original on github.com ↗