nm-applet leaks in get_best_icon_name_for_ap() (or its uses)

This bug was fixed in the package network-manager-applet - 0.8.4~git.20110228t141430.abba62f-0ubuntu1

---------------
network-manager-applet (0.8.4~git.20110228t141430.abba62f-0ubuntu1) natty; urgency=low

  * upstream snapshot 2011-02-28 14:14:30 (GMT)
    + abba62fe1c22422bb84f12875fd3612b90ead434
    - editor: fix usage of nm_connection_update_secrets() (LP: #721071)
    - release: bump version to 0.8.3.996 (0.8.4-beta2)
    - mobile: fix path to GtkBuilder file
    - applet: blacklist "Free Public Wifi" (LP: #659461)
    - applet: consolidate manufacturer default SSID lists
    - release: bump version to 0.8.3.995 (0.8.4-beta1)
    - release: update NEWS with changes since 0.8.2
    - editor: remove unused argument from CEPage 'initialized' signal
    - bluetooth: don't create empty plugin dirs when disabled (bgo #641300)
    - Updated Polish translation
  * debian/patches/nm-applet-use-indicator.patch: Further update indicator
    patch to fix memory leaks in get_best_icon_name_for_ap(). (LP: #724554)
  * debian/patches/nm-applet-use-indicator.patch: Set indicator icon for mobile
    according to the connection's current signal strength (LP: #726669)
  * debian/patches/indicator-accessible-desc.patch: provide accessible
    descriptions for the various icons and devices states for NetworkManager
    (LP: #719736)
 -- Mathieu Trudel-Lapierre <email address hidden> Fri, 04 Mar 2011 15:05:58 -0500

This doesn't appear to be completely fixed. The icon name is passed to foo_set_icon, which is apparently supposed to free it. However, the responsibility to free the icon name is not clearly documented in the code, nor does the function actually do it in all cases.

Since there are conditional compilation sections in foo_set_icon, it is somewhat difficult to follow. It appears that the only place the icon name gets freed is on line 2096, and that is inside an if-block inside the else condition of an ifdef block. Not only that, the function has several early returns and the call to g_free won't happen for those either.

Here is an example valgrind log of the current code:

==10301== 16 bytes in 1 blocks are definitely lost in loss record 2,425 of 9,326
==10301== at 0x4C290A4: realloc (vg_replace_malloc.c:525)
==10301== by 0x8F62B20: g_realloc (gmem.c:233)
==10301== by 0x8F7DCD6: g_string_maybe_expand (gstring.c:401)
==10301== by 0x8F7E3B6: g_string_insert_len (gstring.c:741)
==10301== by 0x8F7E5D4: g_string_assign (gstring.c:615)
==10301== by 0x428F39: get_best_icon_name_for_ap (applet-device-wifi.c:369)
==10301== by 0x42909B: wireless_get_icon (applet-device-wifi.c:1489)
==10301== by 0x41D252: applet_update_icon (applet.c:2605)
==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)

Because the structure of foo_set_icon is not suited to guarantee that it frees the icon name, I suggest removing the call to g_free from foo_set_icon, and adding calls to g_free in applet_update_icon instead.

Patch attached. The patch is against r66, after the debian patch series has been applied. Probably it should be merged with the nm-applet-use-indicator patch, but I think a patch against a patch would have been very hard to read.

Sorry, that patch didn't fix the leak properly.

Moving the g_free from foo_set_icon to where foo_set_icon is called in src/applet.c seems to help a fair amount (at least by my quick look at the memory used by nm-applet locally now).

I've committed this fix to the packaging branch for nm-applet. This means I'll push the fix in the next upload of nm-applet to *Oneiric*. I'll also see about backporting this to Natty in the NetworkManager *trunk* PPA if it's useful for testing (or you could try a live CD of Oneiric in a few days, but expect other issues).

For now, marking Fix Committed; which marks it as available in the bzr packaging branches.

I can't find the branch you are talking about.

Is this the branch you are talking about?

https://code.launchpad.net/~network-manager/network-manager-applet/ubuntu.head

For some reason, you can't get to that by clicking "code" on the header of this page. It is confusing.

In addition to moving the call to g_free, you need to remove the call to g_strdup at the bottom of foo_set_icon, so that icon_name is just a static string in the case where it is originally NULL. Otherwise you fix one leak and create another. This is the part of the patch I am talking about:

if (icon_name == NULL && layer == ICON_LAYER_LINK) {
  icon_name = g_strdup ("nm-no-connection");
}

This problem is one of the reasons I deleted my patch.

Additionally, it might be a good idea to declare icon_name const in the foo_set_icon prototype to underscore the fact that foo_set_icon is not responsible for freeing it.

Some day, for the sake of consistency, it might be a good idea to make memory management for new_tip work the same way as for icon_name, so that in foo_set_icon() new_tip is const. There are a bunch of allocate-just-to-free cases that can be eliminated as well.

I can confirm this... it raised in 15 minutes from 4 to 6 MB... for whatever reason..

This bug was fixed in the package network-manager-applet - 0.8.9997+git.20110529t170033.9ec4c5d-0ubuntu1

---------------
network-manager-applet (0.8.9997+git.20110529t170033.9ec4c5d-0ubuntu1) oneiric; urgency=low

  * upstream snapshot 2011-05-29 17:00:33 (GMT)
    + 9ec4c5de855de5d9ee6c17752c3b0de6c68e9384
    - applet: fix leak in import/upgrade code (LP: #784756)
  * debian/rules: switch back to git "master" branch.
  * debian/patches/0001-applet-fix-possibly-uninitialized-variable.patch: drop,
    this is fixed upstream.
  * debian/patches/04-autostart.patch: refreshed.
  * debian/patches/20_use_full_vpn_dialog_service_name_path.patch: refreshed.
  * debian/patches/lp328572-dxteam-connect-text.patch: refreshed.
  * debian/patches/lp337960_dxteam_notification_icon_names.diff: refreshed.
  * debian/patches/lp341684_device_sensitive_disconnect_notify.patch: refresh.
  * debian/patches/lp460144_correctly_update_notification.patch: refreshed.
  * debian/patches/lp341684_device_sensitive_disconnect_notify.patch: refresh.
  * debian/patches/lp358526_generic_disconnected_notification_icon.patch:
    refreshed.
  * debian/patches/nm-applet-use-indicator.patch: refreshed and modified to
    build with GTK3 appindicator.
    - properly free icon_name for indicators (LP: #724554), thanks JKL.
    - fix leak in applet_menu_item_add_complex_separator_helper due to new'ing
      a GtkSeparatorMenuItem on top of a GtkImageMenuItem (LP: #784711).
  * debian/control:
    - Bump Build-Depends to libappindicator3-dev.
    - Update Build-Depends for GTK to libgtk-3-dev.
    - Bump network-manager and libnm Depends and Build-Depends to 0.8.998.
  * debian/rules, debian/patches/series: pass --libexecdir to configure, which
    now renders the patch 20_use_full_vpn_dialog_service_name_path.patch
    unnecessary, so we're dropping it.
  * debian/patches/20_use_full_vpn_dialog_service_name_path.patch: dropped,
    see above.
 -- Mathieu Trudel-Lapierre <email address hidden> Mon, 30 May 2011 13:25:18 -0400