getmntinfo(3) sanity

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

getmntinfo(3) sanity

Vladimir Kirillov-2
Hello, tech@!

The getmntinfo(3) page says that the mentioned function uses static
storage which cannot be freed, however this storage is being actually
malloced and can be freed without consequences.

I see no real need in doing such weird things (correct me if i'm
wrong) so here's a diff to actually do pass a singly-malloced buffer
to the caller who should free it manually.

I've also converted the essential users of that function:
bin/df sbin/mount sbin/umount sbin/mountd usr.bin/fstat.
If you find this approach good/useful I'll convert the rest (the
manpage will also need some love).

It is good to subsequently replace all static stuff in libc, right?

Index: getmntinfo.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/getmntinfo.c,v
retrieving revision 1.7
diff -u -p -r1.7 getmntinfo.c
--- getmntinfo.c 8 Aug 2005 08:05:34 -0000 1.7
+++ getmntinfo.c 14 Oct 2010 22:28:06 -0000
@@ -38,25 +38,22 @@
 int
 getmntinfo(struct statfs **mntbufp, int flags)
 {
- static struct statfs *mntbuf;
- static int mntsize;
- static size_t bufsize;
+ struct statfs *mntbuf;
+ int mntsize;
+ size_t bufsize = 0;
 
- if (mntsize <= 0 && (mntsize = getfsstat(0, 0, MNT_NOWAIT)) < 0)
+ if ((mntsize = getfsstat(NULL, 0, MNT_NOWAIT)) < 0)
  return (0);
- if (bufsize > 0 && (mntsize = getfsstat(mntbuf, bufsize, flags)) < 0)
+
+ bufsize = mntsize * sizeof(struct statfs);
+ if ((mntbuf = malloc(bufsize)) == NULL)
+ return (0);
+
+ if ((mntsize = getfsstat(mntbuf, bufsize, flags)) < 0) {
+ free(mntbuf);
  return (0);
- while (bufsize <= mntsize * sizeof(struct statfs)) {
- if (mntbuf)
- free(mntbuf);
- bufsize = (mntsize + 1) * sizeof(struct statfs);
- if ((mntbuf = (struct statfs *)malloc(bufsize)) == 0) {
- bufsize = 0;
- return (0);
- }
- if ((mntsize = getfsstat(mntbuf, bufsize, flags)) < 0)
- return (0);
  }
+
  *mntbufp = mntbuf;
  return (mntsize);
 }

Index: bin/df/df.c
===================================================================
RCS file: /cvs/src/bin/df/df.c,v
retrieving revision 1.50
diff -u -p -r1.50 df.c
--- bin/df/df.c 27 Oct 2009 23:59:21 -0000 1.50
+++ bin/df/df.c 14 Oct 2010 23:19:58 -0000
@@ -74,7 +74,7 @@ int
 main(int argc, char *argv[])
 {
  struct stat stbuf;
- struct statfs *mntbuf;
+ struct statfs *mntbuf = NULL;
  long mntsize;
  int ch, i;
  int width, maxwidth;
@@ -125,16 +125,19 @@ main(int argc, char *argv[])
  if (!*argv) {
  mntsize = regetmntinfo(&mntbuf, mntsize);
  } else {
+ free(mntbuf);
  mntbuf = calloc(argc, sizeof(struct statfs));
  if (mntbuf == NULL)
  err(1, NULL);
  mntsize = 0;
  for (; *argv; argv++) {
+ int freemntpt = 0;
  if (stat(*argv, &stbuf) < 0) {
- if ((mntpt = getmntpt(*argv)) == 0) {
+ if ((mntpt = getmntpt(*argv)) == NULL) {
  warn("%s", *argv);
  continue;
  }
+ freemntpt = 1;
  } else if (S_ISCHR(stbuf.st_mode) || S_ISBLK(stbuf.st_mode)) {
  if (!raw_df(*argv, &mntbuf[mntsize]))
  ++mntsize;
@@ -156,6 +159,9 @@ main(int argc, char *argv[])
  ++mntsize;
  else
  warn("%s", *argv);
+
+ if (freemntpt)
+ free(mntpt);
  }
  }
 
@@ -176,21 +182,26 @@ main(int argc, char *argv[])
  bsdprint(mntbuf, mntsize, maxwidth);
  }
 
- exit(mntsize ? 0 : 1);
+ free(mntbuf);
+ return (mntsize ? 0 : 1);
 }
 
 char *
 getmntpt(char *name)
 {
+ char *s = NULL;
  long mntsize, i;
- struct statfs *mntbuf;
+ struct statfs *mntbuf = NULL;
 
  mntsize = getmntinfo(&mntbuf, MNT_NOWAIT);
  for (i = 0; i < mntsize; i++) {
- if (!strcmp(mntbuf[i].f_mntfromname, name))
- return (mntbuf[i].f_mntonname);
+ if (!strcmp(mntbuf[i].f_mntfromname, name)) {
+ s = strdup(mntbuf[i].f_mntonname);
+ break;
+ }
  }
- return (0);
+ free(mntbuf);
+ return (s);
 }
 
 static enum { IN_LIST, NOT_IN_LIST } which;
@@ -257,7 +268,11 @@ regetmntinfo(struct statfs **mntbufp, lo
  struct statfs *mntbuf;
 
  if (!lflag && typelist == NULL)
- return (nflag ? mntsize : getmntinfo(mntbufp, MNT_WAIT));
+ if (!nflag) {
+ free(*mntbufp);
+ return (getmntinfo(mntbufp, MNT_WAIT));
+ } else
+ return (mntsize);
 
  mntbuf = *mntbufp;
  j = 0;
Index: sbin/mount/mount.c
===================================================================
RCS file: /cvs/src/sbin/mount/mount.c,v
retrieving revision 1.50
diff -u -p -r1.50 mount.c
--- sbin/mount/mount.c 27 Oct 2009 23:59:33 -0000 1.50
+++ sbin/mount/mount.c 14 Oct 2010 23:19:58 -0000
@@ -59,7 +59,7 @@ int selected(const char *);
 char   *catopt(char *, const char *);
 char   *flags2opts(u_int32_t);
 struct statfs
-       *getmntpt(const char *);
+       *getmntpt(struct statfs *, int, const char *);
 int hasopt(const char *, const char *);
 void maketypelist(char *);
 void mangle(char *, int *, const char **, int);
@@ -100,7 +100,7 @@ int
 main(int argc, char * const argv[])
 {
  const char *mntonname, *vfstype;
- struct fstab *fs;
+ struct fstab *fs, *fsa = NULL;
  struct statfs *mntbuf;
  FILE *mountdfp;
  pid_t pid;
@@ -194,6 +194,7 @@ main(int argc, char * const argv[])
  continue;
  prmount(&mntbuf[i]);
  }
+ free(mntbuf);
  exit(rval);
  }
  break;
@@ -204,32 +205,37 @@ main(int argc, char * const argv[])
  if (realpath(*argv, mntpath) == NULL)
  err(1, "realpath %s", *argv);
  if (hasopt(options, "update")) {
- if ((mntbuf = getmntpt(mntpath)) == NULL)
+ struct statfs *mntpt;
+
+ if ((mntsize = getmntinfo(&mntbuf, MNT_NOWAIT)) == 0)
+ err(1, "getmntinfo");
+
+ if ((mntpt = getmntpt(mntbuf, mntsize, mntpath)) == NULL)
  errx(1,
     "unknown special file or file system %s.",
     *argv);
- if ((mntbuf->f_flags & MNT_ROOTFS) &&
-    !strcmp(mntbuf->f_mntfromname, "root_device")) {
+ if ((mntpt->f_flags & MNT_ROOTFS) &&
+    !strcmp(mntpt->f_mntfromname, "root_device")) {
  /* Lookup fstab for name of root device. */
- fs = getfsfile(mntbuf->f_mntonname);
+ fs = getfsfile(mntpt->f_mntonname);
  if (fs == NULL)
  errx(1,
     "can't find fstab entry for %s.",
     *argv);
  } else {
- fs = malloc(sizeof(*fs));
+ fsa = fs = malloc(sizeof(*fs));
  if (fs == NULL)
  err(1, "malloc");
- fs->fs_vfstype = mntbuf->f_fstypename;
- fs->fs_spec = mntbuf->f_mntfromname;
+ fs->fs_vfstype = mntpt->f_fstypename;
+ fs->fs_spec = mntpt->f_mntfromname;
  }
  /*
  * It's an update, ignore the fstab file options.
  * Get the current options, so we can change only
  * the options which given via a command line.
  */
- fs->fs_mntops = flags2opts(mntbuf->f_flags);
- mntonname = mntbuf->f_mntonname;
+ fs->fs_mntops = flags2opts(mntpt->f_flags);
+ mntonname = mntpt->f_mntonname;
  } else {
  if ((fs = getfsfile(mntpath)) == NULL &&
     (fs = getfsspec(mntpath)) == NULL)
@@ -280,7 +286,9 @@ main(int argc, char * const argv[])
  (void)fclose(mountdfp);
  }
 
- exit(rval);
+ free(fsa);
+ free(mntbuf);
+ return (rval);
 }
 
 int
@@ -584,12 +592,10 @@ prmount(struct statfs *sf)
 }
 
 struct statfs *
-getmntpt(const char *name)
+getmntpt(struct statfs *mntbuf, int mntsize, const char *name)
 {
- struct statfs *mntbuf;
- int i, mntsize;
+ int i;
 
- mntsize = getmntinfo(&mntbuf, MNT_NOWAIT);
  for (i = 0; i < mntsize; i++)
  if (strcmp(mntbuf[i].f_mntfromname, name) == 0 ||
     strcmp(mntbuf[i].f_mntonname, name) == 0)
Index: sbin/umount/umount.c
===================================================================
RCS file: /cvs/src/sbin/umount/umount.c,v
retrieving revision 1.21
diff -u -p -r1.21 umount.c
--- sbin/umount/umount.c 27 Oct 2009 23:59:34 -0000 1.21
+++ sbin/umount/umount.c 14 Oct 2010 23:19:58 -0000
@@ -119,7 +119,7 @@ main(int argc, char *argv[])
 int
 umountall(void)
 {
- struct statfs *fs;
+ struct statfs *fs = NULL;
  int n;
  int rval;
 
@@ -137,6 +137,7 @@ umountall(void)
  if (umountfs(fs[n].f_mntonname))
  rval = 1;
  }
+ free(fs);
  return (rval);
 }
 
@@ -164,6 +165,7 @@ umountfs(char *oname)
     if (S_ISBLK(sb.st_mode)) {
  if ((mntpt = getmntname(name, MNTON, type)) == NULL) {
  warnx("%s: not currently mounted", name);
+ free(mntpt);
  return (1);
  }
     } else if (!S_ISDIR(sb.st_mode)) {
@@ -177,9 +179,9 @@ umountfs(char *oname)
  * 99.9% of the time the path in the kernel is the one
  * realpath() returns but check the original just in case...
  */
+ newname = mntpt = NULL;
  if (!(newname = getmntname(name, MNTFROM, type)) &&
     !(mntpt = getmntname(name, MNTON, type)) ) {
- mntpt = oname;
  if (!(newname = getmntname(oname, MNTFROM, type)) &&
     !(mntpt = getmntname(oname, MNTON, type))) {
  warnx("%s: not currently mounted", oname);
@@ -245,13 +247,16 @@ umountfs(char *oname)
  auth_destroy(clp->cl_auth);
  clnt_destroy(clp);
  }
+ free(mntpt);
+ free(newname);
  return (0);
 }
 
 char *
 getmntname(char *name, mntwhat what, char *type)
 {
- struct statfs *mntbuf;
+ char *mntname = NULL;
+ struct statfs *mntbuf = NULL;
  int i, mntsize;
 
  if ((mntsize = getmntinfo(&mntbuf, MNT_NOWAIT)) == 0) {
@@ -263,16 +268,19 @@ getmntname(char *name, mntwhat what, cha
  if (type)
  memcpy(type, mntbuf[i].f_fstypename,
     sizeof(mntbuf[i].f_fstypename));
- return (mntbuf[i].f_mntonname);
+ mntname = strdup(mntbuf[i].f_mntonname);
+ break;
  }
  if ((what == MNTFROM) && !strcmp(mntbuf[i].f_mntonname, name)) {
  if (type)
  memcpy(type, mntbuf[i].f_fstypename,
     sizeof(mntbuf[i].f_fstypename));
- return (mntbuf[i].f_mntfromname);
+ mntname = strdup(mntbuf[i].f_mntfromname);
+ break;
  }
  }
- return (NULL);
+ free(mntbuf);
+ return (mntname);
 }
 
 static enum { IN_LIST, NOT_IN_LIST } which;
Index: sbin/mountd/mountd.c
===================================================================
RCS file: /cvs/src/sbin/mountd/mountd.c,v
retrieving revision 1.71
diff -u -p -r1.71 mountd.c
--- sbin/mountd/mountd.c 22 Mar 2010 16:35:27 -0000 1.71
+++ sbin/mountd/mountd.c 14 Oct 2010 23:19:58 -0000
@@ -688,7 +688,7 @@ get_exportlist(void)
  struct grouplist *grp, *tgrp;
  struct exportlist **epp;
  struct dirlist *dirhead;
- struct statfs fsb, *ofsp, *fsp;
+ struct statfs fsb, *ofsp = NULL, *fsp;
  struct hostent *hpe;
  struct ucred anon;
  union {
@@ -738,7 +738,6 @@ get_exportlist(void)
  out_of_mem();
 
  for (i = 0; i < num; i++) {
-
  if (!strncmp(fsp->f_fstypename, MOUNT_MFS, MFSNAMELEN) ||
     !strncmp(fsp->f_fstypename, MOUNT_FFS, MFSNAMELEN) ||
     !strncmp(fsp->f_fstypename, MOUNT_EXT2FS, MFSNAMELEN) ||
@@ -1047,8 +1046,9 @@ nextline:
  syslog(LOG_ERR, "Can't delete exports for %s: %m",
     fsp->f_mntonname);
  }
- free(fstbl);
  fclose(exp_file);
+ free(fstbl);
+ free(ofsp);
 }
 
 /*
Index: usr.bin/fstat/fstat.c
===================================================================
RCS file: /cvs/src/usr.bin/fstat/fstat.c,v
retrieving revision 1.70
diff -u -p -r1.70 fstat.c
--- usr.bin/fstat/fstat.c 27 Oct 2009 23:59:38 -0000 1.70
+++ usr.bin/fstat/fstat.c 14 Oct 2010 23:19:58 -0000
@@ -100,6 +100,7 @@ int signo; /* signal to send (fuser onl
 
 kvm_t *kd;
 uid_t uid;
+static struct statfs *mntbuf;
 
 void fstat_dofile(struct kinfo_file2 *);
 void fstat_header(void);
@@ -266,7 +267,8 @@ main(int argc, char *argv[])
  if (fuser)
  fuser_run();
 
- exit(0);
+ free(mntbuf);
+ return (0);
 }
 
 void
@@ -737,7 +739,6 @@ getinetproto(number)
 int
 getfname(char *filename)
 {
- static struct statfs *mntbuf;
  static int nmounts;
  int i;
  struct stat sb;
@@ -757,7 +758,7 @@ getfname(char *filename)
  if (fuser && !fsflg && S_ISBLK(sb.st_mode)) {
  if (mntbuf == NULL) {
  nmounts = getmntinfo(&mntbuf, MNT_NOWAIT);
- if (nmounts == -1)
+ if (nmounts == 0)
  err(1, "getmntinfo");
  }
  for (i = 0; i < nmounts; i++) {

Reply | Threaded
Open this post in threaded view
|

Re: getmntinfo(3) sanity

Ingo Schwarze
Hi Vladimir,

Vladimir Kirillov wrote on Fri, Oct 15, 2010 at 02:31:21AM +0300:

> The getmntinfo(3) page says that the mentioned function uses static
> storage which cannot be freed, however this storage is being actually
> malloced and can be freed without consequences.

Err, no.
Reading the code below, in case you free the pointer returned
in *mntbufp, the address will still exist internally in the static
struct statfs *mntbuf, and the next time you call getmntinfo(),
the statement  if (mntbuf) free(mntbuf)  results in a double free.

> I see no real need in doing such weird things (correct me if i'm
> wrong)

As far as i understand, getmntinfo() cannot use a static buffer
because the size is not known beforehand.  Thus, it mallocs
its internal buffer and frees it during the next call.
Off the top of my head, i wouldn't know how to simplify that.

> so here's a diff to actually do pass a singly-malloced buffer
> to the caller who should free it manually.

No, wait, you can't change the calling convention of an existing
library function just like that.  That would turn all application
code which is now correct into a horrible bunch of memory leaks,
and all application code written for the new convention would
result in double frees when linked against old libraries.

> I've also converted the essential users of that function:
> bin/df sbin/mount sbin/umount sbin/mountd usr.bin/fstat.
> If you find this approach good/useful I'll convert the rest (the
> manpage will also need some love).

And what about those users of the function not in base?
This is a library function, anybody may be using it in private code.

> It is good to subsequently replace all static stuff in libc, right?

Not quite.
Instead, some people double the number of libc functions by
adding thread-safe versions, like in ctime() -> ctime() and ctime_r(),
and that is ugly enough.
I'm pretty sure you must not change how existing functions work.

Yours,
  Ingo


> Index: getmntinfo.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/getmntinfo.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 getmntinfo.c
> --- getmntinfo.c 8 Aug 2005 08:05:34 -0000 1.7
> +++ getmntinfo.c 14 Oct 2010 22:28:06 -0000
> @@ -38,25 +38,22 @@
>  int
>  getmntinfo(struct statfs **mntbufp, int flags)
>  {
> - static struct statfs *mntbuf;
> - static int mntsize;
> - static size_t bufsize;
> + struct statfs *mntbuf;
> + int mntsize;
> + size_t bufsize = 0;
>  
> - if (mntsize <= 0 && (mntsize = getfsstat(0, 0, MNT_NOWAIT)) < 0)
> + if ((mntsize = getfsstat(NULL, 0, MNT_NOWAIT)) < 0)
>   return (0);
> - if (bufsize > 0 && (mntsize = getfsstat(mntbuf, bufsize, flags)) < 0)
> +
> + bufsize = mntsize * sizeof(struct statfs);
> + if ((mntbuf = malloc(bufsize)) == NULL)
> + return (0);
> +
> + if ((mntsize = getfsstat(mntbuf, bufsize, flags)) < 0) {
> + free(mntbuf);
>   return (0);
> - while (bufsize <= mntsize * sizeof(struct statfs)) {
> - if (mntbuf)
> - free(mntbuf);
> - bufsize = (mntsize + 1) * sizeof(struct statfs);
> - if ((mntbuf = (struct statfs *)malloc(bufsize)) == 0) {
> - bufsize = 0;
> - return (0);
> - }
> - if ((mntsize = getfsstat(mntbuf, bufsize, flags)) < 0)
> - return (0);
>   }
> +
>   *mntbufp = mntbuf;
>   return (mntsize);
>  }

Reply | Threaded
Open this post in threaded view
|

Re: getmntinfo(3) sanity

Vladimir Kirillov-2
Hi, Ingo!

On 02:06 Fri 15 Oct, Ingo Schwarze wrote:
> > so here's a diff to actually do pass a singly-malloced buffer
> > to the caller who should free it manually.
>
> No, wait, you can't change the calling convention of an existing
> library function just like that.  That would turn all application
> code which is now correct into a horrible bunch of memory leaks,
> and all application code written for the new convention would
> result in double frees when linked against old libraries.

Right.

> > It is good to subsequently replace all static stuff in libc, right?
>
> Not quite.
> Instead, some people double the number of libc functions by
> adding thread-safe versions, like in ctime() -> ctime() and ctime_r(),
> and that is ugly enough.
> I'm pretty sure you must not change how existing functions work.
 
My primary goal of this is having a thread libc and a set of functions
in it to be safely used by daemons (getmntinfo(3) is used at least by
amd, mountd and snmpd) with controlled memory usage.

I've seen internal libc mutexes to protect usage of shared data in some
places, however they are likely to be stubs, right?

So, functions like getmntinfo() should be either internally protected or
provided a second "*_r" implementation.

Which method is the most preferred by the project?

Reply | Threaded
Open this post in threaded view
|

Re: getmntinfo(3) sanity

Otto Moerbeek
On Sat, Oct 16, 2010 at 02:41:33PM +0300, Vladimir Kirillov wrote:

> Hi, Ingo!
>
> On 02:06 Fri 15 Oct, Ingo Schwarze wrote:
> > > so here's a diff to actually do pass a singly-malloced buffer
> > > to the caller who should free it manually.
> >
> > No, wait, you can't change the calling convention of an existing
> > library function just like that.  That would turn all application
> > code which is now correct into a horrible bunch of memory leaks,
> > and all application code written for the new convention would
> > result in double frees when linked against old libraries.
>
> Right.
>
> > > It is good to subsequently replace all static stuff in libc, right?
> >
> > Not quite.
> > Instead, some people double the number of libc functions by
> > adding thread-safe versions, like in ctime() -> ctime() and ctime_r(),
> > and that is ugly enough.
> > I'm pretty sure you must not change how existing functions work.
>  
> My primary goal of this is having a thread libc and a set of functions
> in it to be safely used by daemons (getmntinfo(3) is used at least by
> amd, mountd and snmpd) with controlled memory usage.
>
> I've seen internal libc mutexes to protect usage of shared data in some
> places, however they are likely to be stubs, right?

No, in a threaded environent, the locking primitives used by e.g.
malloc(3) are real.

>
> So, functions like getmntinfo() should be either internally protected or
> provided a second "*_r" implementation.
>
> Which method is the most preferred by the project?

You forget a third way of dealing with this: document that the function
returns static data and let the user of the API deal with it.

        -Otto