GNOME Bugzilla – Bug 726353
Leaking credentials, cursors, principals and GDateTime
Last modified: 2014-03-17 15:06:40 UTC
See the downstream Fedora 20 bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1046197 It has a patch and valgrind logs.
Created attachment 272004 [details] [review] kerberos: Leaking credentials, cursors, principals and GDateTime This is a slightly tweaked and more complete version of the patch Michael submitted. However, there is one line that I am not sure about, which I will subsequently point out.
Review of attachment 272004 [details] [review]: ::: src/goaidentity/goakerberosidentity.c @@ +642,3 @@ } + krb5_free_cred_contents (self->priv->kerberos_context, &credentials); Do we really need this line? If we succeeded in getting the next credential, then we are going to free it up in the next iteration of the loop. It appears to me that if we fail to do so, then we don't need to free it up.
Created attachment 272008 [details] [review] identity: Plug a few leaked strings
This part of the valgrind logs appear consistent with the reports of leaking file descriptors (most likely timerfds) that we are still getting: ==24607== 64 bytes in 1 blocks are possibly lost in loss record 2,385 of 3,673 ==24607== at 0x4C291D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==24607== by 0xBB73EC6: g_malloc0 (gmem.c:134) ==24607== by 0xB8E31D4: g_closure_new_simple (gclosure.c:206) ==24607== by 0xB8FF264: g_source_set_dummy_callback (gsourceclosure.c:319) ==24607== by 0xB605C8C: g_unix_input_stream_pollable_create_source (gunixinputstream.c:519) ==24607== by 0x416312: goa_alarm_set_time (goaalarm.c:461) ==24607== by 0x410700: set_alarm.isra.1 (goakerberosidentity.c:789) ==24607== by 0x410900: reset_alarms (goakerberosidentity.c:881) ==24607== by 0x412037: goa_kerberos_identity_update (goakerberosidentity.c:1310) ==24607== by 0x413F5B: on_job_scheduled (goakerberosidentitymanager.c:426) ==24607== by 0xB5CBF35: io_job_thread (gioscheduler.c:89) ==24607== by 0xB5EB864: g_task_thread_pool_thread (gtask.c:1245) I will take a closer look when I am on a network with Kerberos on it.
(In reply to comment #2) > Do we really need this line? If we succeeded in getting the next credential, > then we are going to free it up in the next iteration of the loop. It appears > to me that if we fail to do so, then we don't need to free it up. Reviewing this, yes, if krb5_cc_next_cred() doesn't allocate on failure then it is unnecessary. I was going to submit a git patch this weekend (now) but I see you've already taken this over. Do you still want me to supply a revised patch?
Created attachment 272033 [details] [review] kerberos: Leaking credentials, cursors, principals and GDateTime
(In reply to comment #5) > (In reply to comment #2) > > Do we really need this line? If we succeeded in getting the next credential, > > then we are going to free it up in the next iteration of the loop. It appears > > to me that if we fail to do so, then we don't need to free it up. > > Reviewing this, yes, if krb5_cc_next_cred() doesn't allocate on failure then it > is unnecessary. Removed it. Thanks for taking a look. > I was going to submit a git patch this weekend (now) but I see you've already > taken this over. Do you still want me to supply a revised patch? We still need to investigate the GoaAlarms issue.
So the patches look fine, but they squish a number of changes together. I've split them out for readability. Will attach and push.
Created attachment 272156 [details] [review] kerberos: fix leak in register_error_domain In the event a type was not known, we were leaking the type name.
Created attachment 272157 [details] [review] kerberos: refactor register_error_domain This commit move type class freeing to the otherside of the "out" label to make the code robust against potential future changes.
Created attachment 272158 [details] [review] kerberos: make enum_class in register_error_domain const Just as a defensive programming measure.
Created attachment 272159 [details] [review] kerberos: fix leak in add_temporary_account The account description was not getting freed.
Created attachment 272160 [details] [review] kerberos: don't free alarm in set_alarm, free in callers It's a little unexpected that set_alarm "eats" the alarm passed in. This commit makes it the caller's responsibility to free the alarm to more closely match typical practice.
Created attachment 272161 [details] [review] kerberos: fix leak in reset_alarms A GDateTime "now" object wasn't getting cleaned up after use.
Created attachment 272162 [details] [review] kerberos: fix principal leak in identity_renew The code carefully freed the principal in all error cases, but then failed to free the principal in the non-error case!
Created attachment 272163 [details] [review] kerberos: consolidate exit path code in identity_renew the principal is freed in 3 different places. This commit consolidates it to one place for clarity and added robustness against future changes.
Created attachment 272164 [details] [review] kerberos: fix leak in verify_identity The kerberos API allocates credentials on the heap, when iterating over a credential collection (for each iteration). This commit makes sure each of those credentials is freed.
Created attachment 272165 [details] [review] kerberos: make sure credential cache sequence is always explicitly ended The code currently fails to pair krb5_cc_start_seq_get with krb5_cc_end_seq_get in an error case. This can lead the cursor getting leaked.
Attachment 272156 [details] pushed as c679b88 - kerberos: fix leak in register_error_domain Attachment 272157 [details] pushed as 0ab3d27 - kerberos: refactor register_error_domain Attachment 272158 [details] pushed as 16bcb7c - kerberos: make enum_class in register_error_domain const Attachment 272159 [details] pushed as c4c47a8 - kerberos: fix leak in add_temporary_account Attachment 272160 [details] pushed as 3060fdf - kerberos: don't free alarm in set_alarm, free in callers Attachment 272161 [details] pushed as cc2c0ac - kerberos: fix leak in reset_alarms Attachment 272162 [details] pushed as 4923d99 - kerberos: fix principal leak in identity_renew Attachment 272163 [details] pushed as d020a84 - kerberos: consolidate exit path code in identity_renew Attachment 272164 [details] pushed as 16ed9d4 - kerberos: fix leak in verify_identity Attachment 272165 [details] pushed as 2e41de2 - kerberos: make sure credential cache sequence is always explicitly ended
Thanks, Ray. Could these be backported to 3-12 and 3-10?
To ssh://halfline@git.gnome.org/git/gnome-online-accounts 91d2059..007d9fd gnome-3-12 -> gnome-3-12 0b0c715..30e67d1 gnome-3-10 -> gnome-3-10