Hi Jeff
On Tue, Jan 03, 2012 at 07:22:29AM -0500, Jeff Layton wrote:
> > +
> > + mtabfile = strdup(MOUNTED);
>
> This looks like a buffer overrun. Here, you're strduping MOUNTED...
>
> > + mtabdir = dirname(mtabfile);
>
> Then altering that string to get the dirname...
>
> > + if (!mtabdir) {
> > + fprintf(stderr, "del_mtab: cannot determine current mtab path");
> > + rc = EX_FILEIO;
> > + goto del_mtab_exit;
> > + }
> > +
> > + tmpfile = strcat(mtabdir, MNT_TMP_FILE);
>
> Then using strcat to append MNT_TMP_FILE to mtabdir. But, won't that be
> longer than the original string that you strdup'ed?
>
Right, makes sense. I'll be changing this to pre-allocate enough memory
to tmpfile and mtabdir before concatenate the strings.
> > + if (rc) {
> > + fprintf(stderr, "del_mtab: unable to add mount entry to mtab\n");
> > + ftruncate(fd, statbuf.st_size);
>
> ^^^^^^^^^^^^
> At this point, statbuf is full of leftover stack
> junk. You're going to truncate it to an unknown
> value here since you haven't fstat'ed it yet.
>
> In any case, there's no reason to truncate the
> original mnttab here. You aren't altering it.
> Won't that potentially fail since you opened it
> "r" anyway?
>
About the ftruncate, I'm removing it since it's not necessary. About the
open mode of mnttab, the rename() function doesn't need the file to be
opened, look that both files are closed before call rename().
> > + }
> > +
> > + tmprc = my_endmntent(mntmtab, statbuf.st_size);
> > + if (tmprc) {
> > + fprintf(stderr, "del_mtab: error %d detected on close of mtab\n", tmprc);
> > + rc = EX_FILEIO;
> > + }
>
> You probably don't need to use my_endmntent here since the original
> mtab isn't being altered, just replaced.
>
> > + endmntent(mnttmp);
>
> You should however fflush the mnttmp stream, and fsync that fd before
> you replace the mtab. You could use my_endmntent for that, but there's
> no need to truncate if that fails -- you'll just want to delete the
> tmpfile in that case and return an error (right?).
>
I'm changing the order here. I'll close mnttmp using my_endmntent() to
ensure it gets synched before it's closed and use endmntent() to close
the mntmtab since the last one does not need a sync.
I hope to have the patch today yet.
--
--Carlos
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
