GNOME Bugzilla – Bug 789820
GPollFileMonitor is not cleaning up correctly
Last modified: 2017-11-03 14:09:00 UTC
Created attachment 362827 [details] [review] Suggested fix GPollFileMonitor is consuming memory that is never freed. When schedule_poll_timeout() is called it creates new timeout source (with ref_count = 1 initially) and this newly created source is attached to main context (ref_count is incremented to 2): static void schedule_poll_timeout (GPollFileMonitor* poll_monitor) { poll_monitor->timeout = g_timeout_source_new_seconds (POLL_TIME_SECS); g_source_set_callback (poll_monitor->timeout, poll_file_timeout, poll_monitor, NULL); g_source_attach (poll_monitor->timeout, g_main_context_get_thread_default ()); } But once the timeout is called the initial ref_count is not decremented so when the main context is done with dispatching the ref_count is decremented to 1 and is never freed. This leads in constant memory consumption increase and also high CPU usage as every time the main_context_check() or main_context_prepare() is called it iterates through sources list which gets bigger and bigger: Also poll_monitor->timeout is set to FALSE in poll_file_timeout() though timeout is of type GSource*. Also there is a read-after-free error in file monitor cancel routine, timeout is destroyed and then unref'ed. I believe unreferencing it should be enough (and should be also safer if someone is still referencing the source). static gboolean g_poll_file_monitor_cancel (GFileMonitor* monitor) { GPollFileMonitor *poll_monitor = G_POLL_FILE_MONITOR (monitor); if (poll_monitor->timeout) { g_source_destroy (poll_monitor->timeout); g_source_unref (poll_monitor->timeout); poll_monitor->timeout = NULL; } return TRUE; } Suggested fix attached.
Review of attachment 362827 [details] [review]: Can you please provide your patch as a git-formatted one using `git format-patch` so we can get the attribution correct? Your analysis of the bug looks correct to me, apart from the bit about g_source_destroy(). See my comments below about that. ::: glib/gio/gpollfilemonitor.c.orig @@ -210,3 @@ if (poll_monitor->timeout) { - g_source_destroy (poll_monitor->timeout); Removing this line is incorrect. g_source_destroy() doesn’t drop the refcount of the GSource to 0 and finalise it — it just removes the GSource from the GMainLoop (and hence that ref is dropped). The naming is a bit confusing given the modern meaning of ‘destroy’ in GLib, but that’s what it does.
Created attachment 362902 [details] [review] git format-patch version
Oh, ok. I briefly looked at g_source_destroy() and got the impression that under some circumstances g_source_unref_internal() is called from g_source_destroy() which can free the GSource. I removed it from the patch then.
(In reply to Tomas Kotal from comment #3) > Oh, ok. I briefly looked at g_source_destroy() and got the impression that > under some circumstances g_source_unref_internal() is called from > g_source_destroy() which can free the GSource. Indeed it can, but the GSource will only be freed if that was the *last* ref. The GPollFileMonitor holds another reference to the GSource, which it drops in the g_source_unref() call after g_source_destroy().
Review of attachment 362902 [details] [review]: Looks good to me, thanks.
I tweaked the commit message a little before pushing. Thanks for the patch!