proc/readproc.c: Work around a design flaw in readeither().

readeither() caches (in new_p) a pointer to the proc_t of a task-group
leader, but readeither()'s callers can do pretty much anything with the
proc_t structure passed to and/or returned by this function. For
example, they can 1/ free it or 2/ recycle it (by passing it to
readeither() as x).

1/ leads to a use-after-free, and 2/ leads to unexpected behavior when
taskreader()/simple_readtask() is called with new_p equal to x (this is
not a theoretical flaw: 2/ happens in readproctab3() when want_task()
returns false and p is a group leader).

As a workaround, we keep a copy of new_p's first member (tid) in static
storage, and the next times we enter readeither() we check this "canary"
against the tid in new_p: if they differ, we reset new_p to NULL, which
forces the allocation of a new proc_t (the new "leader", or reference).

This always detects 2/ (because free_acquired(x,1) memsets x and hence
new_p); always detects 1/ if freed via free_acquired() and/or freeproc()
(very likely, otherwise memory may be leaked); probably detects 1/ even
if freed directly via free() (because the canary is the first member of
proc_t, likely to be overwritten by free()); but can not detect 1/ if
free() does not write to new_p's chunk at all.

Moreover, accessing new_p->tid to check the canary in case 1/ is itself
a use-after-free, so a better long-term solution should be implemented
at some point (we wanted to avoid intrusive and backward-incompatible
changes in this library function, hence this imperfect workaround).

diff --git a/proc/readproc.c b/proc/readproc.c
index f8561c1e9d26e66ffabadc987dc5e7eb494577a7..0f002315ecaa6e150d32736ec45671548a66f79b 100644 (file)
--- a/proc/readproc.c
+++ b/proc/readproc.c
@@ -1383,13 +1383,17 @@ out:
 proc_t* readeither (PROCTAB *restrict const PT, proc_t *restrict x) {
     static proc_t skel_p;    // skeleton proc_t, only uses tid + tgid
     static proc_t *new_p;    // for process/task transitions
+    static int canary;
     char path[PROCPATHLEN];
     proc_t *saved_x, *ret;
 
     saved_x = x;
     if (!x) x = xcalloc(sizeof(*x));
     else free_acquired(x,1);
-    if (new_p) goto next_task;
+    if (new_p) {
+        if (new_p->tid != canary) new_p = NULL;
+        goto next_task;
+    }
 
 next_proc:
     new_p = NULL;
@@ -1406,7 +1410,10 @@ next_task:
     || (!(ret = PT->taskreader(PT,new_p,x,path)))) {       // simple_readtask
         goto next_proc;
     }
-    if (!new_p) new_p = ret;
+    if (!new_p) {
+        new_p = ret;
+        canary = new_p->tid;
+    }
     return ret;
 
 end_procs: