From ee4d93c6fd5d470343c1f1776270b55f685632ba 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 f5b0090ce..5de39ced1 100644 --- a/gio/glocalfilemonitor.c +++ b/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,13 +360,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) { @@ -454,7 +451,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; } From 8e9f238667d376d3a7bb0b25814d5ad78d6f38d9 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 5de39ced1..fde52193a 100644 --- a/gio/glocalfilemonitor.c +++ b/gio/glocalfilemonitor.c @@ -360,11 +360,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) { @@ -597,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 From e5a691e84af9c63c150b8b99370b7374b7ce6984 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 bbc61f8b0..7274f792e 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 (); }