[glib: 1/5] gdbusauthmechanismsha1: Don’t delete a stale lock file if it’s changed

[Simon McVittie <smcv src gnome org>]

commit dc0eb5e49a38da4186f37f89be21983704114567
Author: Philip Withnall <pwithnall endlessos org>
Date:   Fri Feb 18 13:17:19 2022 +0000

    gdbusauthmechanismsha1: Don’t delete a stale lock file if it’s changed
    
    The retry loop for acquiring the lock for the authentication cookie file
    currently tries to acquire the lock for 0.5s, then gives up, assumes the
    lock file is stale, and deletes it.
    
    That’s great if the lock file *is* stale because it’s been left there by
    a crashed process.
    
    It’s not so great if the lock file just happens to have been there every
    time this process checked, because the cookie file is highly contested
    while (for example) running lots of parallel unit tests.
    
    Check for that situation by comparing the mtime of the lock file and
    continuing to retry if it’s changed.
    
    Signed-off-by: Philip Withnall <pwithnall endlessos org>
    
    Fixes: #1929

 gio/gdbusauthmechanismsha1.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
---
diff --git a/gio/gdbusauthmechanismsha1.c b/gio/gdbusauthmechanismsha1.c
index 8137b6352..ed5aa3f96 100644
--- a/gio/gdbusauthmechanismsha1.c
+++ b/gio/gdbusauthmechanismsha1.c
@@ -38,6 +38,7 @@
 #include "gdbusauthmechanismsha1.h"
 #include "gcredentials.h"
 #include "gdbuserror.h"
+#include "glocalfileinfo.h"
 #include "gioenumtypes.h"
 #include "gioerror.h"
 #include "gdbusprivate.h"
@@ -509,6 +510,7 @@ _log (const gchar *message,
  * and was created successfully) */
 static gint
 create_lock_exclusive (const gchar  *lock_path,
+                       gint64       *mtime_nsec,
                        GError      **error)
 {
   int errsv;
@@ -518,6 +520,16 @@ create_lock_exclusive (const gchar  *lock_path,
   errsv = errno;
   if (ret < 0)
     {
+      GLocalFileStat stat_buf;
+
+      /* Get the modification time to distinguish between the lock being stale
+       * or highly contested. */
+      if (mtime_nsec != NULL &&
+          g_local_file_stat (lock_path, G_LOCAL_FILE_STAT_FIELD_MTIME, G_LOCAL_FILE_STAT_FIELD_ALL, 
&stat_buf) == 0)
+        *mtime_nsec = _g_stat_mtime (&stat_buf) * G_USEC_PER_SEC * 1000 + _g_stat_mtim_nsec (&stat_buf);
+      else if (mtime_nsec != NULL)
+        *mtime_nsec = 0;
+
       g_set_error (error,
                    G_IO_ERROR,
                    g_io_error_from_errno (errsv),
@@ -538,6 +550,7 @@ keyring_acquire_lock (const gchar  *path,
   gint ret;
   guint num_tries;
   int errsv;
+  gint64 lock_mtime_nsec = 0, lock_mtime_nsec_prev = 0;
 
   /* Total possible sleep period = max_tries * timeout_usec = 0.5s */
   const guint max_tries = 50;
@@ -565,13 +578,21 @@ keyring_acquire_lock (const gchar  *path,
 
   for (num_tries = 0; num_tries < max_tries; num_tries++)
     {
+      lock_mtime_nsec_prev = lock_mtime_nsec;
+
       /* Ignore the error until the final call. */
-      ret = create_lock_exclusive (lock, NULL);
+      ret = create_lock_exclusive (lock, &lock_mtime_nsec, NULL);
       if (ret >= 0)
         break;
 
       /* sleep 10ms, then try again */
       g_usleep (timeout_usec);
+
+      /* If the mtime of the lock file changed, don’t count the retry, as it
+       * seems like there’s contention between processes for the lock file,
+       * rather than a stale lock file from a crashed process. */
+      if (num_tries > 0 && lock_mtime_nsec != lock_mtime_nsec_prev)
+        num_tries--;
     }
 
   if (num_tries == max_tries)
@@ -594,7 +615,7 @@ keyring_acquire_lock (const gchar  *path,
       _log ("Deleted stale lock file '%s'", lock);
 
       /* Try one last time to create it, now that we've deleted the stale one */
-      ret = create_lock_exclusive (lock, error);
+      ret = create_lock_exclusive (lock, NULL, error);
       if (ret < 0)
         goto out;
     }