libkvm requires kvm_getargv before kvm_getenv when both needed

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

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 -0000       1.69
+++ print.c     1 May 2018 18:29:52 -0000
@@ -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) {

Reply | Threaded
Open this post in threaded view
|

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

Theo de Raadt-2
ktrace makes the problem more clear:

 25908 ps       CALL  sysctl(1.55.75675.1<kern.procargs.75675.1>,0xed0cc780000,0x7f7ffffcd3d8,0,0)
 25908 ps       RET   sysctl -1 errno 14 Bad address

Reply | Threaded
Open this post in threaded view
|

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

Vadim Zhukov
In reply to this post by Vadim Zhukov
2018-05-01 21:53 GMT+03:00 Theo de Raadt <[hidden email]>:
> ktrace makes the problem more clear:
>
>  25908 ps       CALL  sysctl(1.55.75675.1<kern.procargs.75675.1>,0xed0cc780000,0x7f7ffffcd3d8,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 -0000 1.58
+++ kvm_proc.c 1 May 2018 19:23:01 -0000
@@ -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;

Reply | Threaded
Open this post in threaded view
|

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

Theo de Raadt-2
Vadim Zhukov <[hidden email]> wrote:

> 2018-05-01 21:53 GMT+03:00 Theo de Raadt <[hidden email]>:
> > ktrace makes the problem more clear:
> >
> >  25908 ps       CALL  sysctl(1.55.75675.1<kern.procargs.75675.1>,0xed0cc780000,0x7f7ffffcd3d8,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?

Reply | Threaded
Open this post in threaded view
|

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

Todd C. Miller-2
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

Reply | Threaded
Open this post in threaded view
|

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

Vadim Zhukov
In reply to this post by Vadim Zhukov
2018-05-02 16:54 GMT+03:00 Todd C. Miller <[hidden email]>:

> 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 <stddef> 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 -0000 1.63
+++ kvm.c 3 May 2018 10:42:51 -0000
@@ -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 -0000 1.25
+++ kvm_private.h 3 May 2018 10:42:51 -0000
@@ -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 -0000 1.58
+++ kvm_proc.c 3 May 2018 10:42:51 -0000
@@ -76,6 +76,7 @@
 #include <sys/stat.h>
 #include <sys/ioctl.h>
 #include <sys/tty.h>
+#include <stddef.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -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 void ps_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 len, cc;
+ int len, cc, *parglen, *pargc;
+ size_t argc;
 
  /*
  * Check that there aren't an unreasonable number of arguments,
@@ -270,77 +272,88 @@ kvm_argv(kvm_t *kd, const struct kinfo_p
  if (narg > ARG_MAX || addr < VM_MIN_ADDRESS || addr >= VM_MAXUSER_ADDRESS)
  return (0);
 
- if (kd->argv == 0) {
- /*
- * Try to avoid reallocs.
- */
- kd->argc = MAX(narg + 1, 32);
- kd->argv = _kvm_reallocarray(kd, NULL, kd->argc,
-    sizeof(*kd->argv));
- if (kd->argv == 0)
- return (0);
- } else if (narg + 1 > kd->argc) {
- kd->argc = MAX(2 * kd->argc, narg + 1);
- kd->argv = (char **)_kvm_reallocarray(kd, kd->argv, kd->argc,
-    sizeof(*kd->argv));
- if (kd->argv == 0)
- return (0);
+ if (isenv) {
+ pargspc = &kd->envspc;
+ pargbuf = &kd->envbuf;
+ parglen = &kd->envlen;
+ pargv = &kd->envp;
+ pargc = &kd->envc;
+ } else {
+ pargspc = &kd->argspc;
+ pargbuf = &kd->argbuf;
+ parglen = &kd->arglen;
+ pargv = &kd->argv;
+ pargc = &kd->argc;
  }
- if (kd->argspc == 0) {
- kd->argspc = _kvm_malloc(kd, kd->nbpg);
- if (kd->argspc == 0)
+
+ if (*pargv == 0)
+ argc = MAX(narg + 1, 32);
+ else if (narg + 1 > *pargc)
+ argc = MAX(2 * (*pargc), narg + 1);
+ else
+ goto argv_allocated;
+ argv = _kvm_reallocarray(kd, *pargv, argc, sizeof(**pargv));
+ if (argv == 0)
+ return (0);
+ *pargv = argv;
+ *pargc = argc;
+
+argv_allocated:
+ if (*pargspc == 0) {
+ *pargspc = _kvm_malloc(kd, kd->nbpg);
+ if (*pargspc == 0)
  return (0);
- kd->arglen = kd->nbpg;
+ *parglen = kd->nbpg;
  }
- if (kd->argbuf == 0) {
- kd->argbuf = _kvm_malloc(kd, kd->nbpg);
- if (kd->argbuf == 0)
+ if (*pargbuf == 0) {
+ *pargbuf = _kvm_malloc(kd, kd->nbpg);
+ if (*pargbuf == 0)
  return (0);
  }
  cc = sizeof(char *) * narg;
- if (kvm_ureadm(kd, p, addr, (char *)kd->argv, cc) != cc)
+ if (kvm_ureadm(kd, p, addr, (char *)*pargv, cc) != cc)
  return (0);
- ap = np = kd->argspc;
- argv = kd->argv;
+ ap = np = *pargspc;
+ argv = *pargv;
  len = 0;
 
  /*
  * Loop over pages, filling in the argument vector.
  */
- while (argv < kd->argv + narg && *argv != 0) {
+ while (argv < *pargv + narg && *argv != 0) {
  addr = (u_long)*argv & ~(kd->nbpg - 1);
  if (addr != oaddr) {
- if (kvm_ureadm(kd, p, addr, kd->argbuf, kd->nbpg) !=
+ if (kvm_ureadm(kd, p, addr, *pargbuf, kd->nbpg) !=
     kd->nbpg)
  return (0);
  oaddr = addr;
  }
  addr = (u_long)*argv & (kd->nbpg - 1);
- cp = kd->argbuf + addr;
+ cp = *pargbuf + addr;
  cc = kd->nbpg - addr;
  if (maxcnt > 0 && cc > maxcnt - len)
  cc = maxcnt - len;
  ep = memchr(cp, '\0', cc);
  if (ep != 0)
  cc = ep - cp + 1;
- if (len + cc > kd->arglen) {
- int off;
+ if (len + cc > *parglen) {
+ ptrdiff_t off;
  char **pp;
- char *op = kd->argspc;
+ char *op = *pargspc;
  char *newp;
 
- newp = _kvm_reallocarray(kd, kd->argspc,
-    kd->arglen, 2);
+ newp = _kvm_reallocarray(kd, *pargspc,
+    *parglen, 2);
  if (newp == 0)
  return (0);
- kd->argspc = newp;
- kd->arglen *= 2;
+ *pargspc = newp;
+ *parglen *= 2;
  /*
  * Adjust argv pointers in case realloc moved
  * the string space.
  */
- off = kd->argspc - op;
- for (pp = kd->argv; pp < argv; pp++)
+ off = *pargspc - op;
+ for (pp = *pargv; pp < argv; pp++)
  *pp += off;
  ap += off;
  np += off;
@@ -367,7 +380,7 @@ kvm_argv(kvm_t *kd, const struct kinfo_p
  }
  /* Make sure argv is terminated. */
  *argv = 0;
- return (kd->argv);
+ return (*pargv);
 }
 
 static void
@@ -412,7 +425,7 @@ proc_verify(kvm_t *kd, const struct kinf
 }
 
 static char **
-kvm_doargv(kvm_t *kd, const struct kinfo_proc *p, int nchr,
+kvm_doargv(kvm_t *kd, const struct kinfo_proc *p, int nchr, int isenv,
     void (*info)(struct ps_strings *, u_long *, int *))
 {
  static struct ps_strings *ps;
@@ -444,7 +457,7 @@ kvm_doargv(kvm_t *kd, const struct kinfo
  (*info)(&arginfo, &addr, &cnt);
  if (cnt == 0)
  return (0);
- ap = kvm_argv(kd, p, addr, cnt, nchr);
+ ap = kvm_argv(kd, p, addr, cnt, nchr, isenv);
  /*
  * For live kernels, make sure this process didn't go away.
  */
@@ -454,47 +467,53 @@ kvm_doargv(kvm_t *kd, const struct kinfo
 }
 
 static char **
-kvm_arg_sysctl(kvm_t *kd, pid_t pid, int nchr, int env)
+kvm_arg_sysctl(kvm_t *kd, pid_t pid, int nchr, int isenv)
 {
  size_t len, orglen;
  int mib[4], ret;
- char *buf;
+ char *buf, **pargbuf;
 
- orglen = env ? kd->nbpg : 8 * kd->nbpg; /* XXX - should be ARG_MAX */
- if (kd->argbuf == NULL &&
-    (kd->argbuf = _kvm_malloc(kd, orglen)) == NULL)
+ if (isenv) {
+ pargbuf = &kd->envbuf;
+ orglen = kd->nbpg;
+ } else {
+ pargbuf = &kd->argbuf;
+ orglen = 8 * kd->nbpg; /* XXX - should be ARG_MAX */
+ }
+ if (*pargbuf == NULL &&
+    (*pargbuf = _kvm_malloc(kd, orglen)) == NULL)
  return (NULL);
 
 again:
  mib[0] = CTL_KERN;
  mib[1] = KERN_PROC_ARGS;
  mib[2] = (int)pid;
- mib[3] = env ? KERN_PROC_ENV : KERN_PROC_ARGV;
+ mib[3] = isenv ? KERN_PROC_ENV : KERN_PROC_ARGV;
 
  len = orglen;
- ret = (sysctl(mib, 4, kd->argbuf, &len, NULL, 0) < 0);
+ ret = (sysctl(mib, 4, *pargbuf, &len, NULL, 0) < 0);
  if (ret && errno == ENOMEM) {
- buf = _kvm_reallocarray(kd, kd->argbuf, orglen, 2);
+ buf = _kvm_reallocarray(kd, *pargbuf, orglen, 2);
  if (buf == NULL)
  return (NULL);
  orglen *= 2;
- kd->argbuf = buf;
+ *pargbuf = buf;
  goto again;
  }
 
  if (ret) {
- free(kd->argbuf);
- kd->argbuf = NULL;
+ free(*pargbuf);
+ *pargbuf = NULL;
  _kvm_syserr(kd, kd->program, "kvm_arg_sysctl");
  return (NULL);
  }
 #if 0
- for (argv = (char **)kd->argbuf; *argv != NULL; argv++)
+ for (argv = (char **)*pargbuf; *argv != NULL; argv++)
  if (strlen(*argv) > nchr)
  *argv[nchr] = '\0';
 #endif
 
- return (char **)(kd->argbuf);
+ return (char **)(*pargbuf);
 }
 
 /*
@@ -505,7 +524,7 @@ kvm_getargv(kvm_t *kd, const struct kinf
 {
  if (ISALIVE(kd))
  return (kvm_arg_sysctl(kd, kp->p_pid, nchr, 0));
- return (kvm_doargv(kd, kp, nchr, ps_str_a));
+ return (kvm_doargv(kd, kp, nchr, 0, ps_str_a));
 }
 
 char **
@@ -513,7 +532,7 @@ kvm_getenvv(kvm_t *kd, const struct kinf
 {
  if (ISALIVE(kd))
  return (kvm_arg_sysctl(kd, kp->p_pid, nchr, 1));
- return (kvm_doargv(kd, kp, nchr, ps_str_e));
+ return (kvm_doargv(kd, kp, nchr, 1, ps_str_e));
 }
 
 /*

Reply | Threaded
Open this post in threaded view
|

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

Todd C. Miller-2
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 <stddef> is needed only
>      for the ptrdiff_t.

Looks good, OK millert@

 - todd

Reply | Threaded
Open this post in threaded view
|

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 <stddef> 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

Reply | Threaded
Open this post in threaded view
|

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

Todd C. Miller-2
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

Reply | Threaded
Open this post in threaded view
|

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

Vadim Zhukov
In reply to this post by Vadim Zhukov
2018-05-03 18:59 GMT+03:00 Otto Moerbeek <[hidden email]>:
> 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 -0000 1.64
+++ kvm.c 3 May 2018 16:08:37 -0000
@@ -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 -0000 1.15
+++ kvm_mips64.c 3 May 2018 16:08:37 -0000
@@ -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

Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>:
> > 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 -0000 1.64
> +++ kvm.c 3 May 2018 16:08:37 -0000
> @@ -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 -0000 1.15
> +++ kvm_mips64.c 3 May 2018 16:08:37 -0000
> @@ -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