From 36987551ffa2e4f2a4e815425fc86cdaa91c929c Mon Sep 17 00:00:00 2001 From: liningjie Date: Mon, 25 Sep 2023 14:07:39 +0800 Subject: [PATCH] glocalfilemonitor: Avoid file monitor destruction from event thread (cherry picked from commit 56e3299845c7e7e30d352c09e2ea4f9eea45576f) --- ...onitor-destruction-from-event-thread.patch | 271 ++++++++++++++++++ glib2.spec | 6 +- 2 files changed, 276 insertions(+), 1 deletion(-) create mode 100644 backport-glocalfilemonitor-Avoid-file-monitor-destruction-from-event-thread.patch diff --git a/backport-glocalfilemonitor-Avoid-file-monitor-destruction-from-event-thread.patch b/backport-glocalfilemonitor-Avoid-file-monitor-destruction-from-event-thread.patch new file mode 100644 index 0000000..7bf787b --- /dev/null +++ b/backport-glocalfilemonitor-Avoid-file-monitor-destruction-from-event-thread.patch @@ -0,0 +1,271 @@ +From 222f6ceada3c54cddf1cfa7a3b846716bafe244c Mon Sep 17 00:00:00 2001 +From: Benjamin Berg +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 ) at ../glib/gthread-posix.c:1493 + #2 0x00007fea69222062 in g_mutex_lock (mutex=mutex@entry=0x7fea6911e148 ) 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=) 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=, child=0x7fea64001460 "spawned-1", rename_to=rename_to@entry=0x0, other=other@entry=0x0, event_time=) at ../gio/glocalfilemonitor.c:457 + #10 0x00007fea6907fe0e in ih_event_callback (event=0x7fea64001420, sub=0x1492620, file_event=) 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=, 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 , user_data=) 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=) 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=) at ../glib/gmain.c:6176 + #21 0x00007fea691f9c2d in g_thread_proxy (data=0x1487cc0) at ../glib/gthread.c:827 + #22 0x00007fea68d93b1a in start_thread (arg=) 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 4f85fea522..f408d07076 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; + } +-- +GitLab + + +From 57bde3c9bda9cfdf1e55fd6ddc1c354bde1ee654 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +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 + +Helps: #1941 +--- + gio/glocalfilemonitor.c | 29 +++++++++++++++++++++++------ + 1 file changed, 23 insertions(+), 6 deletions(-) + +diff --git a/gio/glocalfilemonitor.c b/gio/glocalfilemonitor.c +index f408d07076..68afd7b51c 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 +-- +GitLab + + +From 1fc6a5c9b6a2b620ac4370d64c558f9b33aff063 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +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 + +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 a47aeab386..082f0db26f 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 (); + } +-- +GitLab + diff --git a/glib2.spec b/glib2.spec index a7bccce..2fc7528 100644 --- a/glib2.spec +++ b/glib2.spec @@ -1,6 +1,6 @@ Name: glib2 Version: 2.66.8 -Release: 12 +Release: 13 Summary: The core library that forms the basis for projects such as GTK+ and GNOME License: LGPLv2+ URL: http://www.gtk.org @@ -70,6 +70,7 @@ Patch6059: backport-Implement-GFileIface.set_display_name-for-resource-file patch6060: backport-gdbusinterfaceskeleton-Fix-a-use-after-free-of-a-GDBusMethodInvocation.patch patch6061: backport-CVE-2023-24593_CVE-2023-25180-1.patch patch6062: backport-CVE-2023-24593_CVE-2023-25180-2.patch +patch6063: backport-glocalfilemonitor-Avoid-file-monitor-destruction-from-event-thread.patch BuildRequires: chrpath gcc gcc-c++ gettext perl-interpreter %ifnarch i686 @@ -242,6 +243,9 @@ glib-compile-schemas %{_datadir}/glib-2.0/schemas &> /dev/null || : %endif %changelog +* Mon Sep 25 2023 liningjie - 2.66.8-13 +- glocalfilemonitor: Avoid file monitor destruction from event thread + * Sat Apr 1 2023 hanhuihui - 2.66.8-12 - fix CVE-2023-24593 and CVE-2023-25180 -- Gitee