After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 768845 - Don't leak the GSimpleAsyncResult and object path during sign_in_identity
Don't leak the GSimpleAsyncResult and object path during sign_in_identity
Status: RESOLVED FIXED
Product: gnome-online-accounts
Classification: Core
Component: Kerberos
3.18.x
Other All
: Normal normal
: ---
Assigned To: GNOME Online Accounts maintainer(s)
GNOME Online Accounts maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-07-15 14:19 UTC by Debarshi Ray
Modified: 2016-07-15 16:09 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
kerberos: Don't leak the object path from sign_in_identity (1.20 KB, patch)
2016-07-15 14:20 UTC, Debarshi Ray 		committed Details | Review
kerberos: Don't leak the GSimpleAsyncResult in sign_in_identity (886 bytes, patch)
2016-07-15 14:47 UTC, Debarshi Ray 		committed Details | Review

Description Debarshi Ray 2016-07-15 14:19:13 UTC 
sign_in_identity_sync returns a newly allocated object_path. sign_in_thread transfers this pointer back to the calling thread via the GSimpleAsyncResult without using a GDestroyNotify. This is then read out by on_initial_sign_in_done.

So far so good. The async version matches the sync version by returning a newly allocated string.

However, on_initial_sign_in_done should now free the string when it is done with it. It doesn't.

I couldn't get valgrind to detect this leak. Maybe I just missed it in the logs.
Comment 1 Debarshi Ray 2016-07-15 14:20:33 UTC 
Created attachment 331592 [details] [review]
kerberos: Don't leak the object path from sign_in_identity
Comment 2 Debarshi Ray 2016-07-15 14:46:49 UTC 
We are also leaking the GSimpleAsyncResult that is used to implement sign_in_identity.
Comment 3 Ray Strode [halfline] 2016-07-15 14:47:02 UTC 
Review of attachment 331592 [details] [review]:

oh nice find!  fix seems reasonable to me.  Any particular reason you went this route over using the GDestroyNotify ?
Comment 4 Debarshi Ray 2016-07-15 14:47:23 UTC 
Created attachment 331597 [details] [review]
kerberos: Don't leak the GSimpleAsyncResult in sign_in_identity
Comment 5 Debarshi Ray 2016-07-15 14:49:22 UTC 
By the way, I found these while playing with Christophe's GTask patches in bug 764157
Comment 6 Ray Strode [halfline] 2016-07-15 14:52:07 UTC 
Review of attachment 331597 [details] [review]:

clearly right
Comment 7 Debarshi Ray 2016-07-15 15:02:02 UTC 
(In reply to Ray Strode [halfline] from comment #3)
> Review of attachment 331592 [details] [review] [review]:
> 
> oh nice find!  fix seems reasonable to me.  Any particular reason you went
> this route over using the GDestroyNotify ?

Overall, I wanted the async variant to match its sync counterpart for consistency So, the callback of sign_in_identity should get a new string.

Now, I could have used a GDestroyNotify with g_simple_async_result_set_op_res_gpointer but GSimpleAsyncResult is a bit weird and different from GTask in this aspect. Unlike g_task_propagate_pointer, g_simple_async_result_get_op_res_gpointer doesn't transfer ownership (see bug 683376). So, if we use a GDestroyNotify, we either need to g_strdup the return value or NULLify the op_res inside the GSimpleAsyncResult.
Comment 8 Debarshi Ray 2016-07-15 16:09:01 UTC 
Thanks for the reviews, halfline!