From 3120d1f8012377820dbc011713377ff9daab5f5c Mon Sep 17 00:00:00 2001 From: Mikael Kanstrup Date: Thu, 26 Nov 2015 09:27:51 +0100 Subject: Fix memory leaks in all_ifaces when interface list changes Valgrind report leaks of several allocations like these: 590 bytes in 50 blocks are possibly lost in loss record 29,818 of 31,670 at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) by 0xCB9C8A7: __vasprintf_chk (vasprintf_chk.c:82) by 0xA3D8DCA: g_vasprintf (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.3200.4) by 0xA3B846C: g_strdup_vprintf (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.3200.4) by 0xA3B850B: g_strdup_printf (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.3200.4) by 0x6F4B51: scan_local_interfaces (iface_lists.c:254) by 0x6EF3D8: iface_mon_handler2 (iface_monitor.c:113) by 0xBE56F1D: ??? (in /lib/libnl-3.so.200.3.0) by 0xBA16F19: ??? (in /usr/lib/libnl-route-3.so.200.3.0) by 0xBE54E5E: nl_cache_parse (in /lib/libnl-3.so.200.3.0) by 0xBE585CA: nl_msg_parse (in /lib/libnl-3.so.200.3.0) by 0x6EF372: iface_mon_handler (iface_monitor.c:123) When the list of network interfaces is updated allocations done for global_capture_opts.all_ifaces elements leak memory. Fixed by introducing a helper function to be used for removing an interface_t element from all_ifaces array. While at it also fixed misc leaks when updating individual allocated records of all_ifaces elements. Change-Id: I035e6936a44edeef2ebe4780931c14cde99e93a4 Reviewed-on: https://code.wireshark.org/review/12209 Petri-Dish: Alexis La Goutte Tested-by: Petri Dish Buildbot Reviewed-by: Peter Wu --- capture_opts.c | 44 +++++++++++++++++++++++++++++++++++++ capture_opts.h | 3 +++ ui/gtk/capture_dlg.c | 8 +++++++ ui/iface_lists.c | 5 ++++- ui/qt/capture_filter_edit.cpp | 1 + ui/qt/capture_interfaces_dialog.cpp | 1 + ui/qt/interface_tree.cpp | 12 ++++------ ui/qt/manage_interfaces_dialog.cpp | 3 +++ 8 files changed, 68 insertions(+), 9 deletions(-) diff --git a/capture_opts.c b/capture_opts.c index 5c6042167e..c2c25d1009 100644 --- a/capture_opts.c +++ b/capture_opts.c @@ -1146,6 +1146,50 @@ collect_ifaces(capture_options *capture_opts) } } +static void +capture_opts_free_interface_t_links(gpointer elem, gpointer unused _U_) +{ + link_row* e = (link_row*)elem; + if (e != NULL) + g_free(e->name); + g_free(elem); +} + +static void +capture_opts_free_interface_t_addrs(gpointer elem, gpointer unused _U_) +{ + g_free(elem); +} + +void +capture_opts_free_interface_t(interface_t *device) +{ + if (device != NULL) { + g_free(device->name); + g_free(device->display_name); + g_free(device->friendly_name); + g_free(device->addresses); + g_free(device->cfilter); + g_list_foreach(device->links, + capture_opts_free_interface_t_links, NULL); + g_list_free(device->links); +#ifdef HAVE_PCAP_REMOTE + g_free(device->remote_opts.remote_host_opts.remote_host); + g_free(device->remote_opts.remote_host_opts.remote_port); + g_free(device->remote_opts.remote_host_opts.auth_username); + g_free(device->remote_opts.remote_host_opts.auth_password); +#endif + g_free(device->if_info.name); + g_free(device->if_info.friendly_name); + g_free(device->if_info.vendor_description); + g_slist_foreach(device->if_info.addrs, + capture_opts_free_interface_t_addrs, NULL); + g_slist_free(device->if_info.addrs); +#ifdef HAVE_EXTCAP + g_free(device->if_info.extcap); +#endif + } +} #endif /* HAVE_LIBPCAP */ diff --git a/capture_opts.h b/capture_opts.h index 58efc54ef0..520a0e1e45 100644 --- a/capture_opts.h +++ b/capture_opts.h @@ -371,6 +371,9 @@ capture_opts_del_iface(capture_options *capture_opts, guint if_index); extern void collect_ifaces(capture_options *capture_opts); +extern void +capture_opts_free_interface_t(interface_t *device); + /* Default capture buffer size in Mbytes. */ #define DEFAULT_CAPTURE_BUFFER_SIZE 2 diff --git a/ui/gtk/capture_dlg.c b/ui/gtk/capture_dlg.c index a3f3390ce0..98342ad313 100644 --- a/ui/gtk/capture_dlg.c +++ b/ui/gtk/capture_dlg.c @@ -766,6 +766,7 @@ capture_all_filter_check_syntax_cb(GtkWidget *w _U_, gpointer user_data _U_) colorize_filter_te_as_empty(filter_te); if (strlen(device.cfilter) > 0) { g_array_remove_index(global_capture_opts.all_ifaces, i); + g_free(device.cfilter); device.cfilter = g_strdup(filter_text); g_array_insert_val(global_capture_opts.all_ifaces, i, device); update_filter_string(device.name, filter_text); @@ -775,6 +776,7 @@ capture_all_filter_check_syntax_cb(GtkWidget *w _U_, gpointer user_data _U_) } g_assert(filter_text != NULL); g_array_remove_index(global_capture_opts.all_ifaces, i); + g_free(device.cfilter); device.cfilter = g_strdup(filter_text); g_array_insert_val(global_capture_opts.all_ifaces, i, device); g_mutex_lock(cfc_data_mtx); @@ -1245,6 +1247,7 @@ insert_new_rows(GList *list) ip_str = g_string_new(""); str = ""; ips = 0; + memset(&device, 0, sizeof(device)); device.name = g_strdup(if_info->name); /* Is this interface hidden and, if so, should we include it anyway? */ @@ -3208,6 +3211,7 @@ static void capture_all_cb(GtkToggleButton *button, gpointer d _U_) device = g_array_index(global_capture_opts.all_ifaces, interface_t, i); if (strcmp(device.name, interface) == 0) { g_array_remove_index(global_capture_opts.all_ifaces, i); + g_free(device.cfilter); device.cfilter = g_strdup(filter_text); g_array_insert_val(global_capture_opts.all_ifaces, i, device); update_filter_string(device.name, filter_text); @@ -3364,6 +3368,8 @@ static void change_pipe_name_cb(gpointer dialog _U_, gint btn, gpointer data) for (i = 0; i < global_capture_opts.all_ifaces->len; i++) { device = g_array_index(global_capture_opts.all_ifaces, interface_t, i); if (strcmp(pipe_name, device.name) == 0) { + g_free(device.name); + g_free(device.display_name); device.name = g_strdup((gchar *)data); device.display_name = g_strdup_printf("%s", device.name); g_array_remove_index(global_capture_opts.all_ifaces, i); @@ -3473,6 +3479,7 @@ add_pipe_cb(gpointer w _U_) } } pipe_name = g_strdup(g_save_file); + memset(&device, 0, sizeof(device)); device.name = g_strdup(g_save_file); device.display_name = g_strdup_printf("%s", device.name); device.hidden = FALSE; @@ -4015,6 +4022,7 @@ remove_remote_host(GtkWidget *w _U_, gpointer data _U_) } else { if (strcmp(host, device.remote_opts.remote_host_opts.remote_host) == 0) { g_array_remove_index(global_capture_opts.all_ifaces, i); + capture_opts_free_interface_t(&device); } } } diff --git a/ui/iface_lists.c b/ui/iface_lists.c index be9c8bfaf1..4ecf4730e4 100644 --- a/ui/iface_lists.c +++ b/ui/iface_lists.c @@ -73,6 +73,7 @@ fill_from_ifaces (interface_t *device) device->pmode = interface_opts.promisc_mode; device->has_snaplen = interface_opts.has_snaplen; device->snaplen = interface_opts.snaplen; + g_free(device->cfilter); device->cfilter = g_strdup(interface_opts.cfilter); if (interface_opts.linktype != -1) { device->active_dlt = interface_opts.linktype; @@ -131,10 +132,10 @@ scan_local_interfaces(void (*update_cb)(void)) capture_opts_del_iface(&global_capture_opts, j); } } + capture_opts_free_interface_t(&device); } } } - /* Scan through the list and build a list of strings to display. */ g_free(global_capture_opts.ifaces_err_info); if_list = capture_interface_list(&global_capture_opts.ifaces_err, @@ -142,6 +143,7 @@ scan_local_interfaces(void (*update_cb)(void)) update_cb); count = 0; for (if_entry = if_list; if_entry != NULL; if_entry = g_list_next(if_entry)) { + memset(&device, 0, sizeof(device)); if_info = (if_info_t *)if_entry->data; ip_str = g_string_new(""); ips = 0; @@ -316,6 +318,7 @@ scan_local_interfaces(void (*update_cb)(void)) } } if (!found) { /* new interface, maybe a pipe */ + memset(&device, 0, sizeof(device)); device.name = g_strdup(interface_opts.name); device.display_name = interface_opts.descr ? g_strdup_printf("%s: %s", device.name, interface_opts.descr) : diff --git a/ui/qt/capture_filter_edit.cpp b/ui/qt/capture_filter_edit.cpp index 02ced86f50..99b8fa8e48 100644 --- a/ui/qt/capture_filter_edit.cpp +++ b/ui/qt/capture_filter_edit.cpp @@ -344,6 +344,7 @@ void CaptureFilterEdit::setFilterSyntaxState(QString filter, bool valid, QString // continue; /* Programming error: somehow managed to select an "unsupported" entry */ // } g_array_remove_index(global_capture_opts.all_ifaces, i); + g_free(device.cfilter); device.cfilter = qstring_strdup(filter); g_array_insert_val(global_capture_opts.all_ifaces, i, device); // update_filter_string(device.name, filter_text); diff --git a/ui/qt/capture_interfaces_dialog.cpp b/ui/qt/capture_interfaces_dialog.cpp index 7f139890e6..949b2128d8 100644 --- a/ui/qt/capture_interfaces_dialog.cpp +++ b/ui/qt/capture_interfaces_dialog.cpp @@ -538,6 +538,7 @@ void CaptureInterfacesDialog::updateInterfaces() #endif gchar* prefFilter = capture_dev_user_cfilter_find(device->name); if (prefFilter) { + g_free(device->cfilter); device->cfilter = prefFilter; } ti->setText(col_filter_, device->cfilter); diff --git a/ui/qt/interface_tree.cpp b/ui/qt/interface_tree.cpp index feff96e284..9d6554a276 100644 --- a/ui/qt/interface_tree.cpp +++ b/ui/qt/interface_tree.cpp @@ -361,21 +361,17 @@ void InterfaceTree::updateSelectedInterfaces() void InterfaceTree::setSelectedInterfaces() { #ifdef HAVE_LIBPCAP - interface_t device; + interface_t *device; QTreeWidgetItemIterator iter(this); while (*iter) { QString device_name = (*iter)->data(IFTREE_COL_NAME, Qt::UserRole).value(); for (guint i = 0; i < global_capture_opts.all_ifaces->len; i++) { - device = g_array_index(global_capture_opts.all_ifaces, interface_t, i); - if (device_name.compare(QString().fromUtf8(device.name)) == 0) { - (*iter)->setSelected(device.selected); - global_capture_opts.all_ifaces = g_array_remove_index(global_capture_opts.all_ifaces, i); - g_array_insert_val(global_capture_opts.all_ifaces, i, device); + device = &g_array_index(global_capture_opts.all_ifaces, interface_t, i); + if (device_name.compare(QString().fromUtf8(device->name)) == 0) { + (*iter)->setSelected(device->selected); break; } - global_capture_opts.all_ifaces = g_array_remove_index(global_capture_opts.all_ifaces, i); - g_array_insert_val(global_capture_opts.all_ifaces, i, device); } iter++; } diff --git a/ui/qt/manage_interfaces_dialog.cpp b/ui/qt/manage_interfaces_dialog.cpp index 89feac1621..ef216b1f80 100644 --- a/ui/qt/manage_interfaces_dialog.cpp +++ b/ui/qt/manage_interfaces_dialog.cpp @@ -238,6 +238,7 @@ void ManageInterfacesDialog::pipeAccepted() continue; } global_capture_opts.all_ifaces = g_array_remove_index(global_capture_opts.all_ifaces, i); + capture_opts_free_interface_t(&device); } // Next rebuild a fresh list @@ -259,6 +260,7 @@ void ManageInterfacesDialog::pipeAccepted() } } + memset(&device, 0, sizeof(device)); device.name = qstring_strdup(pipe_name); device.display_name = g_strdup(device.name); device.hidden = FALSE; @@ -506,6 +508,7 @@ void ManageInterfacesDialog::addRemoteInterfaces(GList* rlist, remote_options *r ip_str = g_string_new(""); str = ""; ips = 0; + memset(&device, 0, sizeof(device)); device.name = g_strdup(if_info->name); /* Is this interface hidden and, if so, should we include it anyway? */ -- cgit v1.2.3