Merge pull request #908 from giampaolo/refactor-c-pid-exists

Refactor C psutil_pid_exists(), fixes #783 and #855

HISTORY.rst

@@ -14,6 +14,8 @@ Bug tracker at https://github.com/giampaolo/psutil/issues
 
 **Bug fixes**
 
+- #783: [OSX] Process.status() may erroneously return "running" for zombie
+  processes.
 - #798: [Windows] Process.open_files() returns and empty list on Windows 10.
 - #825: [Linux] cpu_affinity; fix possible double close and use of unopened
   socket.
@@ -25,6 +27,9 @@ Bug tracker at https://github.com/giampaolo/psutil/issues
 - #906: [BSD] disk_partitions(all=False) returned an empty list. Now the
   argument is ignored and all partitions are always returned.
 - #907: [FreeBSD] Process.exe() may fail with OSError(ENOENT).
+- #908: [OSX, BSD] different process methods could errounesuly mask the real
+  error for high-privileged PIDs and raise NoSuchProcess and AccessDenied
+  instead of OSError and RuntimeError.
 
 
 4.3.1 - 2016-09-01

psutil/_psutil_bsd.c

@@ -59,6 +59,8 @@
 #include <netinet/in.h>   // process open files/connections
 #include <sys/un.h>
 
+#include "_psutil_common.h"
+
 #ifdef __FreeBSD__
     #include "arch/bsd/freebsd.h"
     #include "arch/bsd/freebsd_socks.h"
@@ -557,9 +559,10 @@ psutil_proc_open_files(PyObject *self, PyObject *args) {
     if (psutil_kinfo_proc(pid, &kipp) == -1)
         goto error;
 
+    errno = 0;
     freep = kinfo_getfile(pid, &cnt);
     if (freep == NULL) {
-        psutil_raise_ad_or_nsp(pid);
+        psutil_raise_for_pid(pid, "kinfo_getfile() failed");
         goto error;
     }
 

psutil/_psutil_common.c

@@ -6,6 +6,11 @@
  * Routines common to all platforms.
  */
 
+#ifdef PSUTIL_POSIX
+#include <sys/types.h>
+#include <signal.h>
+#endif
+
 #include <Python.h>
 
 
@@ -35,3 +40,73 @@ AccessDenied(void) {
     Py_XDECREF(exc);
     return NULL;
 }
+
+
+#ifdef PSUTIL_POSIX
+/*
+ * Check if PID exists. Return values:
+ * 1: exists
+ * 0: does not exist
+ * -1: error (Python exception is set)
+ */
+int
+psutil_pid_exists(long pid) {
+    int ret;
+
+    // No negative PID exists, plus -1 is an alias for sending signal
+    // too all processes except system ones. Not what we want.
+    if (pid < 0)
+        return 0;
+
+    // As per "man 2 kill" PID 0 is an alias for sending the seignal to
+    // every  process in the process group of the calling process.
+    // Not what we want.
+    if (pid == 0) {
+#if defined(PSUTIL_LINUX) || defined(PSUTIL_FREEBSD)
+        // PID 0 does not exist on these platforms.
+        return 0;
+#else
+        return 1;
+#endif
+    }
+
+    ret = kill(pid , 0);
+    if (ret == 0)
+        return 1;
+    else {
+        if (errno == ESRCH)
+            return 0;
+        else if (errno == EPERM)
+            return 1;
+        else {
+            PyErr_SetFromErrno(PyExc_OSError);
+            return -1;
+        }
+    }
+}
+
+
+/*
+ * Utility used for those syscalls which do not return a meaningful
+ * error that we can translate into an exception which makes sense.
+ * As such, we'll have to guess.
+ * On UNIX, if errno is set, we return that one (OSError).
+ * Else, if PID does not exist we assume the syscall failed because
+ * of that so we raise NoSuchProcess.
+ * If none of this is true we giveup and raise RuntimeError(msg).
+ * This will always set a Python exception and return NULL.
+ */
+int
+psutil_raise_for_pid(long pid, char *msg) {
+    // Set exception to AccessDenied if pid exists else NoSuchProcess.
+    if (errno != 0) {
+        PyErr_SetFromErrno(PyExc_OSError);
+        return 0;
+    }
+    if (psutil_pid_exists(pid) == 0)
+        NoSuchProcess();
+    else
+        PyErr_SetString(PyExc_RuntimeError, msg);
+    return 0;
+}
+#endif

psutil/_psutil_common.h

@@ -8,3 +8,8 @@
 
 PyObject* AccessDenied(void);
 PyObject* NoSuchProcess(void);
+
+#ifdef PSUTIL_POSIX
+int psutil_pid_exists(long pid);
+void psutil_raise_for_pid(long pid, char *msg);
+#endif

psutil/_psutil_osx.c

@@ -66,20 +66,6 @@ psutil_sys_vminfo(vm_statistics_data_t *vmstat) {
 }
 
 
-/*
- * Set exception to AccessDenied if pid exists else NoSuchProcess.
- */
-void
-psutil_raise_ad_or_nsp(long pid) {
-    int ret;
-    ret = psutil_pid_exists(pid);
-    if (ret == 0)
-        NoSuchProcess();
-    else
-        AccessDenied();
-}
-
-
 /*
  * Return a Python list of all the PIDs running on the system.
  */
@@ -183,9 +169,10 @@ psutil_proc_exe(PyObject *self, PyObject *args) {
 
     if (! PyArg_ParseTuple(args, "l", &pid))
         return NULL;
+    errno = 0;
     ret = proc_pidpath(pid, &buf, sizeof(buf));
     if (ret == 0) {
-        psutil_raise_ad_or_nsp(pid);
+        psutil_raise_for_pid(pid, "proc_pidpath() syscall failed");
         return NULL;
     }
 #if PY_MAJOR_VERSION >= 3
@@ -323,9 +310,11 @@ psutil_proc_memory_maps(PyObject *self, PyObject *args) {
         goto error;
 
     err = task_for_pid(mach_task_self(), pid, &task);
-
     if (err != KERN_SUCCESS) {
-        psutil_raise_ad_or_nsp(pid);
+        if (psutil_pid_exists(pid) == 0)
+            NoSuchProcess();
+        else
+            AccessDenied();
         goto error;
     }
 
@@ -592,7 +581,10 @@ psutil_proc_memory_uss(PyObject *self, PyObject *args) {
 
     err = task_for_pid(mach_task_self(), pid, &task);
     if (err != KERN_SUCCESS) {
-        psutil_raise_ad_or_nsp(pid);
+        if (psutil_pid_exists(pid) == 0)
+            NoSuchProcess();
+        else
+            AccessDenied();
         return NULL;
     }
 
@@ -1039,7 +1031,10 @@ psutil_proc_threads(PyObject *self, PyObject *args) {
     // task_for_pid() requires special privileges
     err = task_for_pid(mach_task_self(), pid, &task);
     if (err != KERN_SUCCESS) {
-        psutil_raise_ad_or_nsp(pid);
+        if (psutil_pid_exists(pid) == 0)
+            NoSuchProcess();
+        else
+            AccessDenied();
         goto error;
     }
 

psutil/arch/bsd/freebsd.c

@@ -66,51 +66,6 @@ psutil_kinfo_proc(const pid_t pid, struct kinfo_proc *proc) {
 }
 
 
-/*
- * Return 1 if PID exists in the current process list, else 0, -1
- * on error.
- * TODO: this should live in _psutil_posix.c but for some reason if I
- * move it there I get a "include undefined symbol" error.
- */
-int
-psutil_pid_exists(long pid) {
-    int ret;
-
-    if (pid < 0)
-        return 0;
-    if (pid == 0)
-        return 1;
-
-    ret = kill(pid , 0);
-    if (ret == 0)
-        return 1;
-    else {
-        if (errno == ESRCH)
-            return 0;
-        else if (errno == EPERM)
-            return 1;
-        else {
-            PyErr_SetFromErrno(PyExc_OSError);
-            return -1;
-        }
-    }
-}
-
-
-
-int
-psutil_raise_ad_or_nsp(long pid) {
-    // Set exception to AccessDenied if pid exists else NoSuchProcess.
-    int ret;
-    ret = psutil_pid_exists(pid);
-    if (ret == 0)
-        NoSuchProcess();
-    else if (ret == 1)
-        AccessDenied();
-    return ret;
-}
-
-
 // remove spaces from string
 static void psutil_remove_spaces(char *str) {
     char *p1 = str;
@@ -604,9 +559,10 @@ psutil_proc_cwd(PyObject *self, PyObject *args) {
     if (psutil_kinfo_proc(pid, &kipp) == -1)
         goto error;
 
+    errno = 0;
     freep = kinfo_getfile(pid, &cnt);
     if (freep == NULL) {
-        psutil_raise_ad_or_nsp(pid);
+        psutil_raise_for_pid(pid, "kinfo_getfile() failed");
         goto error;
     }
 
@@ -656,9 +612,10 @@ psutil_proc_num_fds(PyObject *self, PyObject *args) {
     if (psutil_kinfo_proc(pid, &kipp) == -1)
         return NULL;
 
+    errno = 0;
     freep = kinfo_getfile(pid, &cnt);
     if (freep == NULL) {
-        psutil_raise_ad_or_nsp(pid);
+        psutil_raise_for_pid(pid, "kinfo_getfile() failed");
         return NULL;
     }
     free(freep);
@@ -822,9 +779,10 @@ psutil_proc_memory_maps(PyObject *self, PyObject *args) {
     if (psutil_kinfo_proc(pid, &kp) == -1)
         goto error;
 
+    errno = 0;
     freep = kinfo_getvmmap(pid, &cnt);
     if (freep == NULL) {
-        psutil_raise_ad_or_nsp(pid);
+        psutil_raise_for_pid(pid, "kinfo_getvmmap() failed");
         goto error;
     }
     for (i = 0; i < cnt; i++) {

psutil/arch/bsd/freebsd.h

@@ -10,8 +10,6 @@ typedef struct kinfo_proc kinfo_proc;
 
 int psutil_get_proc_list(struct kinfo_proc **procList, size_t *procCount);
 int psutil_kinfo_proc(const pid_t pid, struct kinfo_proc *proc);
-int psutil_pid_exists(long pid);
-int psutil_raise_ad_or_nsp(long pid);
 
 //
 PyObject* psutil_cpu_count_phys(PyObject* self, PyObject* args);

psutil/arch/bsd/freebsd_socks.c

@@ -25,7 +25,7 @@
 #include <net/if_media.h>
 #include <libutil.h>
 
-#include "freebsd.h"
+#include "../../_psutil_common.h"
 
 
 #define HASHSIZE 1009
@@ -497,9 +497,10 @@ psutil_proc_connections(PyObject *self, PyObject *args) {
         goto error;
     }
 
+    errno = 0;
     freep = kinfo_getfile(pid, &cnt);
     if (freep == NULL) {
-        psutil_raise_ad_or_nsp(pid);
+        psutil_raise_for_pid(pid, "kinfo_getfile() failed");
         goto error;
     }
 

psutil/arch/bsd/netbsd.c

@@ -39,7 +39,6 @@
 #include <arpa/inet.h>
 
 
-#include "netbsd.h"
 #include "netbsd_socks.h"
 #include "../../_psutil_common.h"
 
@@ -52,16 +51,6 @@
 // ============================================================================
 
 
-int
-psutil_raise_ad_or_nsp(long pid) {
-    // Set exception to AccessDenied if pid exists else NoSuchProcess.
-    if (psutil_pid_exists(pid) == 0)
-        NoSuchProcess();
-    else
-        AccessDenied();
-}
-
-
 int
 psutil_kinfo_proc(pid_t pid, kinfo_proc *proc) {
     // Fills a kinfo_proc struct based on process pid.
@@ -124,31 +113,6 @@ kinfo_getfile(pid_t pid, int* cnt) {
 }
 
 
-int
-psutil_pid_exists(pid_t pid) {
-    // Return 1 if PID exists in the current process list, else 0, -1
-    // on error.
-    // TODO: this should live in _psutil_posix.c but for some reason if I
-    // move it there I get a "include undefined symbol" error.
-    int ret;
-    if (pid < 0)
-        return 0;
-    ret = kill(pid , 0);
-    if (ret == 0)
-        return 1;
-    else {
-        if (ret == ESRCH)
-            return 0;
-        else if (ret == EPERM)
-            return 1;
-        else {
-            PyErr_SetFromErrno(PyExc_OSError);
-            return -1;
-        }
-    }
-}
-
-
 // XXX: This is no longer used as per
 // https://github.com/giampaolo/psutil/pull/557#issuecomment-171912820
 // Current implementation uses /proc instead.
@@ -549,9 +513,10 @@ psutil_proc_num_fds(PyObject *self, PyObject *args) {
     if (! PyArg_ParseTuple(args, "l", &pid))
         return NULL;
 
+    errno = 0;
     freep = kinfo_getfile(pid, &cnt);
     if (freep == NULL) {
-        psutil_raise_ad_or_nsp(pid);
+        psutil_raise_for_pid(pid, "kinfo_getfile() failed");
         return NULL;
     }
     free(freep);