On Thu, 29 Dec 2011 15:02:05 -0200
Carlos Maiolino <cmaiolino@xxxxxxxxxx> wrote:
> During a remount of a cifs filesystem, the mtab file is not properly updated, which
> leads to a doubled entry of the same filesystem in the /etc/mtab file. This patch
> adds a new function del_mtab() which is called before the add_mtab() in case the fs
> is being remounted.
> The del_mtab() function will delete from the mtab, the old entry from the filesystem which
> is being remounted, and then, calls add_mtab() to add an updated entry to the mtab file.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> ---
> mount.cifs.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 102 insertions(+), 0 deletions(-)
>
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 147f7fc..d9bf0b9 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -42,6 +42,7 @@
> #include <fcntl.h>
> #include <limits.h>
> #include <paths.h>
> +#include <libgen.h>
> #include <sys/mman.h>
> #include <sys/wait.h>
> #ifdef HAVE_LIBCAP_NG
> @@ -161,6 +162,7 @@
> #define OPT_BKUPUID 30
> #define OPT_BKUPGID 31
>
> +#define MNT_TMP_FILE "/.mtab.tmp.XXXXXX"
>
> /* struct for holding parsed mount info for use by privleged process */
> struct parsed_mount_info {
> @@ -1624,6 +1626,101 @@ add_mtab_exit:
> return rc;
> }
>
> +static int
> +del_mtab(char *mountpoint)
> +{
> + int fd, tmprc, rc = 0;
> + FILE *mnttmp, *mntmtab;
> + struct mntent *mountent;
> + struct stat statbuf;
> + char *mtabfile, *mtabdir, *tmpfile;
> +
> + 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?
> +
> + atexit(unlink(tmpfile));
> + tmpfile = mktemp(tmpfile);
> + if (!tmpfile) {
> + fprintf(stderr, "del_mtab: cannot setup tmp file destination");
> + rc = EX_FILEIO;
> + goto del_mtab_exit;
> + }
> +
> + atexit(unlock_mtab);
> + rc = lock_mtab();
> + if (rc) {
> + fprintf(stderr, "del_mtab: cannot lock mtab");
> + rc = EX_FILEIO;
> + goto del_mtab_exit;
> + }
> +
> + mntmtab = setmntent(MOUNTED, "r");
> + if (!mntmtab) {
> + fprintf(stderr, "del_mtab: could not update mount table\n");
> + rc = EX_FILEIO;
> + goto del_mtab_exit;
> + }
> +
> + mnttmp = setmntent(tmpfile, "w");
> + if (!mnttmp) {
> + fprintf(stderr, "del_mtab: could not update mount table\n");
> + endmntent(mntmtab);
> + rc = EX_FILEIO;
> + goto del_mtab_exit;
> + }
> +
> + fd = fileno(mntmtab);
> + if (fd < 0) {
> + fprintf(stderr, "del_mtab: mntent does not appear to be valid\n");
> + endmntent(mntmtab);
> + endmntent(mnttmp);
> + rc = EX_FILEIO;
> + goto del_mtab_exit;
> + }
> +
> + while ((mountent = getmntent(mntmtab)) != NULL) {
> + if (!strcmp(mountent->mnt_dir,mountpoint))
> + continue;
> + rc = addmntent(mnttmp, mountent);
> + 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?
> + rc = EX_FILEIO;
> + break;
> + }
> + }
> +
> + rc = fstat(fd, &statbuf);
> + if (rc != 0) {
> + fprintf(stderr, "del_mtab: unable to fstat open mtab\n");
> + endmntent(mntmtab);
> + endmntent(mnttmp);
> + rc = EX_FILEIO;
> + goto del_mtab_exit;
> + }
> +
> + 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?).
> +
> + rename(tmpfile, MOUNTED);
> + unlink(tmpfile);
> +
> +del_mtab_exit:
> + unlock_mtab();
> + return rc;
> +}
> +
> /* have the child drop root privileges */
> static int
> drop_child_privs(void)
> @@ -2021,6 +2118,11 @@ mount_retry:
> }
>
> do_mtab:
> + if (parsed_info->flags & MS_REMOUNT){
> + rc = del_mtab(mountpoint);
> + if (rc)
> + goto mount_exit;
> + }
> if (!parsed_info->nomtab && !mtab_unusable())
> rc = add_mtab(orig_dev, mountpoint, parsed_info->flags, fstype);
>
--
Jeff Layton <jlayton@xxxxxxxxx>
--
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
