Re: fix regress in pstat.c
On Sat, Sep 17, 2016 at 09:20:31PM +0200, Theo Buehler wrote: > While this patch avoids the bug, it isn't quite right: pstat -ft should > be equivalent to "pstat -f && pstat -t" but... > > $ pstat -ft > pstat: kvm_openfiles: /dev/mem: Permission denied > > The actual problem is in ttymode(): if kd != NULL (which is the case if > -f is combined with -t), there are accesses of the kvm datastructures > without them being properly initialized: > > 879 if (kd != NULL) > 880 KGET(TTY_NTTY, ntty); > > and > > 896 KGET(TTY_TTYLIST, tty_head); > 897 for (tp = TAILQ_FIRST(&tty_head); tp; > 898 tp = TAILQ_NEXT(&tty, tty_link)) { > 899 KGET2(tp, &tty, sizeof tty, "tty struct"); > 900 tty2itty(&tty, &itty); > 901 ttyprt(&itty); > 902 } > > These code paths must not be hit unless need_nlist is true, so expose it > globally and replace 'kd != NULL' with the proper condition need_nlist > > The following patch works in all affected cases, pstat -t, pstat -f, > pstat -tf, both as root and non-root as well as pstat -tv as root > (combinations with -d aren't interesting since pstat exits before > doing anything else but -d). > > Index: pstat.c > === > RCS file: /var/cvs/src/usr.sbin/pstat/pstat.c,v > retrieving revision 1.108 > diff -u -p -r1.108 pstat.c > --- pstat.c 14 Aug 2016 22:47:26 - 1.108 > +++ pstat.c 17 Sep 2016 17:57:38 - > @@ -89,6 +89,7 @@ struct e_vnode { > struct vnode vnode; > }; > > +int need_nlist; > int usenumflag; > int totalflag; > int kflag; > @@ -141,7 +142,7 @@ main(int argc, char *argv[]) > const char *dformat = NULL; > extern char *optarg; > extern int optind; > - int i, need_nlist; > + int i; > > hideroot = getuid(); > > @@ -869,7 +870,7 @@ ttymode(void) > struct itty itty, *itp; > size_t nlen; > > - if (kd == 0) { > + if (!need_nlist) { > mib[0] = CTL_KERN; > mib[1] = KERN_TTYCOUNT; > nlen = sizeof(ntty); > @@ -879,7 +880,7 @@ ttymode(void) > KGET(TTY_NTTY, ntty); > (void)printf("%d terminal device%s\n", ntty, ntty == 1 ? "" : "s"); > (void)printf("%s", hdr); > - if (kd == 0) { > + if (!need_nlist) { > mib[0] = CTL_KERN; > mib[1] = KERN_TTY; > mib[2] = KERN_TTY_INFO; > Thanks Theo. Fixes the regress and works well with your pending pledge diff. Rob
Re: fix regress in pstat.c
While this patch avoids the bug, it isn't quite right: pstat -ft should be equivalent to "pstat -f && pstat -t" but... $ pstat -ft pstat: kvm_openfiles: /dev/mem: Permission denied The actual problem is in ttymode(): if kd != NULL (which is the case if -f is combined with -t), there are accesses of the kvm datastructures without them being properly initialized: 879 if (kd != NULL) 880 KGET(TTY_NTTY, ntty); and 896 KGET(TTY_TTYLIST, tty_head); 897 for (tp = TAILQ_FIRST(&tty_head); tp; 898 tp = TAILQ_NEXT(&tty, tty_link)) { 899 KGET2(tp, &tty, sizeof tty, "tty struct"); 900 tty2itty(&tty, &itty); 901 ttyprt(&itty); 902 } These code paths must not be hit unless need_nlist is true, so expose it globally and replace 'kd != NULL' with the proper condition need_nlist The following patch works in all affected cases, pstat -t, pstat -f, pstat -tf, both as root and non-root as well as pstat -tv as root (combinations with -d aren't interesting since pstat exits before doing anything else but -d). Index: pstat.c === RCS file: /var/cvs/src/usr.sbin/pstat/pstat.c,v retrieving revision 1.108 diff -u -p -r1.108 pstat.c --- pstat.c 14 Aug 2016 22:47:26 - 1.108 +++ pstat.c 17 Sep 2016 17:57:38 - @@ -89,6 +89,7 @@ struct e_vnode { struct vnode vnode; }; +intneed_nlist; intusenumflag; inttotalflag; intkflag; @@ -141,7 +142,7 @@ main(int argc, char *argv[]) const char *dformat = NULL; extern char *optarg; extern int optind; - int i, need_nlist; + int i; hideroot = getuid(); @@ -869,7 +870,7 @@ ttymode(void) struct itty itty, *itp; size_t nlen; - if (kd == 0) { + if (!need_nlist) { mib[0] = CTL_KERN; mib[1] = KERN_TTYCOUNT; nlen = sizeof(ntty); @@ -879,7 +880,7 @@ ttymode(void) KGET(TTY_NTTY, ntty); (void)printf("%d terminal device%s\n", ntty, ntty == 1 ? "" : "s"); (void)printf("%s", hdr); - if (kd == 0) { + if (!need_nlist) { mib[0] = CTL_KERN; mib[1] = KERN_TTY; mib[2] = KERN_TTY_INFO;
fix regress in pstat.c
ttymode() needs nlist, otherwise "pstat -tf" will fail since kd will not be NULL and the calls from ttymode() to KGET will error as follows: pstat: cannot read ntty: invalid address (0) pstat: cannot read tty_head: invalid address (0) KGET(TTY_NTTY, ntty) and KGET(TTY_TTYLIST, tty_head) both result in a kvm_read() with an address (globalnl[TTY_NTTY].n_value) of zero. The following diff resolves this issue. Took the opportunity to reorder the flags to match the switch statement. With help from tb@. Rob Index: pstat.c === RCS file: /cvs/src/usr.sbin/pstat/pstat.c,v retrieving revision 1.108 diff -u -p -r1.108 pstat.c --- pstat.c 14 Aug 2016 22:47:26 - 1.108 +++ pstat.c 17 Sep 2016 17:00:28 - @@ -190,7 +190,7 @@ main(int argc, char *argv[]) if ((dformat == NULL && argc > 0) || (dformat && argc == 0)) usage(); - need_nlist = vnodeflag || dformat; + need_nlist = dformat || (fileflag && ttyflag) || vnodeflag; if (nlistf != NULL || memf != NULL) { if (fileflag || totalflag)