After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 794528 - Fix segfault caused by use-after-free in GPollFileMonitor
Fix segfault caused by use-after-free in GPollFileMonitor
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
2.56.x
Other FreeBSD
: High critical
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2018-03-20 13:57 UTC by Ting-Wei Lan
Modified: 2018-03-26 10:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gpollfilemonitor: Fix use-after-free caused by leaking GSource (932 bytes, patch)
2018-03-20 14:00 UTC, Ting-Wei Lan
none Details | Review
gpollfilemonitor: Fix use-after-free caused by leaking GSource (860 bytes, patch)
2018-03-21 15:49 UTC, Ting-Wei Lan
committed Details | Review

Description Ting-Wei Lan 2018-03-20 13:57:43 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.
Comment 1 Ting-Wei Lan 2018-03-20 14:00:28 UTC
Created attachment 369908 [details] [review]
gpollfilemonitor: Fix use-after-free caused by leaking GSource
Comment 2 Philip Withnall 2018-03-21 14:52:34 UTC
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.
Comment 3 Ting-Wei Lan 2018-03-21 15:49:12 UTC
(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.
Comment 4 Ting-Wei Lan 2018-03-21 15:49:32 UTC
Created attachment 369969 [details] [review]
gpollfilemonitor: Fix use-after-free caused by leaking GSource
Comment 5 Philip Withnall 2018-03-26 09:57:31 UTC
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
Comment 6 Philip Withnall 2018-03-26 10:01:04 UTC
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