GNOME Bugzilla – Bug 777808
Deadlock in g_cancellable_disconnect()
Last modified: 2017-04-11 09:31:42 UTC
Created attachment 344353 [details] gdb backrace When user passes a GCancellable object to the async operation and calls g_cancellable_cancel() for any reason, it will cause a deadlock. A recent patch to virt-viewer[1] exposes this problem, but luckily this situation is already documented[2] [1] https://www.redhat.com/archives/virt-tools-list/2017-January/msg00126.html [2] https://developer.gnome.org/gio/stable/GCancellable.html#g-cancellable-disconnect.
Created attachment 344354 [details] [review] Proposed fix
Review of attachment 344354 [details] [review]: ::: govirt/ovirt-proxy.c @@ +217,3 @@ } if ((data->cancellable != NULL) && (data->cancellable_cb_id != 0)) { g_cancellable_disconnect(data->cancellable, data->cancellable_cb_id); I'm wondering if not using an idle, but instead doing this would work: if ((data->cancellable != NULL) && (data->cancellable_cb_id != 0)) { if (g_cancellable_is_cancelled(data->cancellable)) { /* Cancellable has already been cancelled, we don't need to use * g_cancellable_disconnect() to disconnect the signal handler * as we know the 'cancelled' signal is no longer going to be emitted */ g_signal_handler_disconnect(data->cancellable, data->cancellable_cb_id); } else { g_cancellable_disconnect(data->cancellable, data->cancellable_cb_id); } } @@ +218,3 @@ if ((data->cancellable != NULL) && (data->cancellable_cb_id != 0)) { g_cancellable_disconnect(data->cancellable, data->cancellable_cb_id); + g_clear_object(&data->cancellable); data->cancellable is leaked when it's not NULL but data->cancellable_cb_id is 0. You can move the g_clear_object outside of the if(), this should work juste fine. @@ +266,3 @@ + * avoid this + */ + g_idle_add(ovirt_proxy_call_async_data_free, data); Looking a bit at code using GCancellable, there might be an alternative way. @@ +292,3 @@ data->destroy_call_data = destroy_func; if (cancellable != NULL) { + data->cancellable = g_object_ref(cancellable); Imo this part makes sense regardless of how we fix the deadlock
I have just tried this new version without using idle and it works. Which one would be the best?
Created attachment 344407 [details] [review] Alternate solution Disregard the commit message by the way.
(In reply to Eduardo Lima (Etrunko) from comment #3) > I have just tried this new version without using idle and it works. Which > one would be the best? I prefer the alternate solution.
Review of attachment 344407 [details] [review]: ::: govirt/ovirt-proxy.c @@ +292,3 @@ data->destroy_call_data = destroy_func; if (cancellable != NULL) { + data->cancellable = g_object_ref(cancellable); I don't think this belongs to this patch (ie the patch should work even without this ?)
(In reply to Christophe Fergeau from comment #6) > Review of attachment 344407 [details] [review] [review]: > > ::: govirt/ovirt-proxy.c > @@ +292,3 @@ > data->destroy_call_data = destroy_func; > if (cancellable != NULL) { > + data->cancellable = g_object_ref(cancellable); > > I don't think this belongs to this patch (ie the patch should work even > without this ?) Hmm, from your previous comments, I understood that it would make sense to hold the reference to the cancellable either way.
It makes sense, it was needed in the first version iirc. In the second version, I don't think it's required. But regardless of the patch, it makes sense yes, just not sure we should have this change squashed in this ptach.
(In reply to Christophe Fergeau from comment #8) > It makes sense, it was needed in the first version iirc. In the second > version, I don't think it's required. But regardless of the patch, it makes > sense yes, just not sure we should have this change squashed in this ptach. Alright, I will split those patches and submit. Thanks for the review.
Created attachment 344796 [details] [review] Hold reference to cancellable object
Created attachment 344797 [details] [review] Check if operation is cancelled before disconnecting signal
This was pushed a while ago.