!162 [sync] PR-157: glocalfilemonitor: Avoid file monitor destruction from event thread

From: @openeuler-sync-bot 
Reviewed-by: @dillon_chen 
Signed-off-by: @dillon_chen

backport-glocalfilemonitor-Avoid-file-monitor-destruction-from-event-thread.patch

@@ -0,0 +1,96 @@
+From 222f6ceada3c54cddf1cfa7a3b846716bafe244c Mon Sep 17 00:00:00 2001
+From: Benjamin Berg <bberg@redhat.com>
+Date: Fri, 18 Mar 2022 12:16:12 +0100
+Subject: [PATCH 1/3] glocalfilemonitor: Avoid file monitor destruction from
+ event thread
+
+Taking a reference to the GFileMonitor when handling events may cause
+object destruction from th worker thread that calls the function. This
+condition happens if the surrounding code drops the otherwise last
+reference ot the GFileMonitor. The series of events causes destruction
+from an unrelated worker thread and also triggers g_file_monitor_cancel
+to be called from g_file_monitor_source_handle_event.
+
+For the inotify backend, this results in a deadlock as cancellation
+needs to take a lock that protects data structures from being modified
+while events are dispatched.
+
+One alternative to this approach might be to add an RCU (release, copy,
+update) approach to the lists contained in the wd_dir_hash and
+wd_file_hash hash tables.
+
+Fixes: #1941
+
+An example stack trace of this happening is:
+
+Thread 2 (Thread 0x7fea68b1d640 (LWP 260961) "gmain"):
+ #0  syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
+ #1  0x00007fea692215dc in g_mutex_lock_slowpath (mutex=mutex@entry=0x7fea6911e148 <g.inotify_lock_lock>) at ../glib/gthread-posix.c:1493
+ #2  0x00007fea69222062 in g_mutex_lock (mutex=mutex@entry=0x7fea6911e148 <g.inotify_lock_lock>) at ../glib/gthread-posix.c:1517
+ #3  0x00007fea6908025a in _ih_sub_cancel (sub=0x1492620) at ../gio/inotify/inotify-helper.c:131
+ #4  0x00007fea6907f9da in g_inotify_file_monitor_cancel (monitor=0x14a3550) at ../gio/inotify/ginotifyfilemonitor.c:75
+ #5  0x00007fea68fae959 in g_file_monitor_cancel (monitor=0x14a3550) at ../gio/gfilemonitor.c:241
+ #6  0x00007fea68fae9dc in g_file_monitor_dispose (object=0x14a3550) at ../gio/gfilemonitor.c:123
+ #7  0x00007fea69139341 in g_object_unref (_object=<optimized out>) at ../gobject/gobject.c:3636
+ #8  g_object_unref (_object=0x14a3550) at ../gobject/gobject.c:3553
+ #9  0x00007fea6907507a in g_file_monitor_source_handle_event (fms=0x14c3560, event_type=<optimized out>, child=0x7fea64001460 "spawned-1", rename_to=rename_to@entry=0x0, other=other@entry=0x0, event_time=<optimized out>) at ../gio/glocalfilemonitor.c:457
+ #10 0x00007fea6907fe0e in ih_event_callback (event=0x7fea64001420, sub=0x1492620, file_event=<optimized out>) at ../gio/inotify/inotify-helper.c:218
+ #11 0x00007fea6908075c in ip_event_dispatch (dir_list=dir_list@entry=0x14c14c0, file_list=0x0, event=event@entry=0x7fea64001420) at ../gio/inotify/inotify-path.c:493
+ #12 0x00007fea6908094e in ip_event_dispatch (event=0x7fea64001420, file_list=<optimized out>, dir_list=0x14c14c0) at ../gio/inotify/inotify-path.c:448
+ #13 ip_event_callback (event=0x7fea64001420) at ../gio/inotify/inotify-path.c:548
+ #14 ip_event_callback (event=0x7fea64001420) at ../gio/inotify/inotify-path.c:530
+ #15 0x00007fea69081391 in ik_source_dispatch (source=0x14a2bf0, func=0x7fea69080890 <ip_event_callback>, user_data=<optimized out>) at ../gio/inotify/inotify-kernel.c:327
+ #16 0x00007fea691d0824 in g_main_dispatch (context=0x14a2cc0) at ../glib/gmain.c:3417
+ #17 g_main_context_dispatch (context=0x14a2cc0) at ../glib/gmain.c:4135
+ #18 0x00007fea691d0b88 in g_main_context_iterate (context=context@entry=0x14a2cc0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:4211
+ #19 0x00007fea691d0c2f in g_main_context_iteration (context=0x14a2cc0, may_block=may_block@entry=1) at ../glib/gmain.c:4276
+ #20 0x00007fea691d0c81 in glib_worker_main (data=<optimized out>) at ../glib/gmain.c:6176
+ #21 0x00007fea691f9c2d in g_thread_proxy (data=0x1487cc0) at ../glib/gthread.c:827
+ #22 0x00007fea68d93b1a in start_thread (arg=<optimized out>) at pthread_create.c:443
+ #23 0x00007fea68e18650 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
+---
+ gio/glocalfilemonitor.c | 14 +++++---------
+ 1 file changed, 5 insertions(+), 9 deletions(-)
+
+diff --git a/gio/glocalfilemonitor.c b/gio/glocalfilemonitor.c
+index 4f85fea52..f408d0707 100644
+--- a/gio/glocalfilemonitor.c
++++ b/gio/glocalfilemonitor.c
+@@ -348,7 +348,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));
+@@ -359,13 +358,11 @@ g_file_monitor_source_handle_event (GFileMonitorSource *fms,
+ 
+   g_mutex_lock (&fms->lock);
+ 
+-  /* monitor is already gone -- don't bother */
+-  instance = g_weak_ref_get (&fms->instance_ref);
+-  if (instance == NULL)
+-    {
+-      g_mutex_unlock (&fms->lock);
+-      return TRUE;
+-    }
++  /* NOTE: 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.
++   */
+ 
+   switch (event_type)
+     {
+@@ -452,7 +449,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;
+ }
+-- 
+2.41.0.windows.3
+

backport-glocalfilemonitor-Skip-event-handling-if-the-source-has-been-destroyed.patch

@@ -0,0 +1,76 @@
+From 57bde3c9bda9cfdf1e55fd6ddc1c354bde1ee654 Mon Sep 17 00:00:00 2001
+From: Philip Withnall <pwithnall@endlessos.org>
+Date: Mon, 30 May 2022 17:54:18 +0100
+Subject: [PATCH 2/3] glocalfilemonitor: Skip event handling if the source has
+ been destroyed
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+This should prevent unbounded growth of the `event_queue` in the
+unlikely case that the `GSource` is removed from its `GMainContext` and
+destroyed separately from the `GFileMonitor`.
+
+I’m not sure if that can currently happen, but it could with future
+refactoring, so it’s best to address the possibility now while we’re
+thinking about this bit of code.
+
+Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
+
+Helps: #1941
+---
+ gio/glocalfilemonitor.c | 29 +++++++++++++++++++++++------
+ 1 file changed, 23 insertions(+), 6 deletions(-)
+
+diff --git a/gio/glocalfilemonitor.c b/gio/glocalfilemonitor.c
+index f408d0707..68afd7b51 100644
+--- a/gio/glocalfilemonitor.c
++++ b/gio/glocalfilemonitor.c
+@@ -358,11 +358,28 @@ g_file_monitor_source_handle_event (GFileMonitorSource *fms,
+ 
+   g_mutex_lock (&fms->lock);
+ 
+-  /* NOTE: 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.
++  /* NOTE:
++   *
++   * 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;
++    }
+ 
+   switch (event_type)
+     {
+@@ -595,9 +612,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
+-- 
+2.41.0.windows.3
+

backport-tests-Add-a-test-for-GFileMonitor-deadlocks.patch

@@ -0,0 +1,97 @@
+From 1fc6a5c9b6a2b620ac4370d64c558f9b33aff063 Mon Sep 17 00:00:00 2001
+From: Philip Withnall <pwithnall@endlessos.org>
+Date: Mon, 30 May 2022 17:55:43 +0100
+Subject: [PATCH 3/3] tests: Add a test for GFileMonitor deadlocks
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+This test is opportunistic in that it’s not possible to detect whether
+the race condition has been hit (other than by hitting a deadlock).
+
+So the only approach we can take for testing is to loop over the code
+which has previously been known to cause a deadlock a number of times.
+
+The number of repetitions is chosen from running the test with the
+deadlock fix reverted.
+
+Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
+
+Helps: #1941
+---
+ gio/tests/testfilemonitor.c | 52 +++++++++++++++++++++++++++++++++++++
+ 1 file changed, 52 insertions(+)
+
+diff --git a/gio/tests/testfilemonitor.c b/gio/tests/testfilemonitor.c
+index a47aeab38..082f0db26 100644
+--- a/gio/tests/testfilemonitor.c
++++ b/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 ();
+ }
+-- 
+2.41.0.windows.3
+

glib2.spec

@@ -1,6 +1,6 @@
 Name:           glib2
 Version:        2.72.2
-Release:        11
+Release:        12
 Summary:        The core library that forms the basis for projects such as GTK+ and GNOME
 License:        LGPLv2+
 URL:            http://www.gtk.org
@@ -67,6 +67,9 @@ patch6057:      backport-add-g_free_sized-and-g_aligned_free_sized.patch
 patch6058:      backport-gkeyfile-Fix-group-comment-management.patch
 patch6059:      backport-gkeyfile-Ensure-we-don-t-add-extra-blank-line-above-new-group.patch
 patch6060:      backport-gkeyfile-Skip-group-comment-when-adding-a-new-key-to-a-group.patch
+patch6061:      backport-glocalfilemonitor-Avoid-file-monitor-destruction-from-event-thread.patch
+patch6062:      backport-glocalfilemonitor-Skip-event-handling-if-the-source-has-been-destroyed.patch
+patch6063:      backport-tests-Add-a-test-for-GFileMonitor-deadlocks.patch
 
 BuildRequires:  chrpath gcc gcc-c++ gettext perl-interpreter
 BUildRequires:  glibc-devel libattr-devel libselinux-devel meson
@@ -262,6 +265,9 @@ glib-compile-schemas %{_datadir}/glib-2.0/schemas &> /dev/null || :
 %endif
 
 %changelog
+* Mon Sep 25 2023 liningjie <liningjie@xfusion.com> - 2.72.2-12
+- glocalfilemonitor: Avoid file monitor destruction from event thread
+
 * Sat Aug 19 2023 hanhuihui <hanhuihui5@huawei.com> - 2.72.2-11
 - fix double unref and fix group comment management