Skip to content

xapp-sn-watcher: Don't use the item after it has been disposed - #212

Open
zquestz wants to merge 2 commits into
linuxmint:masterfrom
zquestz:sn-watcher-dispose-safety
Open

xapp-sn-watcher: Don't use the item after it has been disposed#212
zquestz wants to merge 2 commits into
linuxmint:masterfrom
zquestz:sn-watcher-dispose-safety

Conversation

@zquestz

@zquestz zquestz commented Aug 29, 2026

Copy link
Copy Markdown

property_proxy_acquired() has a use-after-free window on item teardown.
Found by reading during the same investigation as #210 and #211; there is
no deterministic reproducer, so the reasoning is spelled out below.

The watcher's hash table holds the only reference to an SnItem. When a
client leaves the bus, handle_sn_item_name_owner_lost() removes it and
dispose/finalize run immediately. But the g_dbus_proxy_new() call started
by initialize_item() holds no reference to the item, only a bare pointer
in user_data, and its callback may not have run yet. When it does:

  • it writes item->prop_proxy through the freed pointer before looking at
    the error
  • on G_IO_ERROR_CANCELLED it falls out of the error branch (there is no
    return on that path) and carries on connecting signals and building a
    status icon on the freed item, and leaks the GError as well

Any client that registers and exits again before the property proxy
arrives can hit this. A client stuck in a register/exit loop, which is
what a crashing tray app looks like, rolls those dice continuously.

Note that checking the error before touching the item would not be enough
on its own: cancellation is racy. If the async result was already queued
when sn_item_dispose() cancelled the cancellable, the callback runs with a
successful result and no CANCELLED error, on an item that is already gone.
The item has to stay alive until the callback has run.

The fix:

  • initialize_item() passes g_object_ref (item) as callback data, so the
    in-flight call keeps the item alive; dispose is simply deferred until
    the callback drops its reference
  • property_proxy_acquired() finishes into a local variable, inspects the
    error before touching the item, and returns cleanly on any error
    including cancellation (which also fixes the GError leak)

Verified that a build with this change compiles without warnings and that
items still register, publish, and are removed normally on a private
session bus.

Update: a second commit applies the same treatment to the GetAll call in
update_all_properties(). Its callback has the identical shape (bare item
pointer, guarded only by the cancellable) and the identical race: a client
that exits can have its GetAll failure queued before the watcher processes
NameOwnerChanged, so the callback runs after the item is freed and calls
g_dbus_proxy_get_name() through the freed pointer in the error branch.
This appears to be the crash in #189 and #194: both core dumps show SIGSEGV
in g_dbus_proxy_get_name called from watcher code inside a GIO async
completion, triggered by an application exiting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant