[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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cygwin-getmntent_r.patch
Type: application/x-itunes-itlp
Size: 4364 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20120606/a1e87528/attachment.bin>
-------------- next part --------------
#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;
}