GNOME Bugzilla – Bug 748411
Leak: strings read from GVariant as dups and not freed
Last modified: 2015-04-27 14:57:12 UTC
In panels/power/cc-power-panel.c and panels/sharing/cc-remote-login.c, there are two places where a string is being extracted from a GVariant and treated as a const gchar*, without usign "(&s)": static void add_automatic_suspend_section (CcPowerPanel *self) { [...] if (result) { g_variant_get (result, "(s)", &s); if (g_strcmp0 (s, "yes") == 0) value = 1; g_variant_unref(result); } if (result) [...] } static void state_ready_callback (GObject *source_object, GAsyncResult *result, CallbackData *callback_data) { [...] g_variant_get (state_variant, "(s)", &state_string); [...] } This is leaking those two strings, which should either be freed or treated as real references to memory (no dups)
Created attachment 302288 [details] [review] Patch proposal Simple three-line patch to fix this
Review of attachment 302288 [details] [review]: There are other places in cc-remote-login.c that get this wrong and I'd prefer that the patch actually changes this to g_variant_get_string() and g_variant_get_variant() as applicable for better readability. Also, we generally split patches according to the panel and prefix the commit subject with the panel name. Thanks
(In reply to Rui Matos from comment #2) > Review of attachment 302288 [details] [review] [review]: > > There are other places in cc-remote-login.c that get this wrong and I'd > prefer that the patch actually changes this to g_variant_get_string() and > g_variant_get_variant() as applicable for better readability. > I see, I'll take a look once I have some spare time. Thanks for pointing it out. > Also, we generally split patches according to the panel and prefix the > commit subject with the panel name. Thanks I submitted it this way because it was so small that I thought it would be Ok, but I'm perfectly fine with splitting it. Will come back soon with a follow-up patch (probably not today, though, most likely early next week). Thanks
Created attachment 302439 [details] [review] [1/2] power: Use g_variant_get_string() instead of g_variant_get()
Created attachment 302440 [details] [review] [2/2] sharing: Use g_variant_get_{string|variant}() instead of g_variant_get()
Just uploaded two patches addressing the review comments. Note that I had to use g_variant_get_child_value(), since the original format strings I'm replacing are "(v)" and "(s)". Hope this one looks better, but please let me know if that's not the case. Thanks
Review of attachment 302440 [details] [review]: ++
Review of attachment 302439 [details] [review]: Thanks, I actually forgot that these gdbus apis always return a tuple so this didn't become as simple as I imagined. Oh well
(In reply to Rui Matos from comment #8) > Review of attachment 302439 [details] [review] [review]: > > Thanks, I actually forgot that these gdbus apis always return a tuple so > this didn't become as simple as I imagined. Oh well No problem. I think it's clearer in any way now, thanks for pointing it out. Patches committed, resolving as FIXED.