Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: fix wrapper around OpenProcess (pid_exists() no longer lies) #1094

Merged
merged 14 commits into from
May 28, 2017

Conversation

giampaolo
Copy link
Owner

@giampaolo giampaolo commented May 20, 2017

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;
}
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant