GNOME Bugzilla – Bug 794528
Fix segfault caused by use-after-free in GPollFileMonitor
Last modified: 2018-03-26 10:01:04 UTC
This is a follow-up of bug 739424. Now the kqueue backend of GFileMonitor has worked for me for one month without seeing any application crash. However, we still have polling fallback, and it is possible to crash gnome-shell when the fallback is chosen by adding and deleing files in the applications folder hundreds of times. It is probably a rare case, but it did happen when running jhbuild or upgrading packages. In gio/gpollfilemonitor.c, GPollFileMonitor has three pointer members, but g_poll_file_monitor_finalize only unref the first one, which is a GFile*. The other two members, GFileInfo* and GSource*, are leaked when they are not null. A simple printf testing shows that it is possible for these two members to be non-null when g_poll_file_monitor_finalize is called. Leaking GFileInfo* isn't a serious problem except for wasting memory. However, leaking GSource* is actually a use-after-free problem. Since the ref-count of GSource* can never drop to zero, it remains active after the GPollFileMonitor which created it has been freed. When its callback function, poll_file_timeout, is called, it accesses members in the freed GPollFileMonitor struct and crashes the program with segfault because these pointer members has been filled with garbage values. The solution is simple. We should unref everything we has a reference on in g_poll_file_monitor_finalize. After doing so, I see no application or gnome-shell crash when the polling backend is chosen. By the way, the polling backend is slow and it is very likely to make the desktop not responsive under high load. The kqueue backend is much better and gnome-shell on FreeBSD works as smoothly as gnome-shell on Linux when kqueue backend is chosen. Therefore, I will probably submit another follow-up patch to replace the current white-listing approach of only using kqueue on a few known mountpoints, to a black-listing approach of using kqueue on all file systems except for those mounted under /media and /run/media.
Created attachment 369908 [details] [review] gpollfilemonitor: Fix use-after-free caused by leaking GSource
Review of attachment 369908 [details] [review]: Thanks for the analysis and patch. I agree with your analysis, but I think the patch can be condensed a bit. ::: gio/gpollfilemonitor.c @@ +54,3 @@ + if (poll_monitor->last_info) + g_object_unref (poll_monitor->last_info); Nitpick: You can simplify this to `g_clear_object (&poll_monitor->last_info);` @@ +60,3 @@ + g_source_destroy (poll_monitor->timeout); + g_source_unref (poll_monitor->timeout); + } It would be simpler to call g_poll_file_monitor_cancel() at the start of the finalize implementation.
(In reply to Philip Withnall from comment #2) > Review of attachment 369908 [details] [review] [review]: > > Thanks for the analysis and patch. I agree with your analysis, but I think > the patch can be condensed a bit. > > ::: gio/gpollfilemonitor.c > @@ +54,3 @@ > > + if (poll_monitor->last_info) > + g_object_unref (poll_monitor->last_info); > > Nitpick: You can simplify this to `g_clear_object > (&poll_monitor->last_info);` OK, I didn't do this because I thought we don't have to set it to NULL, but yes, it is shorter. > > @@ +60,3 @@ > + g_source_destroy (poll_monitor->timeout); > + g_source_unref (poll_monitor->timeout); > + } > > It would be simpler to call g_poll_file_monitor_cancel() at the start of the > finalize implementation. OK, I will do it although it looks slightly less obvious to me.
Created attachment 369969 [details] [review] gpollfilemonitor: Fix use-after-free caused by leaking GSource
Comment on attachment 369969 [details] [review] gpollfilemonitor: Fix use-after-free caused by leaking GSource Pushed to master. Pushing to glib-2-56 shortly. Attachment 369969 [details] pushed as ba4a953 - gpollfilemonitor: Fix use-after-free caused by leaking GSource
Pushed to master and glib-2-56. Thanks! 5ef27d0e2 (HEAD -> glib-2-56, origin/glib-2-56) gpollfilemonitor: Fix use-after-free caused by leaking GSource ba4a9538e (HEAD -> master, origin/master, origin/HEAD) gpollfilemonitor: Fix use-after-free caused by leaking GSource