Merge branch 'benzea/fix-inotify-race' into 'main'

glocalfilemonitor: Avoid file monitor destruction from event thread

Closes #1941

See merge request GNOME/glib!2561

gio/glocalfilemonitor.c

@@ -350,7 +350,6 @@ g_file_monitor_source_handle_event (GFileMonitorSource *fms,
                                     gint64              event_time)
 {
   gboolean interesting = TRUE;
-  GFileMonitor *instance = NULL;
 
   g_assert (!child || is_basename (child));
   g_assert (!rename_to || is_basename (rename_to));
@@ -361,9 +360,24 @@ g_file_monitor_source_handle_event (GFileMonitorSource *fms,
 
   g_mutex_lock (&fms->lock);
 
-  /* monitor is already gone -- don't bother */
+  /* NOTE:
-  instance = g_weak_ref_get (&fms->instance_ref);
+   *
-  if (instance == NULL)
+   * We process events even if the file monitor has already been disposed.
+   * The reason is that we must not take a reference to the instance here as
+   * destroying it from the event handling thread will lead to a deadlock when
+   * taking the lock in _ih_sub_cancel.
+   *
+   * This results in seemingly-unbounded growth of the `event_queue` with the
+   * calls to `g_file_monitor_source_queue_event()`. However, each of those sets
+   * the ready time on the #GSource, which means that it will be dispatched in
+   * a subsequent iteration of the #GMainContext it’s attached to. At that
+   * point, `g_file_monitor_source_dispatch()` will return %FALSE, and this will
+   * trigger finalisation of the source. That will clear the `event_queue`.
+   *
+   * If the source is no longer attached, this will return early to prevent
+   * unbounded queueing.
+   */
+  if (g_source_is_destroyed ((GSource *) fms))
     {
       g_mutex_unlock (&fms->lock);
       return TRUE;
@@ -454,7 +468,6 @@ g_file_monitor_source_handle_event (GFileMonitorSource *fms,
   g_file_monitor_source_update_ready_time (fms);
 
   g_mutex_unlock (&fms->lock);
-  g_clear_object (&instance);
 
   return interesting;
 }
@@ -601,9 +614,9 @@ g_file_monitor_source_dispose (GFileMonitorSource *fms)
 
   g_file_monitor_source_update_ready_time (fms);
 
-  g_mutex_unlock (&fms->lock);
-
   g_source_destroy ((GSource *) fms);
+
+  g_mutex_unlock (&fms->lock);
 }
 
 static void

gio/tests/testfilemonitor.c

@@ -1036,6 +1036,57 @@ test_file_hard_links (Fixture       *fixture,
   g_object_unref (data.output_stream);
 }
 
+static void
+test_finalize_in_callback (Fixture       *fixture,
+                           gconstpointer  user_data)
+{
+  GFile *file = NULL;
+  guint i;
+
+  g_test_summary ("Test that finalization of a GFileMonitor in one of its "
+                  "callbacks doesn’t cause a deadlock.");
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/1941");
+
+  file = g_file_get_child (fixture->tmp_dir, "race-file");
+
+  for (i = 0; i < 50; i++)
+    {
+      GFileMonitor *monitor = NULL;
+      GError *local_error = NULL;
+
+      /* Monitor the file. */
+      monitor = g_file_monitor_file (file, G_FILE_MONITOR_NONE, NULL, &local_error);
+      g_assert_no_error (local_error);
+      g_assert_nonnull (monitor);
+
+      /* Create the file. */
+      g_file_replace_contents (file, "hello", 5, NULL, FALSE,
+                               G_FILE_CREATE_NONE, NULL, NULL, &local_error);
+      g_assert_no_error (local_error);
+
+      /* Immediately drop the last ref to the monitor in the hope that this
+       * happens in the middle of the critical section in
+       * g_file_monitor_source_handle_event(), so that any cleanup at the end
+       * of that function is done with a now-finalised file monitor. */
+      g_object_unref (monitor);
+
+      /* Re-create the monitor and do the same again for deleting the file, to
+       * give a second chance at hitting the race condition. */
+      monitor = g_file_monitor_file (file, G_FILE_MONITOR_NONE, NULL, &local_error);
+      g_assert_no_error (local_error);
+      g_assert_nonnull (monitor);
+
+      /* Delete the file. */
+      g_file_delete (file, NULL, &local_error);
+      g_assert_no_error (local_error);
+
+      /* Drop the ref again. */
+      g_object_unref (monitor);
+    }
+
+  g_object_unref (file);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -1047,6 +1098,7 @@ main (int argc, char *argv[])
   g_test_add ("/monitor/dir-not-existent", Fixture, NULL, setup, test_dir_non_existent, teardown);
   g_test_add ("/monitor/cross-dir-moves", Fixture, NULL, setup, test_cross_dir_moves, teardown);
   g_test_add ("/monitor/file/hard-links", Fixture, NULL, setup, test_file_hard_links, teardown);
+  g_test_add ("/monitor/finalize-in-callback", Fixture, NULL, setup, test_finalize_in_callback, teardown);
 
   return g_test_run ();
 }