Created attachment 281325 [details] [review]
daemon: Fix crash when typing password at screen lock

open_reauthentication_requests is a hashtable which contains
GDBusMethodInvocations as values. We insert them in the hashtable with
only one reference, the one coming from the D-Bus method call.

But when a reauthentication starts,
gdm_dbus_manager_complete_open_reauthentication_channel() is called
which internally calls g_dbus_method_invocation_return_value().
This will eat the only reference, and we'll then try to remove the
invocation from the hash table, unref'ing a dead object.

Instead, remove the invocation from the hashtable without unref'ing
(once too much).

Tested on top of Fedora 21's gdm package, and it fixes the crash for me.

Review of attachment 281325 [details] [review]:

Hey thanks for this.

::: daemon/gdm-manager.c
@@ +1802,3 @@
+        } else {
+                g_hash_table_remove (manager->priv->open_reauthentication_requests,
+                                     source_tag);

So I'm not sure this is right, you steal if the look up succeeds but remove if the lookup fails.  If it fails, though, then remove is going to be a noop! So, the remove call added here is dead code.  You could just always steal, but then the question is, why are we using a destroy function to unref when removing from the hash table if we're just going to steal instead of remove? I think one of these two fixes is more right than the above fix:

1) don't pass g_object_unref to g_hash_table_new_full
2) ref the invocation request when inserting it

Either way seems like it should work.

(In reply to comment #3)
<snip>
> 1) don't pass g_object_unref to g_hash_table_new_full
> 2) ref the invocation request when inserting it

No, because you want all the pending invocations to be deleted/cancelled/unref'ed when finalizing the object. Just stealing is probably fine, or we could remove the unref as a destructor, get all the values on finalize, and unref them one-by-one.

Created attachment 281351 [details] [review]
daemon: Fix crash when typing password at screen lock

open_reauthentication_requests is a hashtable which contains
GDBusMethodInvocations as values. We insert them in the hashtable with
only one reference, the one coming from the D-Bus method call.

But when a reauthentication starts,
gdm_dbus_manager_complete_open_reauthentication_channel() is called
which internally calls g_dbus_method_invocation_return_value().
This will eat the only reference, and we'll then try to remove the
invocation from the hash table, unref'ing a dead object.

Instead, remove the invocation from the hashtable without unref'ing
(once too much).

Review of attachment 281351 [details] [review]:

looks good. thanks, agin. i'd do one minor tweak..

::: daemon/gdm-manager.c
@@ +1799,3 @@
                                                                          address);
+                g_hash_table_steal (manager->priv->open_reauthentication_requests,
+                                    source_tag);

I'd put this above the complete call.  it doesn't matter in practice because we aren't dereferencing the pointer, but it reads better (in my mind anyway)

Created attachment 281398 [details] [review]
daemon: Fix crash when typing password at screen lock

open_reauthentication_requests is a hashtable which contains
GDBusMethodInvocations as values. We insert them in the hashtable with
only one reference, the one coming from the D-Bus method call.

But when a reauthentication starts,
gdm_dbus_manager_complete_open_reauthentication_channel() is called
which internally calls g_dbus_method_invocation_return_value().
This will eat the only reference, and we'll then try to remove the
invocation from the hash table, unref'ing a dead object.

Instead, remove the invocation from the hashtable without unref'ing
(once too much).

Attachment 281398 [details] pushed as 52e5154 - daemon: Fix crash when typing password at screen lock