packages/glib2
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

glocalfilemonitor: Avoid file monitor destruction from event thread

Browse Source 
(cherry picked from commit 54ebb050937636f3e8cb2db8a02ddb7d2f975b2e)
This commit is contained in:
liningjie 2023-09-25 13:48:40 +08:00 committed by openeuler-sync-bot
parent 4521ef2c0b
commit 70930634b7
4 changed files with 276 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

96
backport-glocalfilemonitor-Avoid-file-monitor-destruction-from-event-thread.patch Normal file
View File

@ -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

76
backport-glocalfilemonitor-Skip-event-handling-if-the-source-has-been-destroyed.patch Normal file
View File

@ -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

97
backport-tests-Add-a-test-for-GFileMonitor-deadlocks.patch Normal file
View File

@ -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

8
glib2.spec
View File

@ -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