gnome-wall-clock.c: Fix use-after-free in 'changed' signal handlers. - #276
Open
Tedpac wants to merge 1 commit into
Open
gnome-wall-clock.c: Fix use-after-free in 'changed' signal handlers.#276Tedpac wants to merge 1 commit into
Tedpac wants to merge 1 commit into
Conversation
The "changed" handlers for the GSettings and the GFileMonitor are connected with g_signal_connect() and never disconnected; dispose() only unrefs the two objects. That does not guarantee the handlers stop running. GLib dispatches settings change notifications asynchronously and holds a strong reference on the GSettings while a notification is queued (g_settings_backend_dispatch_signal()). If the clock is finalized in the meantime, the GSettings outlives it, on_schema_change() still runs with a dangling GnomeWallClock, and update_format_string() crashes in g_settings_get_value(). cinnamon-screensaver hits this on resume from suspend: when logind's Active property flips, it destroys and rebuilds its stage, dropping the old clock widget and its WallClock, in the same main loop iteration in which a change to org.cinnamon.desktop.interface is delivered. A manual lock never rebuilds the stage, which is why only resume crashes. Connect both handlers with g_signal_connect_object() so they are disconnected automatically when the clock is destroyed, as gnome-idle-monitor.c and gnome-rr.c already do. Fixes linuxmint/cinnamon-screensaver#503.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes linuxmint/cinnamon-screensaver#503 (filed here originally and moved to cinnamon-screensaver: the process that dies is the screensaver, but the faulting code is this library).
Problem
GnomeWallClockconnectson_schema_changeandon_tz_changedto the"changed"signals of itsGSettingsandGFileMonitorwith plaing_signal_connect()and never disconnects them.dispose()only unrefs the two objects.That is not enough. GLib delivers settings change notifications asynchronously and holds a strong reference on the
GSettingswhile a notification is queued (g_settings_backend_dispatch_signal()), so unref'ing it indispose()does not guarantee it dies with the clock. If the clock is finalized while a notification is in flight, the handler still runs with a danglingself:How cinnamon-screensaver hits it
On resume from suspend, logind's
Activeproperty flips and the screensaver destroys and rebuilds its stage (manager.refresh_stage()), dropping the old clock widget and finalizing itsWallClock. If a change toorg.cinnamon.desktop.interfaceis delivered in that same main loop iteration, the queued handler fires on the freed clock and the screensaver segfaults, leaving the session on the "Something went wrong" fallback screen. A manual lock never rebuilds the stage, which is why only resume crashes.The code is unchanged since the 2013 import and the bug is a race, which is probably why it is rarely reported. Any applet or desklet that uses
WallClockis exposed the same way.Fix
Connect both handlers with
g_signal_connect_object()so they are disconnected automatically when the clock is destroyed, asgnome-idle-monitor.candgnome-rr.calready do.on_tz_changedhas the same latent problem with theGFileMonitor, so it gets the same treatment. No API or ABI change.Testing
Built
gnome-wall-clock.cbefore and after the change into a small test program that creates aGnomeWallClockplus a secondGSettingson the same schema whose"changed"handler drops the clock, mirroring the screensaver's stage teardown. A second process then changesclock-use-24hin an isolated D-Bus/dconf session:g_settings_get_value()(exit 139)Also checked that a live clock still reacts to the same settings change after the patch (the format switches between
00:06and12:06 AM).