gdbusauthmechanismsha1: Fix race in keyring_acquire_lock()

There was a time-of-check-to-time-of-use (TOCTTOU) race in the keyring
lock code, where it would check the existence of the lock file using
`access()`, then proceed to call `open(O_CREAT | O_EXCL)` to try and
create the lock file once `access()` showed that it didn’t exist.

The problem is that, because this is happening in a shared directory
(`~/.dbus-keyrings`), another process could quite legitimately create
the lock file in the meantime.

Instead, unconditionally call `open()` and ignore errors from it (which
will be returned if the lock file already exists) until it succeeds (or
the code times out).

This eliminates the TOCTTOU race, and simplifies the timeout behaviour
so there aren’t two loops (check for existence, try to create)
happening. It brings this code in line with what dbus.git does (see
`_dbus_keyring_lock()`).

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Fixes: #1954

gio/gdbusauthmechanismsha1.c

@@ -450,25 +450,52 @@ _log (const gchar *message,
   g_free (s);
 }
 
+/* Returns FD for lock file, if it was created exclusively (didn't exist already,
+ * and was created successfully) */
+static gint
+create_lock_exclusive (const gchar  *lock_path,
+                       GError      **error)
+{
+  int errsv;
+  gint ret;
+
+  ret = g_open (lock_path, O_CREAT | O_EXCL, 0600);
+  errsv = errno;
+  if (ret < 0)
+    {
+      g_set_error (error,
+                   G_IO_ERROR,
+                   g_io_error_from_errno (errsv),
+                   _("Error creating lock file “%s”: %s"),
+                   lock_path,
+                   g_strerror (errsv));
+      return -1;
+    }
+
+  return ret;
+}
+
 static gint
 keyring_acquire_lock (const gchar  *path,
                       GError      **error)
 {
-  gchar *lock;
+  gchar *lock = NULL;
   gint ret;
   guint num_tries;
-#ifdef EEXISTS
-  guint num_create_tries;
-#endif
   int errsv;
 
+  /* Total possible sleep period = max_tries * timeout_usec = 0.5s */
+  const guint max_tries = 50;
+  const guint timeout_usec = 1000 * 10;
+
   g_return_val_if_fail (path != NULL, -1);
   g_return_val_if_fail (error == NULL || *error == NULL, -1);
 
   ret = -1;
-  lock = g_strdup_printf ("%s.lock", path);
+  lock = g_strconcat (path, ".lock", NULL);
 
   /* This is what the D-Bus spec says
+   * (https://dbus.freedesktop.org/doc/dbus-specification.html#auth-mechanisms-sha)
    *
    *  Create a lockfile name by appending ".lock" to the name of the
    *  cookie file. The server should attempt to create this file using
@@ -481,20 +508,21 @@ keyring_acquire_lock (const gchar  *path,
    *         real locking implementations are still flaky on network filesystems
    */
 
-#ifdef EEXISTS
+  for (num_tries = 0; num_tries < max_tries; num_tries++)
-  num_create_tries = 0;
- again:
-#endif
-  num_tries = 0;
-  while (g_file_test (lock, G_FILE_TEST_EXISTS))
     {
+      /* Ignore the error until the final call. */
+      ret = create_lock_exclusive (lock, NULL);
+      if (ret >= 0)
+        break;
+
       /* sleep 10ms, then try again */
-      g_usleep (1000*10);
+      g_usleep (timeout_usec);
-      num_tries++;
+    }
-      if (num_tries == 50)
+
+  if (num_tries == max_tries)
     {
       /* ok, we slept 50*10ms = 0.5 seconds. Conclude that the lock file must be
-           * stale (nuke the it from orbit)
+       * stale (nuke it from orbit)
        */
       if (g_unlink (lock) != 0)
         {
@@ -507,36 +535,12 @@ keyring_acquire_lock (const gchar  *path,
                        g_strerror (errsv));
           goto out;
         }
-          _log ("Deleted stale lock file '%s'", lock);
-          break;
-        }
-    }
 
-  ret = g_open (lock, O_CREAT |
+      _log ("Deleted stale lock file '%s'", lock);
-#ifdef O_EXCL
+
-                O_EXCL,
+      /* Try one last time to create it, now that we've deleted the stale one */
-#else
+      ret = create_lock_exclusive (lock, error);
-                0,
+      if (ret < 0)
-#endif
-                0600);
-  errsv = errno;
-  if (ret == -1)
-    {
-#ifdef EEXISTS
-      /* EEXIST: pathname already exists and O_CREAT and O_EXCL were used. */
-      if (errsv == EEXISTS)
-        {
-          num_create_tries++;
-          if (num_create_tries < 5)
-            goto again;
-        }
-#endif
-      g_set_error (error,
-                   G_IO_ERROR,
-                   g_io_error_from_errno (errsv),
-                   _("Error creating lock file “%s”: %s"),
-                   lock,
-                   g_strerror (errsv));
         goto out;
     }