sn-item: cancel in-flight async calls on dispose to fix SIGSEGV#201
Open
leigh123linux wants to merge 1 commit into
Open
sn-item: cancel in-flight async calls on dispose to fix SIGSEGV#201leigh123linux wants to merge 1 commit into
leigh123linux wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a SIGSEGV in the status notifier item watcher by ensuring in-flight async D-Bus calls are cancelled during object disposal, preventing callbacks from touching cleared proxy state.
Changes:
- Cancel
GCancellableinsn_item_dispose()before clearing proxies to abort in-flight/queued async operations safely. - Add a NULL-guard in
get_all_properties_callback()to bail out if proxies were cleared before processing results.
Comments suppressed due to low confidence (1)
xapp-sn-watcher/sn-item.c:181
- Now that
dispose()actively cancelsitem->cancellable, any pending async ops (e.g. theg_dbus_proxy_new()ininitialize_item()) will complete withG_IO_ERROR_CANCELLED. Inproperty_proxy_acquired(), the CANCELLED branch currently does notg_error_free(error)and does not return, so it will continue initializing the item withprop_proxy == NULL. Please make the CANCELLED path free the error and return early to avoid leaks and inconsistent state.
g_cancellable_cancel (item->cancellable);
g_clear_pointer (&item->sortable_name, g_free);
g_clear_object (&item->status_icon);
g_clear_object (&item->prop_proxy);
g_clear_object (&item->sn_item_proxy);
g_clear_object (&item->cancellable);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d15c7a8 to
b821f09
Compare
Comment on lines
+175
to
+177
| if (item->cancellable != NULL) | ||
| g_cancellable_cancel (item->cancellable); | ||
|
|
Member
Author
There was a problem hiding this comment.
That was your suggestion, are you an idiot?
Comment on lines
+765
to
+769
| if (item->prop_proxy == NULL || item->sn_item_proxy == NULL) | ||
| { | ||
| g_clear_pointer (&properties, g_variant_unref); | ||
| return; | ||
| } |
b821f09 to
5f5aa03
Compare
5f5aa03 to
9ff3b41
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
xapp-sn-watcher/sn-item.c:1178
- With the new dispose-time
g_cancellable_cancel(),g_dbus_proxy_new_finish()can now fail withG_IO_ERROR_CANCELLED. In that case this callback currently neither freeserrornor returns, and it proceeds withitem->prop_proxy == NULLto connect signals/create the status icon/queue updates, which can lead to crashes or inconsistent state. Handle the cancelled case explicitly by freeingerrorand returning early (and ensure theSnItemref is released in the same cleanup path).
item->prop_proxy = g_dbus_proxy_new_finish (res, &error);
if (error != NULL)
{
if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
{
g_critical ("Could not get property proxy for %s: %s\n",
g_dbus_proxy_get_name (item->sn_item_proxy),
error->message);
g_error_free (error);
return;
}
}
Comment on lines
+737
to
+738
| SnItem *item = SN_ITEM (user_data); | ||
| g_object_unref (item); |
Comment on lines
1160
to
1166
| SnItem *item = SN_ITEM (user_data); | ||
| GError *error = NULL; | ||
| gchar *json = NULL; | ||
|
|
||
| g_object_unref (item); | ||
|
|
||
| item->prop_proxy = g_dbus_proxy_new_finish (res, &error); |
9ff3b41 to
e479710
Compare
Comment on lines
+175
to
+176
| if (item->cancellable != NULL) | ||
| g_cancellable_cancel (item->cancellable); |
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.
The crash occurs when a remote D-Bus peer disconnects while an async
g_dbus_proxy_call() for GetAll properties is still in flight. When the
callback fires it calls g_dbus_proxy_get_name(item->sn_item_proxy) in the
error path, but sn_item_proxy may have been cleared by sn_item_dispose(),
leaving a dangling pointer (observed value 0x4e7375746174532f = "/StatusN").
Cancel the GCancellable in sn_item_dispose() before clearing the proxy
objects so that any in-flight or queued async callbacks receive a
cancellation error and return early via the existing G_IO_ERROR_CANCELLED
check, without touching freed state.
A NULL guard on item->prop_proxy and item->sn_item_proxy is added in the
success path of get_all_properties_callback() as a belt-and-suspenders
check against any future code path that clears the proxy without going
through dispose().
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2477415
https://bugzilla.redhat.com/show_bug.cgi?id=2428140