This is the mail archive of the cygwin-patches mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add getmntent_r


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

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

Attachment: mntent-test.c
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]