GNOME Bugzilla – Bug 758361
Crash in mail_client_check_auth_run_cb when goa_mail_auth_run fails
Last modified: 2015-11-26 13:00:02 UTC
I have been seeing crashes like this on my system for a while. If you look at the backtrace then it is obvious that this is a case of goa_main_auth_run_finish returning FALSE without setting the GError. As far as I can see, this can only be caused by our use of g_data_input_stream_read_line, which can return NULL without setting the GError when there is nothing else to read. We are not handling this case, which can lead to this. Backtrace: Program terminated with signal SIGSEGV, Segmentation fault.
+ Trace 235733
$1 = {cancellable = 0x0, input = 0x0, output = 0x0, tls_conn = 0x7fbd50347170 [GTlsClientConnectionGnutls], res = 0x7fbd64003090 [GSimpleAsyncResult], socket = 0x7fbd2c415d00 [GSocket], sc = 0x7fbd3c004dc0 [GSocketClient], conn = 0x7fbd800311e0 [GTcpWrapperConnection], cert_flags = 0, auth = 0x558777e212b0 [GoaImapAuthLogin], tls_type = GOA_TLS_TYPE_STARTTLS, accept_ssl_errors = 1, host_and_port = 0x7fbd18165510 "mail.guillen.com.es", default_port = 143} (gdb)
Created attachment 315916 [details] [review] smtp-auth: Plug leaks in error handling
Created attachment 315917 [details] [review] mail-auth: We want check_cancellable, not handle_cancellation
Created attachment 315918 [details] [review] imap-auth-login, smtp-auth: Always set the GError on failure
Comment on attachment 315916 [details] [review] smtp-auth: Plug leaks in error handling Pushed to master.
Review of attachment 315916 [details] [review]: Already pushed, but ACK.
Review of attachment 315917 [details] [review]: Isn't it roughly equivalent as g_simple_async_result_run_in_thread() is used with these cancellables ?
(In reply to Christophe Fergeau from comment #6) > Review of attachment 315917 [details] [review] [review]: > > Isn't it roughly equivalent as g_simple_async_result_run_in_thread() is used > with these cancellables ? I think g_simple_async_result_set_handle_cancellation is the equivalent of g_task_set_return_on_cancel, which "allows you to create a cancellable wrapper around an uninterruptable function". I don't think we need that here because all the internal methods are cancellable. What we do want is to always return an error when the operation was cancelled.
Review of attachment 315918 [details] [review]: Looks good to me. I would probably have introduced a small g_data_input_stream_read_line() setting an error every time this returns NULL so that the 'if (!ret && local_error == NULL)' checks you added can get a g_warn_if_reached() in case there are other places reaching out: without setting local_error. ::: src/goabackend/goaimapauthlogin.c @@ +475,3 @@ if (credentials == NULL) { + g_prefix_error (&local_error, "Error looking up credentials for IMAP LOGIN in keyring: "); g_prefix_error documentation says "If *err is NULL (ie: an error variable is present but there is no error condition) then also do nothing." Is 'local_error' guaranteed to be non-NULL when credentials == NULL? (probably as goa_utils_lookup_credentials_sync is roughly credentials = g_variant_parse (..., &local_error);
(In reply to Christophe Fergeau from comment #8) > Review of attachment 315918 [details] [review] [review]: > > Looks good to me. I would probably have introduced a small > g_data_input_stream_read_line() setting an error every time this returns > NULL so that the 'if (!ret && local_error == NULL)' checks you added can get > a g_warn_if_reached() in case there are other places reaching out: without > setting local_error. Excellent idea!
Created attachment 316192 [details] [review] imap-auth-login, smtp-auth, utils: Always set the GError on failure
(In reply to Christophe Fergeau from comment #8) > Review of attachment 315918 [details] [review] [review]: > ::: src/goabackend/goaimapauthlogin.c > @@ +475,3 @@ > if (credentials == NULL) > { > + g_prefix_error (&local_error, "Error looking up credentials for > IMAP LOGIN in keyring: "); > > g_prefix_error documentation says "If *err is NULL (ie: an error variable is > present but there is no error condition) then also do nothing." > Is 'local_error' guaranteed to be non-NULL when credentials == NULL? Yes, and looking at the back trace confirms that self->password is already set, so we are not hitting the code path. In fact, these self->password == NULL code paths are totally unused, so maybe we should delete them to avoid useless complexity?