[evolution-data-server] Bug 730438 - Remove Coverity scan TOCTTOU races from file handling code

[Milan Crha <mcrha src gnome org>]

commit 0737b9a3e7d65e4746bb309f7732f3213337e23e
Author: Philip Withnall <philip withnall collabora co uk>
Date:   Mon Sep 8 15:31:14 2014 +0200

    Bug 730438 - Remove Coverity scan TOCTTOU races from file handling code
    
    There are various places in EDS where Coverity has detected TOCTTOU (Time
    Of Check To Time Of Use) races in the file handling code, typically where
    we do a stat() and then some other operation (e.g. open()) on the same file.
    Apart from using extra syscalls unnecessarily, this approach is prone to
    (quite unlikely) races where the file is created/destroyed between the stat()
    and the following call. Instead, we should just examine the return value
    from the following call more closely, and delete the stat() call.

 addressbook/libedata-book/e-book-backend-summary.c |   32 +++++++++------
 camel/camel-movemail.c                             |   43 +++++++++++---------
 camel/providers/local/camel-local-folder.c         |    6 +--
 camel/providers/local/camel-local-store.c          |   16 ++-----
 camel/providers/local/camel-mbox-summary.c         |    8 ++--
 5 files changed, 53 insertions(+), 52 deletions(-)
---
diff --git a/addressbook/libedata-book/e-book-backend-summary.c 
b/addressbook/libedata-book/e-book-backend-summary.c
index dcb041a..33fc73c 100644
--- a/addressbook/libedata-book/e-book-backend-summary.c
+++ b/addressbook/libedata-book/e-book-backend-summary.c
@@ -439,31 +439,37 @@ e_book_backend_summary_open (EBookBackendSummary *summary)
        if (summary->priv->fp)
                return TRUE;
 
-       if (g_stat (summary->priv->summary_path, &sb) == -1) {
+       /* Try opening the summary file. */
+       fp = g_fopen (summary->priv->summary_path, "rb");
+       if (!fp) {
                /* if there's no summary present, look for the .new
                 * file and rename it if it's there, and attempt to
                 * load that */
                gchar *new_filename = g_strconcat (summary->priv->summary_path, ".new", NULL);
-               if (g_stat (new_filename, &sb) == -1) {
-                       g_free (new_filename);
-                       return FALSE;
-               }
-               else {
-                       if (g_rename (new_filename, summary->priv->summary_path) == -1) {
-                               g_warning (
-                                       "%s: Failed to rename '%s' to '%s': %s", G_STRFUNC,
-                                       new_filename, summary->priv->summary_path, g_strerror (errno));
-                       }
-                       g_free (new_filename);
+
+               if (g_rename (new_filename, summary->priv->summary_path) == -1 &&
+                   errno != ENOENT) {
+                       g_warning (
+                               "%s: Failed to rename '%s' to '%s': %s", G_STRFUNC,
+                               new_filename, summary->priv->summary_path, g_strerror (errno));
+               } else {
+                       fp = g_fopen (summary->priv->summary_path, "rb");
                }
+
+               g_free (new_filename);
        }
 
-       fp = g_fopen (summary->priv->summary_path, "rb");
        if (!fp) {
                g_warning ("failed to open summary file");
                return FALSE;
        }
 
+       if (fstat (fileno (fp), &sb) == -1) {
+               g_warning ("failed to get summary file size");
+               fclose (fp);
+               return FALSE;
+       }
+
        if (!e_book_backend_summary_check_magic (summary, fp)) {
                g_warning ("file is not a valid summary file");
                fclose (fp);
diff --git a/camel/camel-movemail.c b/camel/camel-movemail.c
index bfc5d5e..7a643fa 100644
--- a/camel/camel-movemail.c
+++ b/camel/camel-movemail.c
@@ -80,7 +80,7 @@ static gint camel_movemail_copy (gint fromfd, gint tofd, goffset start, gsize by
  * directory. Dot locking is used on the source file (but not the
  * destination).
  *
- * Return Value: Returns -1 on error.
+ * Return Value: Returns -1 on error or 0 on success.
  **/
 gint
 camel_movemail (const gchar *source,
@@ -92,37 +92,42 @@ camel_movemail (const gchar *source,
        gint sfd, dfd;
        struct stat st;
 
-       /* Stat and then open the spool file. If it doesn't exist or
+       /* open files */
+       sfd = open (source, O_RDWR);
+       if (sfd == -1 && errno != ENOENT) {
+               g_set_error (
+                       error, G_IO_ERROR,
+                       g_io_error_from_errno (errno),
+                       _("Could not open mail file %s: %s"),
+                       source, g_strerror (errno));
+               return -1;
+       } else if (sfd == -1) {
+               /* No mail. */
+               return -1;
+       }
+
+       /* Stat the spool file. If it doesn't exist or
         * is empty, the user has no mail. (There's technically a race
         * condition here in that an MDA might have just now locked it
         * to deliver a message, but we don't care. In that case,
         * assuming it's unlocked is equivalent to pretending we were
         * called a fraction earlier.)
         */
-       if (g_stat (source, &st) == -1) {
-               if (errno != ENOENT)
-                       g_set_error (
-                               error, G_IO_ERROR,
-                               g_io_error_from_errno (errno),
-                               _("Could not check mail file %s: %s"),
-                               source, g_strerror (errno));
-               return -1;
-       }
-
-       if (st.st_size == 0)
-               return 0;
-
-       /* open files */
-       sfd = open (source, O_RDWR);
-       if (sfd == -1) {
+       if (fstat (sfd, &st) == -1) {
+               close (sfd);
                g_set_error (
                        error, G_IO_ERROR,
                        g_io_error_from_errno (errno),
-                       _("Could not open mail file %s: %s"),
+                       _("Could not check mail file %s: %s"),
                        source, g_strerror (errno));
                return -1;
        }
 
+       if (st.st_size == 0) {
+               close (sfd);
+               return 0;
+       }
+
        dfd = open (dest, O_WRONLY | O_CREAT | O_APPEND, S_IRUSR | S_IWUSR);
        if (dfd == -1) {
                g_set_error (
diff --git a/camel/providers/local/camel-local-folder.c b/camel/providers/local/camel-local-folder.c
index ee3fb47..c002bcf 100644
--- a/camel/providers/local/camel-local-folder.c
+++ b/camel/providers/local/camel-local-folder.c
@@ -538,7 +538,6 @@ camel_local_folder_construct (CamelLocalFolder *lf,
 #else
        gchar folder_path[PATH_MAX];
 #endif
-       struct stat st;
 #endif
        gint forceindex;
        CamelLocalStore *ls;
@@ -584,11 +583,10 @@ camel_local_folder_construct (CamelLocalFolder *lf,
         */
 #ifndef G_OS_WIN32
        /* follow any symlinks to the mailbox */
-       if (g_lstat (lf->folder_path, &st) != -1 && S_ISLNK (st.st_mode) &&
 #ifdef __GLIBC__
-           (folder_path = realpath (lf->folder_path, NULL)) != NULL) {
+       if ((folder_path = realpath (lf->folder_path, NULL)) != NULL) {
 #else
-           realpath (lf->folder_path, folder_path) != NULL) {
+       if (realpath (lf->folder_path, folder_path) != NULL) {
 #endif
                g_free (lf->folder_path);
                lf->folder_path = g_strdup (folder_path);
diff --git a/camel/providers/local/camel-local-store.c b/camel/providers/local/camel-local-store.c
index 9cec262..2b9e3d2 100644
--- a/camel/providers/local/camel-local-store.c
+++ b/camel/providers/local/camel-local-store.c
@@ -58,7 +58,6 @@ xrename (const gchar *oldp,
          gint missingok,
          GError **error)
 {
-       struct stat st;
        gchar *old, *new;
        gchar *basename;
        gint ret = -1;
@@ -74,19 +73,12 @@ xrename (const gchar *oldp,
 
        d (printf ("renaming %s%s to %s%s\n", oldp, suffix, newp, suffix));
 
-       if (g_stat (old, &st) == -1) {
-               if (missingok && errno == ENOENT) {
-                       ret = 0;
-               } else {
-                       err = errno;
-                       ret = -1;
-               }
-       } else if ((!g_file_test (new, G_FILE_TEST_EXISTS) || g_remove (new) == 0) &&
-                  g_rename (old, new) == 0) {
-               ret = 0;
-       } else {
+       if (g_rename (old, new) == -1 &&
+           !(errno == ENOENT && missingok)) {
                err = errno;
                ret = -1;
+       } else {
+               ret = 0;
        }
 
        if (ret == -1) {
diff --git a/camel/providers/local/camel-mbox-summary.c b/camel/providers/local/camel-mbox-summary.c
index 9d390e0..50335ed 100644
--- a/camel/providers/local/camel-mbox-summary.c
+++ b/camel/providers/local/camel-mbox-summary.c
@@ -687,10 +687,6 @@ mbox_summary_sync_full (CamelMboxSummary *mbs,
        camel_operation_push_message (cancellable, _("Storing folder"));
        camel_folder_summary_lock (s);
 
-       /* preserve attributes as set on the file previously */
-       if (g_stat (cls->folder_path, &st) == 0)
-               filemode = 07777 & st.st_mode;
-
        fd = g_open (cls->folder_path, O_LARGEFILE | O_RDONLY | O_BINARY, 0);
        if (fd == -1) {
                camel_folder_summary_unlock (s);
@@ -703,6 +699,10 @@ mbox_summary_sync_full (CamelMboxSummary *mbs,
                return -1;
        }
 
+       /* preserve attributes as set on the file previously */
+       if (fstat (fd, &st) == 0)
+               filemode = 07777 & st.st_mode;
+
        tmpname_len = strlen (cls->folder_path) + 5;
        tmpname = g_alloca (tmpname_len);
        g_snprintf (tmpname, tmpname_len, "%s.tmp", cls->folder_path);