DBus Menu

multiple reference leaks in server.c

Bug #784890 reported by JKL on 2011-05-18

This bug affects 2 people

	Status	Importance	Assigned to	Milestone
DBus Menu	Fix Released	Undecided	Unassigned	DBus Menu 0.4.90
0.4	Fix Released	Undecided	Unassigned	DBus Menu 0.4.5
0.5	Fix Released	Undecided	Unassigned	DBus Menu 0.4.90
libdbusmenu (Ubuntu)	Fix Released	Undecided	Unassigned

Bug Description

This bug discusses the segment of code beginning at line 1192:

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

At 1199, we have this line:

const gchar ** props = g_variant_get_strv(g_variant_get_child_value(params, 2), NULL);

According to the documentation, g_variant_get_strv returns a shallow copy of a string array, and the array itself must be freed using g_free, although the underlying strings must not be freed.

http://developer.gnome.org/glib/stable/glib-GVariant.html#g-variant-get-strv

The 'props' array is not freed properly, creating a memory leak. Perhaps the correct place to call g_free is at line 1212, after the code block which creates the items variant using the props data.

Below is a valgrind log illustrating the problem.

==10301== 1,873 (1,824 direct, 49 indirect) bytes in 38 blocks are definitely lost in loss record 9,093 of 9,326
==10301== at 0x4C28FAC: malloc (vg_replace_malloc.c:236)
==10301== by 0x8F62A62: g_malloc (gmem.c:164)
==10301== by 0x8F932A2: g_variant_get_strv (gvariant.c:1391)
==10301== by 0x5046045: bus_get_layout (server.c:1199)
==10301== by 0x6D1834F: call_in_idle_cb (gdbusconnection.c:4434)
==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)

Tags:

Related branches

lp:~ted/libdbusmenu/lp784890

Merged into lp:libdbusmenu/0.5 at revision 309

DBus Menu Team: Pending requested 2011-06-17

Merged into lp:libdbusmenu/0.4 at revision 305

DBus Menu Team: Pending requested 2011-06-17

lp:~dbusmenu-team/libdbusmenu/ubuntu-0-4

lp:~dbusmenu-team/libdbusmenu/ubuntu

Rejected for merging into lp:~ubuntu-desktop/libdbusmenu/ubuntu

Ken VanDine: Disapprove on 2011-10-13

Superseded for merging into lp:libdbusmenu/0.5

Ken VanDine: Pending requested 2011-01-27

Diff: 67319 lines (+66205/-527)

110 files modified

.bzrignore (+0/-229)
AUTHORS (+17/-0)
ChangeLog (+5918/-0)
Makefile.in (+821/-0)
aclocal.m4 (+10063/-0)
autogen.sh (+0/-12)
compile (+143/-0)
config.guess (+1502/-0)
config.h.in (+86/-0)
config.sub (+1714/-0)
configure (+16483/-0)
debian/changelog (+657/-0)
debian/control (+225/-0)
debian/copyright (+40/-0)
debian/gir1.2-dbusmenu-glib-0.4.install (+1/-0)
debian/gir1.2-dbusmenu-gtk-0.4.install (+1/-0)
debian/gir1.2-dbusmenu-gtk3-0.4.install (+1/-0)
debian/libdbusmenu-glib-dev.install (+6/-0)
debian/libdbusmenu-glib-doc.install (+1/-0)
debian/libdbusmenu-glib3.install (+1/-0)
debian/libdbusmenu-gtk-dev.install (+6/-0)
debian/libdbusmenu-gtk-doc.install (+1/-0)
debian/libdbusmenu-gtk3-3.install (+1/-0)
debian/libdbusmenu-gtk3-dev.install (+5/-0)
debian/libdbusmenu-gtk3.install (+1/-0)
debian/libdbusmenu-jsonloader-dev.install (+4/-0)
debian/libdbusmenu-jsonloader3.install (+1/-0)
debian/libdbusmenu-tools.install (+3/-0)
debian/rules (+45/-0)
debian/source/format (+1/-0)
debian/watch (+2/-0)
depcomp (+630/-0)
docs/Makefile.in (+613/-0)
docs/libdbusmenu-glib/Makefile.in (+613/-0)
docs/libdbusmenu-glib/reference/Makefile.in (+689/-0)
docs/libdbusmenu-glib/reference/html/annotation-glossary.html (+71/-0)
docs/libdbusmenu-glib/reference/html/api-index-full.html (+406/-0)
docs/libdbusmenu-glib/reference/html/ch01.html (+48/-0)
docs/libdbusmenu-glib/reference/html/index.html (+46/-0)
docs/libdbusmenu-glib/reference/html/index.sgml (+120/-0)
docs/libdbusmenu-glib/reference/html/libdbusmenu-glib-DbusmenuClient.html (+427/-0)
docs/libdbusmenu-glib/reference/html/libdbusmenu-glib-DbusmenuClientMenuitem.html (+92/-0)
docs/libdbusmenu-glib/reference/html/libdbusmenu-glib-DbusmenuMenuitem.html (+1326/-0)
docs/libdbusmenu-glib/reference/html/libdbusmenu-glib-DbusmenuMenuitemProxy.html (+154/-0)
docs/libdbusmenu-glib/reference/html/libdbusmenu-glib-DbusmenuServer.html (+260/-0)
docs/libdbusmenu-glib/reference/html/libdbusmenu-glib.devhelp (+109/-0)
docs/libdbusmenu-glib/reference/html/libdbusmenu-glib.devhelp2 (+109/-0)
docs/libdbusmenu-glib/reference/html/object-tree.html (+32/-0)
docs/libdbusmenu-glib/reference/html/style.css (+266/-0)
docs/libdbusmenu-glib/reference/tmpl/client-menuitem.sgml (+52/-0)
docs/libdbusmenu-glib/reference/tmpl/client.sgml (+168/-0)
docs/libdbusmenu-glib/reference/tmpl/menuitem-proxy.sgml (+57/-0)
docs/libdbusmenu-glib/reference/tmpl/menuitem.sgml (+520/-0)
docs/libdbusmenu-glib/reference/tmpl/server.sgml (+112/-0)
docs/libdbusmenu-glib/reference/version.xml (+1/-0)
docs/libdbusmenu-glib/reference/xml/annotation-glossary.xml (+64/-0)
docs/libdbusmenu-glib/reference/xml/api-index-deprecated.xml (+8/-0)
docs/libdbusmenu-glib/reference/xml/api-index-full.xml (+105/-0)
docs/libdbusmenu-glib/reference/xml/client-menuitem.xml (+70/-0)
docs/libdbusmenu-glib/reference/xml/client.xml (+322/-0)
docs/libdbusmenu-glib/reference/xml/menuitem-proxy.xml (+103/-0)
docs/libdbusmenu-glib/reference/xml/menuitem.xml (+915/-0)
docs/libdbusmenu-glib/reference/xml/server.xml (+199/-0)
docs/libdbusmenu-gtk/Makefile.in (+613/-0)
docs/libdbusmenu-gtk/reference/Makefile.in (+685/-0)
docs/libdbusmenu-gtk/reference/html/api-index-full.html (+122/-0)
docs/libdbusmenu-gtk/reference/html/ch01.html (+42/-0)
docs/libdbusmenu-gtk/reference/html/index.html (+39/-0)
docs/libdbusmenu-gtk/reference/html/index.sgml (+33/-0)
docs/libdbusmenu-gtk/reference/html/libdbusmenu-gtk-DbusmenuGtkClient.html (+316/-0)
docs/libdbusmenu-gtk/reference/html/libdbusmenu-gtk-DbusmenuGtkMenu.html (+169/-0)
docs/libdbusmenu-gtk/reference/html/libdbusmenu-gtk-menuitem.html (+256/-0)
docs/libdbusmenu-gtk/reference/html/libdbusmenu-gtk.devhelp (+36/-0)
docs/libdbusmenu-gtk/reference/html/libdbusmenu-gtk.devhelp2 (+36/-0)
docs/libdbusmenu-gtk/reference/html/object-tree.html (+42/-0)
docs/libdbusmenu-gtk/reference/html/style.css (+266/-0)
docs/libdbusmenu-gtk/reference/libdbusmenu-gtk-sections.txt (+70/-0)
docs/libdbusmenu-gtk/reference/libdbusmenu-gtk.types (+3/-0)
docs/libdbusmenu-gtk/reference/tmpl/client.sgml (+115/-0)
docs/libdbusmenu-gtk/reference/tmpl/genericmenuitem.sgml (+22/-0)
docs/libdbusmenu-gtk/reference/tmpl/libdbusmenu-gtk-unused.sgml (+118/-0)
docs/libdbusmenu-gtk/reference/tmpl/menu.sgml (+67/-0)
docs/libdbusmenu-gtk/reference/tmpl/menuitem.sgml (+84/-0)
docs/libdbusmenu-gtk/reference/version.xml (+1/-0)
docs/libdbusmenu-gtk/reference/xml/api-index-deprecated.xml (+8/-0)
docs/libdbusmenu-gtk/reference/xml/api-index-full.xml (+33/-0)
docs/libdbusmenu-gtk/reference/xml/client.xml (+214/-0)
docs/libdbusmenu-gtk/reference/xml/genericmenuitem.xml (+36/-0)
docs/libdbusmenu-gtk/reference/xml/menu.xml (+118/-0)
docs/libdbusmenu-gtk/reference/xml/menuitem.xml (+165/-0)
install-sh (+520/-0)
libdbusmenu-glib/Doxyfile (+0/-257)
libdbusmenu-glib/Makefile.in (+958/-0)
libdbusmenu-glib/client-marshal.c (+131/-0)
libdbusmenu-glib/client-marshal.h (+28/-0)
libdbusmenu-glib/dbus-menu-clean.xml.c (+132/-0)
libdbusmenu-glib/dbus-menu-clean.xml.h (+1/-0)
libdbusmenu-glib/menuitem-marshal.c (+170/-0)
libdbusmenu-glib/menuitem-marshal.h (+45/-0)
libdbusmenu-glib/server-marshal.c (+164/-0)
libdbusmenu-glib/server-marshal.h (+36/-0)
libdbusmenu-gtk/Makefile.in (+1005/-0)
ltmain.sh (+8413/-0)
missing (+376/-0)
mkinstalldirs (+162/-0)
po/Makefile.in.in (+217/-0)
tests/Makefile.in (+2204/-0)
tools/Makefile.in (+858/-0)
tools/testapp/CMakeLists.txt (+0/-29)
tools/testapp/Makefile.in (+619/-0)

lp:~ubuntu-desktop/libdbusmenu/ubuntu (Merged)

lp:ubuntu/oneiric/libdbusmenu

Revision history for this message

JKL (jkl102001) wrote on 2011-05-22:

In addition, there are 3 GVariant reference leaks at the beginning of bus_get_layout.

JKL (jkl102001) on 2011-05-22

summary:

- memory leak in bus_get_layout
+ multiple reference leaks in server.c

Revision history for this message

JKL (jkl102001) wrote on 2011-05-22:

I marked two other bugs as duplicates. They aren't strictly speaking duplicates, but since everything is in the same file it will be difficult to make separate patches.

Revision history for this message

JKL (jkl102001) wrote on 2011-05-22:

There are so many reference leaks in server.c it is difficult to list them all individually. This patch generally tries to use g_variant_get to unpack parameters, which avoids many problems with reference counting. It makes the earlier one obsolete.

Revision history for this message

JKL (jkl102001) wrote on 2011-05-22:

That patch is broken. For one thing, it looks like the calling convention for the server methods doesn't match what it says in the protocol XML, or maybe I have just misunderstood something, so the patch gets the parameter types wrong. There are probably other problems as well.

I believe I have reached the limit of what I can accomplish here. Good luck.

Brian Murray (brian-murray) on 2011-05-28

tags:

added: patch

Revision history for this message

JKL (jkl102001) wrote on 2011-05-28:

probably should be split up Edit (9.3 KiB, text/plain)

Before throwing in the towel here, I'll post the current patch I'm using on my own system. It fixes this bug, the two that are marked as duplicates (but which are actually separate bugs in the same file), and also #785828. It also fixes several other problems that don't have separate bugs filed. These unreported problems are:

* crash in menuitem.c if variant parameter is NULL

* reference leaks caused by g_variant_get_child_value

* reference leaks from g_variant_type_new

* typo bug in calling g_variant_parse without error parameter

* variant wrapped inside another variant wrapper before being passed to event callback, when it should be just passed as-is

Ted Gould (ted) on 2011-06-20

Changed in dbusmenu:
status:	Fix Committed → Fix Released

Revision history for this message

Launchpad Janitor (janitor) wrote on 2011-06-24:

This bug was fixed in the package libdbusmenu - 0.4.90-0ubuntu1

---------------
libdbusmenu (0.4.90-0ubuntu1) oneiric; urgency=low

  [ Ted Gould ]
  * debian/control, debian/*: Renaming packages for library version
    bump from 3 to 4.
  * New upstream release.
    * Fixing visibility for Eclipse (LP: #770263 and LP: #618587)
    * Unseting a GValue properly (LP: #785828)
    * Memory leaks for GVariant usage (LP: #784890)
    * Making GTK 3 default build
    * Removing the SerializableMenuitem object
  * debian/rules: Making GTK2 the special build

  [ Ken VanDine ]
  * +debian/libdbusmenu-jsonloader4.symbols
  * debian/*.symbols
    - Fixed sonames and removed all the duplicate symbols
  * debian/*.install, debian/rules
    - Use cdbs to do the dual builds for gtk2/gtk3
  * debian/control
    - Updated standards version to 3.9.2
    - Set version on the json-glib build depends to >= 0.13.4
    - Make gir1.2-dbusmenu-glib-0.4 break gir1.2-unity-3.0 and
      gir1.2-indicate-0.5 built against older versions of dbusmenu to
      prevent breakage in python apps that use gir loading multiple versions
      of dbusmenu-glib
-- Ken VanDine <email address hidden> Fri, 24 Jun 2011 14:55:59 -0400