Re: [PATCH] Add getmntent_r

[Yaakov (Cygwin/X)]

On 2012-06-05 07:42, Corinna Vinschen wrote:

+extern "C" struct mntent *
+getmntent_r (FILE *, struct mntent *mntbuf, char *buf, int buflen)
+{
+  struct mntent *mnt = mount_table->getmntent (_my_tls.locals.iteration++);
+  char *tmpbuf;
+  int len = 0, maxlen;
+
+  if (!mnt)
+    {
+      mntbuf = NULL;

This doesn't make sense since mntbuf is a local varibale.  Changing
its value won't be propagated by the calling function anyway.

Further testing of glibc shows that buf and mntbuf are indeed left untouched when returning NULL.

+  maxlen = strlen (mnt->mnt_fsname) + strlen (mnt->mnt_dir)
+           + strlen (mnt->mnt_type) + strlen (mnt->mnt_opts) + 30;
+  tmpbuf = (char *) alloca (maxlen);
+  memset (tmpbuf, '\0', maxlen);
+
+  len += __small_sprintf (tmpbuf, "%s", mnt->mnt_fsname) + 1;
+  len += __small_sprintf (tmpbuf + len, "%s", mnt->mnt_dir) + 1;
+  len += __small_sprintf (tmpbuf + len, "%s", mnt->mnt_type) + 1;
+  len += __small_sprintf (tmpbuf + len, "%s", mnt->mnt_opts) + 1;

This you can have simpler.

+  len += __small_sprintf (tmpbuf + len, "%d %d", mnt->mnt_freq, 
mnt->mnt_passno);

and this I don't grok at all.  Why don't you just copy over the
numbers from mnt to mntbuf?

The string returned into buf is in the following format:

mnt_fsname\0mnt_dir\0mnt_type\0mnt_opts\0mnt_freq" "mnt_passno\0

In the test program, only 5 iterations of the for loop are necessary to display the entire string; the sixth only checks that no garbage is found beyond that (hence the memset call).

+  memcpy (buf, tmpbuf, buflen);

So the function returns success even if the incoming buffer space
is insufficient?

glibc makes no attempt to verify buf or mntbuf; if either of them are not initialized or too small, you're in "undefined behaviour" territory (aka SEGV :-).

This doesn't do what you want.  mntbuf is just a copy of mnt, so the
mntbuf members won't point to buf, but they will point to the internal
storage.  While you have made a copy of the internal strings, they won't
be used.  Also, assuming you first store the strings in tmpbuf and then
memcpy them over to buf,

+  return mnt;

And this returns the internal mntent entry anyway, so even mntbuf is
kind of moot.

Further testing of glibc shows that the returned pointer is indeed &mntent.

Revised patch and testcase attached.


Yaakov

cygwin-getmntent_r.patch
Description: application/itunes-itlp

#ifdef CCOD
#pragma CCOD:script no
#endif

#include <mntent.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

#ifdef __CYGWIN__
#include <dlfcn.h>
#include <cygwin/version.h>
#endif

int
main(void)
{
#if defined(__CYGWIN__) && CYGWIN_VERSION_API_MINOR < 262
  void *libc = dlopen ("cygwin1.dll", 0);
  struct mntent *(*getmntent_r) (FILE *, struct mntent *, char *, int)
    = dlsym (libc, "getmntent_r");
#endif

  FILE *mtab = setmntent (_PATH_MOUNTED, "r");
  int buflen = 256;
  char *buf = (char *) malloc (buflen);
  struct mntent mntent, *mntret;
  int i, len;

  while (((mntret = getmntent_r (mtab, &mntent, buf, buflen)) != NULL))
    {
      len = 0;
      for (i = 0; i < 6; i++)
        len += printf ("%s ", buf + len);
      printf ("\n");
      /* check that these are identical with the above */
      printf ("%s %s %s %s %d %d\n", mntent.mnt_fsname, mntent.mnt_dir,
                                     mntent.mnt_type, mntent.mnt_opts,
                                     mntent.mnt_freq, mntent.mnt_passno);
      /* mntret == &mntent */
      printf ("%p %p\n", &mntent, mntret);
    }
  mntret = getmntent_r (mtab, &mntent, buf, buflen);
  /* mntent and buf are untouched when returning NULL */
  printf ("%s %s %p\n", buf, mntent.mnt_fsname, mntret);

  return 0;
}