Re: fix regress in pstat.c

2016-09-17 Thread Rob Pierce
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(_head); tp;
> 898   tp = TAILQ_NEXT(, tty_link)) {
> 899   KGET2(tp, , sizeof tty, "tty struct");
> 900   tty2itty(, );
> 901   ttyprt();
> 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

2016-09-17 Thread Theo Buehler
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(_head); tp;
898 tp = TAILQ_NEXT(, tty_link)) {
899 KGET2(tp, , sizeof tty, "tty struct");
900 tty2itty(, );
901 ttyprt();
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;