GNOME Bugzilla – Bug 786811
Crash on startup in secret_service_search_cb()
Last modified: 2017-08-25 21:31:28 UTC
Epiphany is crashing for me in startup in secret_service_search_cb() in ephy-password-manager.c. Right here: sscanf (timestamp, "%lf", &server_time_modified); Problem is timestamp can be NULL there. This patch fixes it by not trying to set the timestamp, but I don't know if it's right.
+ Trace 237871
Created attachment 358438 [details] [review] password-manager: Don't try to set timestamp if missing
(In reply to Michael Catanzaro from comment #0) > Problem is timestamp can be NULL there. Wrong, it's actually the string "0"
Created attachment 358449 [details] [review] password-manager: Handle invalid input from gnome-keyring Although unlikely, gnome-keyring data may get broken or corrupted for whatever reason. EphyPasswordManager should not crash in that case though, but rather handle it by trying to fix the corrupted records if possible or delete them if not.
Created attachment 358450 [details] [review] password-manager: Handle invalid input from gnome-keyring Although unlikely, gnome-keyring data may get broken or corrupted for whatever reason. EphyPasswordManager should not crash in that case though, but rather handle it by trying to fix the corrupted records if possible or delete them if not. Forgot to unref original if it cannot be fixed.
Review of attachment 358449 [details] [review]: Hm, thanks for working on this. I think I would favor a simpler approach. ::: lib/sync/ephy-password-manager.c @@ +58,2 @@ static void ephy_synchronizable_manager_iface_init (EphySynchronizableManagerInterface *iface); +static void ephy_password_manager_forget_record (EphyPasswordManager *self, If the record is corrupted, I would leave it be rather than deleting it. There may be salvageable information. E.g. if the origin is somehow corrupted, the password might still be there. Or vice-versa, so it would at least be possible for the user to know which passwords have been lost. Of course both of these cases are extremely unlikely, and not really worth bothering about, but I'm trying to make an argument for doing nothing rather than deleting the records. @@ +566,3 @@ + + if (!hostname || !password_field) { + /* Unfixable corrupted record. Delete it. */ So here I would rather do nothing. g_free (original) as you noticed on IRC that you're missing, and then goto next. @@ +574,2 @@ + if (!id || !target_origin || !timestamp) { + /* Fixable corrupted record. Replace the original. */ Hm, this is a lot of extra code to try fixing records that we know are unlikely to become corrupted. I would remove all of this. All we need to do is not crash. So what I would do is if (!hostname || !password_field || !id || !target_origin || !timestamp), goto next. Also, and this is important as it's very confusing: I failed to notice earlier in my previous code review that we're storing security origins in variables named hostname. I'll file another bug for this.
Created attachment 358451 [details] [review] password-manager: Handle invalid input from gnome-keyring Although unlikely, gnome-keyring data may get broken or corrupted for whatever reason. Epiphany should not crash in that case. According to comment 5: Just skip corrupted records, don't attempt to fix them or delete them.
Comment on attachment 358451 [details] [review] password-manager: Handle invalid input from gnome-keyring Isn't that nicer!
Attachment 358451 [details] pushed as 3980ca9 - password-manager: Handle invalid input from gnome-keyring