Make dbus-uuidgen atomic

A Red Hat QA engineer hit in practice a race condition in dbus-uuidgen
where it could leave an empty file.

dbus-uuidgen (_dbus_create_uuid_file_exclusively) formerly created an
empty file in the path to the uuid, then filled it in.  At some point,
the internal libdbus _dbus_string_save_to_file became atomic on Unix
at least (doing the save to temp file, fsync(), rename() dance).

So _dbus_create_uuid_file_exclusively doesn't need to create the file
beforehand anymore.  However, it *does* need the file to be
world-readable, unlike all other consumers of
_dbus_string_save_to_file.  So add a "world_readable" argument.

7 files changed, 27 insertions, 19 deletions

diff --git a/dbus/dbus-file-unix.c b/dbus/dbus-file-unix.c
index 363a278a..19759336 100644
--- a/dbus/dbus-file-unix.c
+++ b/dbus/dbus-file-unix.c
@@ -156,12 +156,14 @@ _dbus_file_get_contents (DBusString       *str,
  *
  * @param str the string to write out
  * @param filename the file to save string to
+ * @param world_readable If set, ensure the file is world readable
  * @param error error to be filled in on failure
  * @returns #FALSE on failure
  */
 dbus_bool_t
 _dbus_string_save_to_file (const DBusString *str,
                            const DBusString *filename,
+                           dbus_bool_t      world_readable,
                            DBusError        *error)
 {
   int fd;
@@ -211,7 +213,7 @@ _dbus_string_save_to_file (const DBusString *str,
   tmp_filename_c = _dbus_string_get_const_data (&tmp_filename);
 
   fd = open (tmp_filename_c, O_WRONLY | O_BINARY | O_EXCL | O_CREAT,
-             0600);
+             world_readable ? 0644 : 0600);
   if (fd < 0)
     {
       dbus_set_error (error, _dbus_error_from_errno (errno),
@@ -219,6 +221,20 @@ _dbus_string_save_to_file (const DBusString *str,
                       _dbus_strerror (errno));
       goto out;
     }
+  if (world_readable)
+    {
+      /* Ensure the file is world readable even in the presence of
+       * possibly restrictive umasks;
+       * see http://lists.freedesktop.org/archives/dbus/2010-September/013367.html
+       */
+      if (fchmod (fd, 0644) < 0)
+        {
+          dbus_set_error (error, _dbus_error_from_errno (errno),
+                          "Could not chmod %s: %s", tmp_filename_c,
+                          _dbus_strerror (errno));
+          goto out;
+        }
+    }
 
   _dbus_verbose ("tmp file fd %d opened\n", fd);
   
diff --git a/dbus/dbus-file-win.c b/dbus/dbus-file-win.c
index 4f0b0759..53a3fc5b 100644
--- a/dbus/dbus-file-win.c
+++ b/dbus/dbus-file-win.c
@@ -204,12 +204,14 @@ _dbus_file_get_contents (DBusString       *str,
  *
  * @param str the string to write out
  * @param filename the file to save string to
+ * @param world_readable if true, ensure file is world readable
  * @param error error to be filled in on failure
  * @returns #FALSE on failure
  */
 dbus_bool_t
 _dbus_string_save_to_file (const DBusString *str,
                            const DBusString *filename,
+                           dbus_bool_t       world_readable,
                            DBusError        *error)
 {
   HANDLE hnd;
@@ -259,6 +261,7 @@ _dbus_string_save_to_file (const DBusString *str,
   filename_c = _dbus_string_get_const_data (filename);
   tmp_filename_c = _dbus_string_get_const_data (&tmp_filename);
 
+  /* TODO - support world-readable in an atomic fashion */
   hnd = CreateFileA (tmp_filename_c, GENERIC_WRITE,
                      FILE_SHARE_READ | FILE_SHARE_WRITE,
                      NULL, CREATE_NEW, FILE_ATTRIBUTE_NORMAL,
@@ -271,6 +274,8 @@ _dbus_string_save_to_file (const DBusString *str,
       _dbus_win_free_error_string (emsg);
       goto out;
     }
+  if (world_readable)
+    _dbus_make_file_world_readable (tmp_filename_c);
 
   _dbus_verbose ("tmp file %s hnd %p opened\n", tmp_filename_c, hnd);
 
diff --git a/dbus/dbus-file.h b/dbus/dbus-file.h
index 7943198b..24837f47 100644
--- a/dbus/dbus-file.h
+++ b/dbus/dbus-file.h
@@ -45,6 +45,7 @@ dbus_bool_t _dbus_file_get_contents   (DBusString       *str,
                                        DBusError        *error);
 dbus_bool_t _dbus_string_save_to_file (const DBusString *str,
                                        const DBusString *filename,
+                                       dbus_bool_t       world_readable,
                                        DBusError        *error);
 
 dbus_bool_t _dbus_make_file_world_readable   (const DBusString *filename,
diff --git a/dbus/dbus-internals.c b/dbus/dbus-internals.c
index 2ed56b35..5e864ce3 100644
--- a/dbus/dbus-internals.c
+++ b/dbus/dbus-internals.c
@@ -719,27 +719,13 @@ _dbus_create_uuid_file_exclusively (const DBusString *filename,
       goto error;
     }
   
-  /* FIXME this is racy; we need a save_file_exclusively
-   * function. But in practice this should be fine for now.
-   *
-   * - first be sure we can create the file and it
-   *   doesn't exist by creating it empty with O_EXCL
-   * - then create it by creating a temporary file and
-   *   overwriting atomically with rename()
-   */
-  if (!_dbus_create_file_exclusively (filename, error))
-    goto error;
-
   if (!_dbus_string_append_byte (&encoded, '\n'))
     {
       _DBUS_SET_OOM (error);
       goto error;
     }
   
-  if (!_dbus_string_save_to_file (&encoded, filename, error))
-    goto error;
-
-  if (!_dbus_make_file_world_readable (filename, error))
+  if (!_dbus_string_save_to_file (&encoded, filename, TRUE, error))
     goto error;
 
   _dbus_string_free (&encoded);
diff --git a/dbus/dbus-keyring.c b/dbus/dbus-keyring.c
index 031521c2..bef64523 100644
--- a/dbus/dbus-keyring.c
+++ b/dbus/dbus-keyring.c
@@ -605,7 +605,7 @@ _dbus_keyring_reload (DBusKeyring *keyring,
         }
       
       if (!_dbus_string_save_to_file (&contents, &keyring->filename,
-                                      error))
+                                      FALSE, error))
         goto out;
     }
 
diff --git a/dbus/dbus-nonce.c b/dbus/dbus-nonce.c
index 5143939a..a7a82f12 100644
--- a/dbus/dbus-nonce.c
+++ b/dbus/dbus-nonce.c
@@ -170,7 +170,7 @@ generate_and_write_nonce (const DBusString *filename, DBusError *error)
       return FALSE;
     }
 
-  ret = _dbus_string_save_to_file (&nonce, filename, error);
+  ret = _dbus_string_save_to_file (&nonce, filename, FALSE, error);
 
   _dbus_string_free (&nonce);
 
diff --git a/test/break-loader.c b/test/break-loader.c
index f85bd207..7bfa7227 100644
--- a/test/break-loader.c
+++ b/test/break-loader.c
@@ -161,7 +161,7 @@ try_mutated_data (const DBusString *data)
           printf ("Child failed, writing %s\n", _dbus_string_get_const_data (&filename));
 
           dbus_error_init (&error);
-          if (!_dbus_string_save_to_file (data, &filename, &error))
+          if (!_dbus_string_save_to_file (data, &filename, FALSE, &error))
             {
               fprintf (stderr, "Failed to save failed message data: %s\n",
                        error.message);