memory leak in menuitem_property_idle

[JKL]

This bug discusses the segment of code beginning at line 955 of server.c

http://bazaar.launchpad.net/~dbusmenu-team/dbusmenu/trunk/view/head:/libdbusmenu-glib/server.c

The megadata variants are created in two possible ways. One involves g_variant_builder_end, and one involves g_variant_parse. The memory allocation strategy is to either pass the megadata as a tuple to g_dbus_connection_emit_signal, in which case that function takes over responsibility, or free the individual variants using g_variant_unref.

This strategy is incorrect. As documented below, g_variant_builder returns a floating reference, whereas g_variant_parse returns a regular reference. Actually, the documentation for the latter is not explicit, but it appears that it what it indicates.

http://developer.gnome.org/glib/stable/glib-GVariant.html

There are at least two problems here.

1. g_variant_unref does not work for floating references, so it cannot free variants created by g_variant_builder_end

2. the g_variant_new_tuple constructor does not take ownership of its arguments if they are standard references

The implementation needs to ensure that both of the megadata variants are properly freed in all code paths. There are likely many ways to due this, and I suspect that irrespective of whatever I suggest, you will probably do something slightly different. In essence, there are two ways each can be created, and two ways they can be destroyed, for a total of 4 possibilities. The code needs to account for them all.

Below are valgrind logs confirming the problem.

==10301== 1,480 bytes in 37 blocks are definitely lost in loss record 9,062 of 9,326
==10301== at 0x4C28FAC: malloc (vg_replace_malloc.c:236)
==10301== by 0x8F62A62: g_malloc (gmem.c:164)
==10301== by 0x8F79666: g_slice_alloc (gslice.c:842)
==10301== by 0x8F97A09: g_variant_alloc (gvariant-core.c:475)
==10301== by 0x8F97B0C: g_variant_new_from_children (gvariant-core.c:560)
==10301== by 0x8F9532D: g_variant_builder_end (gvariant.c:3260)
==10301== by 0x8F98E67: array_get_value (gvariant-parser.c:901)
==10301== by 0x8F98699: maybe_wrapper (gvariant-parser.c:831)
==10301== by 0x8F9B5FF: g_variant_parse (gvariant-parser.c:515)
==10301== by 0x5046D8E: menuitem_property_idle (server.c:963)
==10301== by 0x8F5BBCC: g_main_context_dispatch (gmain.c:2440)
==10301== by 0x8F5C3A7: g_main_context_iterate.clone.6 (gmain.c:3091)
==10301== by 0x8F5C9F1: g_main_loop_run (gmain.c:3299)
==10301== by 0x416D77: main (main.c:101)

==10301== 480 bytes in 12 blocks are definitely lost in loss record 8,789 of 9,326
==10301== at 0x4C28FAC: malloc (vg_replace_malloc.c:236)
==10301== by 0x8F62A62: g_malloc (gmem.c:164)
==10301== by 0x8F79666: g_slice_alloc (gslice.c:842)
==10301== by 0x8F97A09: g_variant_alloc (gvariant-core.c:475)
==10301== by 0x8F97B0C: g_variant_new_from_children (gvariant-core.c:560)
==10301== by 0x8F9532D: g_variant_builder_end (gvariant.c:3260)
==10301== by 0x8F98E67: array_get_value (gvariant-parser.c:901)
==10301== by 0x8F98699: maybe_wrapper (gvariant-parser.c:831)
==10301== by 0x8F9B5FF: g_variant_parse (gvariant-parser.c:515)
==10301== by 0x5046E13: menuitem_property_idle (server.c:976)
==10301== by 0x8F5BBCC: g_main_context_dispatch (gmain.c:2440)
==10301== by 0x8F5C3A7: g_main_context_iterate.clone.6 (gmain.c:3091)
==10301== by 0x8F5C9F1: g_main_loop_run (gmain.c:3299)
==10301== by 0x416D77: main (main.c:101)

[JKL (jkl102001)]

In the above post I expressed some doubt as to whether g_variant_parse returns a regular reference or floating reference. Indeed the documentation is not completely explicit on this point.

However, reading the source makes it clear that it is a non-floating reference, as I have claimed.

http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/oneiric/glib2.0/oneiric/view/head:/glib/gvariant-parser.c

Note the call to g_variant_ref_sink.

[JKL (jkl102001)]

There are numerous reference leaks in server.c. It would be easier to deal with them all in one place.

https://bugs.launchpad.net/dbusmenu/+bug/784890

[Kevin Turner (keturn)]

The linked bug #784890 was marked as fixed, but the g_variant_parse leak in menuitem_property_idle still is present in libdbusmenu-0.6.2 in precise, according to massif (attached). And while I'm not sure I would have spotted it on my own, reading JKL's detailed explanation above and looking at the current source reads like the condition described is still present; if g_variant_unref is not effective on the return value of g_variant_parse, then there's nothing freeing it.

[Kevin Turner (keturn)]

disregard that last sentence about g_variant_unref and g_variant_parse, I read that backwards.

Upon further reading, I see that JKL's patch was applied, and so maybe this *was* fixed, but http://bazaar.launchpad.net/~dbusmenu-team/dbusmenu/trunk.0.6/revision/362.4.1 added an additional ref to each of those megadata elements.