On Thu, Jul 27 2017, Klemens Nanni <k...@posteo.org> wrote:
> Only main() calls pr_args() in L330 with ep->kp as argument which in
> turn is set in L257 or L266 for every utmp entry. kp is checked against
> NULL already in L229.
>
> Even if kp was somehow NULL chances are high we'd fail before pr_args()
> was called anyway since L244, L256 and L265 would then cause a NULL pointer
> dereference. In fact, proc_compare() even expects its second argument to
> to be not NULL (only the first one is checked explicitly).
>
> Not that crashing is guaranteed upon undefined behaviour but noone
> seems to have reported failure within the last 13 years, so I think it's
> safe to remove that check.
>
> Feedback? Comments?

I don't think this is correct.  ep->kp is set only if we find a matching
live process.  If we don't, the diff results in a crash.  I don't know
all the details of utmp handling, but I really doubt that we can assume
a 1->1 mapping between utmp entries and live processes (stale entries,
race conditions, etc).

Here's an example with a stale ssh entry:

ritchie /usr/src/usr.bin/w$ w
 1:59PM  up 58 mins, 2 users, load averages: 0.27, 0.16, 0.14
USER    TTY FROM              LOGIN@  IDLE WHAT
jca      p0 :0                1:03PM     0 tmux: client (/tmp/tmux-1000/default)
jca      pg 127.0.0.1         1:57PM     1 -
ritchie /usr/src/usr.bin/w$ ./obj/w
 1:59PM  up 58 mins, 2 users, load averages: 0.21, 0.16, 0.13
USER    TTY FROM              LOGIN@  IDLE WHAT
jca      p0 :0                1:03PM     0 tmux: client (/tmp/tmux-1000/default)
Segmentation fault (core dumped)


So the answer is "yes, this can happen".

Index: w.c
===================================================================
RCS file: /d/cvs/src/usr.bin/w/w.c,v
retrieving revision 1.62
diff -u -p -p -u -r1.62 w.c
--- w.c 30 May 2017 15:10:48 -0000      1.62
+++ w.c 27 Jul 2017 11:59:27 -0000
@@ -384,7 +384,7 @@ pr_args(struct kinfo_proc *kp)
        int left;
 
        if (kp == NULL)
-               goto nothing;           /* XXX - can this happen? */
+               goto nothing;           /* no matching process found */
        left = argwidth;
        argv = kvm_getargv(kd, kp, argwidth+60);  /* +60 for ftpd snip */
        if (argv == NULL)


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply via email to