Bug 51842

*** Bug 51454 has been marked as a duplicate of this bug. ***

I've understood how this can happen:
in mcd-account-manager-default.c::_delete_from_keyring() it does:
      gchar *removed = g_strdup (account);
      g_hash_table_insert (amd->removed_accounts, removed, removed);

So what happens if the account is already in the hash table? Doc says:
"""
If the key already exists in the GHashTable its current value is replaced with the new value. If you supplied a value_destroy_func when creating the GHashTable, the old value is freed using that function. If you supplied a key_destroy_func when creating the GHashTable, the passed key is freed using that function.
"""

Since @removed is passed both as value and key, it gets inserted as value but freed as key => we get a freed string as value!!

Created attachment 64015 [details] [review]
McdAccountManagerDefault: Fix a possible double free

If the account is already in the hashtable, g_hash_table_insert()
will set @removed as value, but free it since the key already in
the table is kept.

Note that we could use g_hash_table_add() but that's new in glib 2.32 and MC still depends on 2.30. In master that's certainly OK, but probably not for backporting in stable branch.

Can you write a test for this perchance?

Doesn't it sound weird that MC is actually calling _delete_from_keyring() function from mcd_account_connection_self_nickname_changed_cb() ?

Right, that looks suspicious. Also I'm starting to wonder if I really fixed this issue.

As explained above, we can end with freed strings as value in the hash table, but the value is never used... only the key is. And I don't see how the key could be invalid.

Ok, it continuously wants to remove *GOA* accounts from default storage (keyring and .mission-control/accounts/accounts.cfg), because they are not stored there.

My patch really fix the issue, I was able reproduce the valgrind logs and with the patch I can't anymore.

I've absolutely no idea how to test this behaviour though, that involves having accounts on other storage than the default one... and even then you need valgrind to catch the problem, it does not always crash.

MC already has a test account storage -- the diverted plugin.

All accounts will be saved in the non-default account storage unless the account parameter starts with 'dontdivert'.

I couldn't reproduce this in a unit test. I've spent enough time on this 1 line fix, if someone knows how MC tests works please do it :-)

*** Bug 51813 has been marked as a duplicate of this bug. ***