Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Otto Moerbeek]

On Thu, May 03, 2018 at 07:15:24PM +0300, Vadim Zhukov wrote:

> 2018-05-03 18:59 GMT+03:00 Otto Moerbeek :
> > Yes, looks good from reading. But all te extra checks before calling
> > free can go. That's idiom from a *long* time ago.
> 
> Like that? I've checked all free() calls in libkvm.
> 
> I've also added zeroing of vmst field in mips64 code, like it's done
> for other archs.

ok,

-Otto

> 
> --
> WBR,
>   Vadim Zhukov
> 
> 
> Index: kvm.c
> ===
> RCS file: /cvs/src/lib/libkvm/kvm.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 kvm.c
> --- kvm.c 3 May 2018 15:47:41 -   1.64
> +++ kvm.c 3 May 2018 16:08:37 -
> @@ -649,27 +649,18 @@ kvm_close(kvm_t *kd)
>   if (kd->vmst)
>   _kvm_freevtop(kd);
>   kd->cpu_dsize = 0;
> - if (kd->cpu_data != NULL)
> - free((void *)kd->cpu_data);
> - if (kd->kcore_hdr != NULL)
> - free((void *)kd->kcore_hdr);
> + free(kd->cpu_data);
> + free(kd->kcore_hdr);
>   free(kd->filebase);
>   free(kd->procbase);
> - if (kd->swapspc != 0)
> - free((void *)kd->swapspc);
> - if (kd->argspc != 0)
> - free((void *)kd->argspc);
> - if (kd->argbuf != 0)
> - free((void *)kd->argbuf);
> - if (kd->argv != 0)
> - free((void *)kd->argv);
> - if (kd->envspc != 0)
> - free((void *)kd->envspc);
> - if (kd->envbuf != 0)
> - free((void *)kd->envbuf);
> - if (kd->envp != 0)
> - free((void *)kd->envp);
> - free((void *)kd);
> + free(kd->swapspc);
> + free(kd->argspc);
> + free(kd->argbuf);
> + free(kd->argv);
> + free(kd->envspc);
> + free(kd->envbuf);
> + free(kd->envp);
> + free(kd);
>  
>   return (error);
>  }
> Index: kvm_mips64.c
> ===
> RCS file: /cvs/src/lib/libkvm/kvm_mips64.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 kvm_mips64.c
> --- kvm_mips64.c  9 Feb 2015 22:23:47 -   1.15
> +++ kvm_mips64.c  3 May 2018 16:08:37 -
> @@ -71,8 +71,8 @@ struct vmstate {
>  void
>  _kvm_freevtop(kvm_t *kd)
>  {
> - if (kd->vmst != 0)
> - free(kd->vmst);
> + free(kd->vmst);
> + kd->vmst = NULL;
>  }
>  
>  int

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Vadim Zhukov]

2018-05-03 18:59 GMT+03:00 Otto Moerbeek :
> Yes, looks good from reading. But all te extra checks before calling
> free can go. That's idiom from a *long* time ago.

Like that? I've checked all free() calls in libkvm.

I've also added zeroing of vmst field in mips64 code, like it's done
for other archs.

--
WBR,
  Vadim Zhukov


Index: kvm.c
===
RCS file: /cvs/src/lib/libkvm/kvm.c,v
retrieving revision 1.64
diff -u -p -r1.64 kvm.c
--- kvm.c   3 May 2018 15:47:41 -   1.64
+++ kvm.c   3 May 2018 16:08:37 -
@@ -649,27 +649,18 @@ kvm_close(kvm_t *kd)
if (kd->vmst)
_kvm_freevtop(kd);
kd->cpu_dsize = 0;
-   if (kd->cpu_data != NULL)
-   free((void *)kd->cpu_data);
-   if (kd->kcore_hdr != NULL)
-   free((void *)kd->kcore_hdr);
+   free(kd->cpu_data);
+   free(kd->kcore_hdr);
free(kd->filebase);
free(kd->procbase);
-   if (kd->swapspc != 0)
-   free((void *)kd->swapspc);
-   if (kd->argspc != 0)
-   free((void *)kd->argspc);
-   if (kd->argbuf != 0)
-   free((void *)kd->argbuf);
-   if (kd->argv != 0)
-   free((void *)kd->argv);
-   if (kd->envspc != 0)
-   free((void *)kd->envspc);
-   if (kd->envbuf != 0)
-   free((void *)kd->envbuf);
-   if (kd->envp != 0)
-   free((void *)kd->envp);
-   free((void *)kd);
+   free(kd->swapspc);
+   free(kd->argspc);
+   free(kd->argbuf);
+   free(kd->argv);
+   free(kd->envspc);
+   free(kd->envbuf);
+   free(kd->envp);
+   free(kd);
 
return (error);
 }
Index: kvm_mips64.c
===
RCS file: /cvs/src/lib/libkvm/kvm_mips64.c,v
retrieving revision 1.15
diff -u -p -r1.15 kvm_mips64.c
--- kvm_mips64.c9 Feb 2015 22:23:47 -   1.15
+++ kvm_mips64.c3 May 2018 16:08:37 -
@@ -71,8 +71,8 @@ struct vmstate {
 void
 _kvm_freevtop(kvm_t *kd)
 {
-   if (kd->vmst != 0)
-   free(kd->vmst);
+   free(kd->vmst);
+   kd->vmst = NULL;
 }
 
 int

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Todd C. Miller]

On Thu, 03 May 2018 17:59:39 +0200, Otto Moerbeek wrote:

> Yes, looks good from reading. But all te extra checks before calling
> free can go. That's idiom from a *long* time ago.

There is more cleanup that can be done in this code.  For example,
the use of 0 instead of NULL.  But that can be a separate commit.
 
 - todd

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Otto Moerbeek]

On Thu, May 03, 2018 at 09:19:01AM -0600, Todd C. Miller wrote:

> On Thu, 03 May 2018 13:58:35 +0300, Vadim Zhukov wrote:
> 
> > Here is patch for libkvm that fixes a few memory handling problems.
> > Most changes are mechanical, with some exceptions:
> >
> >   1. Most notable: this splits argv buffer into argv-specific one
> >  and environ-specific one. This makes ps -eww totally happy.
> >
> >   2. realloc() usage in kvm_argv() is now ENOMEM-prone. This is
> >  actually the same change as in one of the patches I've sent
> >  earlier, since that other patch heavily conflicts with this one.
> >
> >   3. The "int off" changed to "ptrdiff_t off", as it should be.
> >  If I understand things correctly, we're lucky that this didn't
> >  strike us on 64-bit archs yet. The  is needed only
> >  for the ptrdiff_t.
> 
> Looks good, OK millert@
> 
>  - todd

Yes, looks good from reading. But all te extra checks before calling
free can go. That's idiom from a *long* time ago.

-Otto

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Todd C. Miller]

On Thu, 03 May 2018 13:58:35 +0300, Vadim Zhukov wrote:

> Here is patch for libkvm that fixes a few memory handling problems.
> Most changes are mechanical, with some exceptions:
>
>   1. Most notable: this splits argv buffer into argv-specific one
>  and environ-specific one. This makes ps -eww totally happy.
>
>   2. realloc() usage in kvm_argv() is now ENOMEM-prone. This is
>  actually the same change as in one of the patches I've sent
>  earlier, since that other patch heavily conflicts with this one.
>
>   3. The "int off" changed to "ptrdiff_t off", as it should be.
>  If I understand things correctly, we're lucky that this didn't
>  strike us on 64-bit archs yet. The  is needed only
>  for the ptrdiff_t.

Looks good, OK millert@

 - todd

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Vadim Zhukov]

2018-05-02 16:54 GMT+03:00 Todd C. Miller :
> On Tue, 01 May 2018 13:35:54 -0600, "Theo de Raadt" wrote:
>> >   b) Their working space should be independent of each other. This
>> >  isn't hard, just splitting kd->argbuf into kd->argbuf and
>> >  kd->envbuf. Seems a bit saner.
>>
>> I think (b) would be the better solution, this seems very fragile.
>>
>> Todd and Guenther -- what do you think?
>
> I'd prefer separate buffer spaces as well, the current situation is
> fragile as we've seen.

Here is patch for libkvm that fixes a few memory handling problems.
Most changes are mechanical, with some exceptions:

  1. Most notable: this splits argv buffer into argv-specific one
 and environ-specific one. This makes ps -eww totally happy.

  2. realloc() usage in kvm_argv() is now ENOMEM-prone. This is
 actually the same change as in one of the patches I've sent
 earlier, since that other patch heavily conflicts with this one.

  3. The "int off" changed to "ptrdiff_t off", as it should be.
 If I understand things correctly, we're lucky that this didn't
 strike us on 64-bit archs yet. The  is needed only
 for the ptrdiff_t.

If required, I'll split (1), (2) and (3) in separate commits; it's
that reviewing them in a single mail and a single context looks like
easier for me; please correct me if I'm wrong... okay, okay, I'm just
a lazy guy.

--
WBR,
  Vadim Zhukov


Index: kvm.c
===
RCS file: /cvs/src/lib/libkvm/kvm.c,v
retrieving revision 1.63
diff -u -p -r1.63 kvm.c
--- kvm.c   14 Dec 2017 17:06:33 -  1.63
+++ kvm.c   3 May 2018 10:42:51 -
@@ -191,6 +191,9 @@ _kvm_open(kvm_t *kd, const char *uf, con
kd->argspc = 0;
kd->argbuf = 0;
kd->argv = 0;
+   kd->envspc = 0;
+   kd->envbuf = 0;
+   kd->envp = 0;
kd->vmst = NULL;
kd->vm_page_buckets = 0;
kd->kcore_hdr = 0;
@@ -660,6 +663,12 @@ kvm_close(kvm_t *kd)
free((void *)kd->argbuf);
if (kd->argv != 0)
free((void *)kd->argv);
+   if (kd->envspc != 0)
+   free((void *)kd->envspc);
+   if (kd->envbuf != 0)
+   free((void *)kd->envbuf);
+   if (kd->envp != 0)
+   free((void *)kd->envp);
free((void *)kd);
 
return (error);
Index: kvm_private.h
===
RCS file: /cvs/src/lib/libkvm/kvm_private.h,v
retrieving revision 1.25
diff -u -p -r1.25 kvm_private.h
--- kvm_private.h   14 Dec 2017 17:06:33 -  1.25
+++ kvm_private.h   3 May 2018 10:42:51 -
@@ -53,10 +53,16 @@ struct __kvm {
struct kinfo_file *filebase;
int nbpg;   /* page size */
char*swapspc;   /* (dynamic) storage for swapped pages */
+
char*argspc, *argbuf; /* (dynamic) storage for argv strings */
int arglen; /* length of the above */
char**argv; /* (dynamic) storage for argv pointers */
int argc;   /* length of above (not actual # present) */
+
+   char*envspc, *envbuf; /* (dynamic) storage for environ strings */
+   int envlen; /* length of the above */
+   char**envp; /* (dynamic) storage for environ pointers */
+   int envc;   /* length of above (not actual # present) */
 
/*
 * Header structures for kernel dumps. Only gets filled in for
Index: kvm_proc.c
===
RCS file: /cvs/src/lib/libkvm/kvm_proc.c,v
retrieving revision 1.58
diff -u -p -r1.58 kvm_proc.c
--- kvm_proc.c  7 Nov 2016 00:26:33 -   1.58
+++ kvm_proc.c  3 May 2018 10:42:51 -
@@ -76,6 +76,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -100,9 +101,9 @@
 static char*_kvm_ureadm(kvm_t *, const struct kinfo_proc *, u_long, u_long 
*);
 static ssize_t kvm_ureadm(kvm_t *, const struct kinfo_proc *, u_long, char *, 
size_t);
 
-static char**kvm_argv(kvm_t *, const struct kinfo_proc *, u_long, int, 
int);
+static char**kvm_argv(kvm_t *, const struct kinfo_proc *, u_long, int, 
int, int);
 
-static char**kvm_doargv(kvm_t *, const struct kinfo_proc *, int,
+static char**kvm_doargv(kvm_t *, const struct kinfo_proc *, int, int,
void (*)(struct ps_strings *, u_long *, int *));
 static int proc_verify(kvm_t *, const struct kinfo_proc *);
 static voidps_str_a(struct ps_strings *, u_long *, int *);
@@ -257,11 +258,12 @@ _kvm_reallocarray(kvm_t *kd, void *p, si
  */
 static char **
 kvm_argv(kvm_t *kd, const struct kinfo_proc *p, u_long addr, int narg,
-int maxcnt)
+int maxcnt, int isenv)
 {
-   char *np, *cp, *ep, *ap, **argv;
+   char *np, *cp, *ep, *ap, **argv, ***pargv, **pargspc, **pargbuf;
u_long oaddr = -1;
-   int 

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Todd C. Miller]

On Tue, 01 May 2018 13:35:54 -0600, "Theo de Raadt" wrote:

> >   b) Their working space should be independent of each other. This
> >  isn't hard, just splitting kd->argbuf into kd->argbuf and
> >  kd->envbuf. Seems a bit saner.
> >
>
> I think (b) would be the better solution, this seems very fragile.
>
> Todd and Guenther -- what do you think?

I'd prefer separate buffer spaces as well, the current situation is
fragile as we've seen.

 - todd

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Theo de Raadt]

Vadim Zhukov  wrote:

> 2018-05-01 21:53 GMT+03:00 Theo de Raadt :
> > ktrace makes the problem more clear:
> > 
> >  25908 ps   CALL  
> > sysctl(1.55.75675.1,0xed0cc78,0x7f7cd3d8,0,0)
> >  25908 ps   RET   sysctl -1 errno 14 Bad address
> 
> And that's it, thanks!
> 
> Now little ps(1) is happy. But now there's a question, about
> kvm_getargv() and kvm_getenv(): what behaviour do we want?
> 
>   a) They use same space, overwriting each other results (this is what
>  happens now, and noone complains).
>
>   b) Their working space should be independent of each other. This
>  isn't hard, just splitting kd->argbuf into kd->argbuf and
>  kd->envbuf. Seems a bit saner.
>
> I'm fine with any direction. The patch below implements (a), since
> it's less patching. Is it okay, or should it be (b)?

I think (b) would be the better solution, this seems very fragile.

Todd and Guenther -- what do you think?

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Vadim Zhukov]

2018-05-01 21:53 GMT+03:00 Theo de Raadt :
> ktrace makes the problem more clear:
> 
>  25908 ps   CALL  
> sysctl(1.55.75675.1,0xed0cc78,0x7f7cd3d8,0,0)
>  25908 ps   RET   sysctl -1 errno 14 Bad address

And that's it, thanks!

Now little ps(1) is happy. But now there's a question, about
kvm_getargv() and kvm_getenv(): what behaviour do we want?

  a) They use same space, overwriting each other results (this is what
 happens now, and noone complains).

  b) Their working space should be independent of each other. This
 isn't hard, just splitting kd->argbuf into kd->argbuf and
 kd->envbuf. Seems a bit saner.

I'm fine with any direction. The patch below implements (a), since
it's less patching. Is it okay, or should it be (b)?

--
WBR,
  Vadim Zhukov


Index: kvm_proc.c
===
RCS file: /cvs/src/lib/libkvm/kvm_proc.c,v
retrieving revision 1.58
diff -u -p -r1.58 kvm_proc.c
--- kvm_proc.c  7 Nov 2016 00:26:33 -   1.58
+++ kvm_proc.c  1 May 2018 19:23:01 -
@@ -458,12 +458,14 @@ kvm_arg_sysctl(kvm_t *kd, pid_t pid, int
 {
size_t len, orglen;
int mib[4], ret;
-   char *buf;
+   void *buf;
 
orglen = env ? kd->nbpg : 8 * kd->nbpg; /* XXX - should be ARG_MAX */
-   if (kd->argbuf == NULL &&
-   (kd->argbuf = _kvm_malloc(kd, orglen)) == NULL)
-   return (NULL);
+
+   buf = _kvm_realloc(kd, kd->argbuf, orglen);
+   if (buf == NULL)
+   return NULL;
+   kd->argbuf = buf;
 
 again:
mib[0] = CTL_KERN;

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Theo de Raadt]

ktrace makes the problem more clear:

 25908 ps   CALL  
sysctl(1.55.75675.1,0xed0cc78,0x7f7cd3d8,0,0)
 25908 ps   RET   sysctl -1 errno 14 Bad address

libkvm requires kvm_getargv before kvm_getenv when both needed

[Vadim Zhukov]

Hi all.

So I finally got bored of ps not displaying command args when "-e" is
present. Yes, ps(1) is broken: compare end of lines in output of "ps
-ww" and "ps -eww". And IIRC it behaves this way long enough, but I
always thought that it's me not missing something in ps(1) manual. Bad
zhuk@.

This is not a ps(1) bug, though: the simple diff below "fixes" it.
Yep, calling kvm_getargv(3) before kvm_getenv(3) makes everyone happy
again.

I've tried to dive into libkvm but went out of oxygen. The only
problem I found was misuse of reallocarray(), to be addressed in
another letter.

So, does any libkvm hacker have any clues where to look for this
argv-envp bug? I'm not sure that I'll find the root issue myself fast
enough (in less than half a year).

--
  WBR,
  Vadim Zhukov


Index: print.c
===
RCS file: /cvs/src/bin/ps/print.c,v
retrieving revision 1.69
diff -u -p -r1.69 print.c
--- print.c 8 Sep 2016 15:11:29 -   1.69
+++ print.c 1 May 2018 18:29:52 -
@@ -118,6 +118,7 @@ command(const struct kinfo_proc *kp, VAR
left = INT_MAX;

if (needenv && kd != NULL) {
+   argv = kvm_getargv(kd, kp, termwidth);
argv = kvm_getenvv(kd, kp, termwidth);
if ((p = argv) != NULL) {
while (*p) {