[PATCH memleak service 0/3] Memory leak fixes in service
Hi again, while doing IPv6 work I found some more memory leak problems. The patch 1 is optional as it just adds some more debug output in ipconfig ref/unref functions but I needed it to find the problem described in second patch. The patch 2 fixes reference counting issue in service.c file. The IPv4 and IPv6 ipconfig structs are created in service.c and the reference count is set to 1 at the beginning. The service.c also calls __connman_ipconfig_enable() which increments ref count to 2. At some point the __connman_ipconfig_disable() is called which then decrements the ref count to 1 and then incorrectly clears ipconfig. The patch unrefs the ipconfig correctly before setting ipconfig to NULL. The patch 3 frees the IPv6 gateway address as it was never done. Regards, Jukka Jukka Rissanen (3): ipconfig: add debugging to ref counting functions memoryleak: ipconfig was not unreferenced properly memoryleak: IPv6 gateway was not freed src/connection.c |1 + src/ipconfig.c | 12 ++-- src/service.c| 34 ++ 3 files changed, 33 insertions(+), 14 deletions(-) ___ connman mailing list connman@connman.net http://lists.connman.net/listinfo/connman
[PATCH memleak service 1/3] ipconfig: add debugging to ref counting functions
---
src/ipconfig.c | 12 ++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/ipconfig.c b/src/ipconfig.c
index 690b51e..b5dc98f 100644
--- a/src/ipconfig.c
+++ b/src/ipconfig.c
@@ -915,6 +915,9 @@ struct connman_ipconfig *connman_ipconfig_create(int index,
*/
struct connman_ipconfig *connman_ipconfig_ref(struct connman_ipconfig
*ipconfig)
{
+ DBG(ipconfig %p refcount %d, ipconfig,
+ g_atomic_int_get(ipconfig-refcount) + 1);
+
g_atomic_int_inc(ipconfig-refcount);
return ipconfig;
@@ -928,8 +931,13 @@ struct connman_ipconfig *connman_ipconfig_ref(struct
connman_ipconfig *ipconfig)
*/
void connman_ipconfig_unref(struct connman_ipconfig *ipconfig)
{
- if (ipconfig
- g_atomic_int_dec_and_test(ipconfig-refcount) == TRUE) {
+ if (ipconfig == NULL)
+ return;
+
+ DBG(ipconfig %p refcount %d, ipconfig,
+ g_atomic_int_get(ipconfig-refcount) - 1);
+
+ if (g_atomic_int_dec_and_test(ipconfig-refcount) == TRUE) {
__connman_ipconfig_disable(ipconfig);
connman_ipconfig_set_ops(ipconfig, NULL);
--
1.7.0.4
___
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
[PATCH memleak service 2/3] memoryleak: ipconfig was not unreferenced properly
The service creates ipconfig and then enables it which means that
ref count goes to 2. At some point it then disables ipconfig but
does not do unref which means there is a memory leak as ref count
never goes to 0.
---
src/service.c | 40
1 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/src/service.c b/src/service.c
index 09cc4eb..0002609 100644
--- a/src/service.c
+++ b/src/service.c
@@ -2232,13 +2232,15 @@ static gboolean connect_timeout(gpointer user_data)
if (service-network != NULL)
__connman_network_disconnect(service-network);
- if (service-ipconfig_ipv4)
- if (__connman_ipconfig_disable(service-ipconfig_ipv4) == 0)
- service-ipconfig_ipv4 = NULL;
+ if (__connman_ipconfig_disable(service-ipconfig_ipv4) == 0) {
+ connman_ipconfig_unref(service-ipconfig_ipv4);
+ service-ipconfig_ipv4 = NULL;
+ }
- if (service-ipconfig_ipv6)
- if (__connman_ipconfig_disable(service-ipconfig_ipv6) == 0)
- service-ipconfig_ipv6 = NULL;
+ if (__connman_ipconfig_disable(service-ipconfig_ipv6) == 0) {
+ connman_ipconfig_unref(service-ipconfig_ipv6);
+ service-ipconfig_ipv6 = NULL;
+ }
__connman_stats_service_unregister(service);
@@ -3351,15 +3353,17 @@ int __connman_service_connect(struct connman_service
*service)
if (err 0) {
if (err != -EINPROGRESS) {
- if (service-ipconfig_ipv4)
- if (__connman_ipconfig_disable(
- service-ipconfig_ipv4) == 0)
- service-ipconfig_ipv4 = NULL;
+ if (__connman_ipconfig_disable(
+ service-ipconfig_ipv4) == 0) {
+ connman_ipconfig_unref(service-ipconfig_ipv4);
+ service-ipconfig_ipv4 = NULL;
+ }
- if (service-ipconfig_ipv6)
- if (__connman_ipconfig_disable(
- service-ipconfig_ipv6) == 0)
- service-ipconfig_ipv6 = NULL;
+ if (__connman_ipconfig_disable(
+ service-ipconfig_ipv6) == 0) {
+ connman_ipconfig_unref(service-ipconfig_ipv6);
+ service-ipconfig_ipv6 = NULL;
+ }
__connman_stats_service_unregister(service);
if (service-userconnect == TRUE)
@@ -3406,11 +3410,15 @@ int __connman_service_disconnect(struct connman_service
*service)
__connman_ipconfig_clear_address(service-ipconfig_ipv4);
__connman_ipconfig_clear_address(service-ipconfig_ipv6);
- if (__connman_ipconfig_disable(service-ipconfig_ipv4) == 0)
+ if (__connman_ipconfig_disable(service-ipconfig_ipv4) == 0) {
+ connman_ipconfig_unref(service-ipconfig_ipv4);
service-ipconfig_ipv4 = NULL;
+ }
- if (__connman_ipconfig_disable(service-ipconfig_ipv6) == 0)
+ if (__connman_ipconfig_disable(service-ipconfig_ipv6) == 0) {
+ connman_ipconfig_unref(service-ipconfig_ipv6);
service-ipconfig_ipv6 = NULL;
+ }
__connman_stats_service_unregister(service);
--
1.7.0.4
___
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
[PATCH memleak service 3/3] memoryleak: IPv6 gateway was not freed
--- src/connection.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/connection.c b/src/connection.c index bfd19b5..789a242 100644 --- a/src/connection.c +++ b/src/connection.c @@ -221,6 +221,7 @@ static int remove_gateway(struct gateway_data *data) err = 0; g_free(data-ipv4_gateway); + g_free(data-ipv6_gateway); g_free(data-vpn_ip); g_free(data); -- 1.7.0.4 ___ connman mailing list connman@connman.net http://lists.connman.net/listinfo/connman
Re: [PATCH memleak service 0/3] Memory leak fixes in service
Hi Jukka, while doing IPv6 work I found some more memory leak problems. The patch 1 is optional as it just adds some more debug output in ipconfig ref/unref functions but I needed it to find the problem described in second patch. The patch 2 fixes reference counting issue in service.c file. The IPv4 and IPv6 ipconfig structs are created in service.c and the reference count is set to 1 at the beginning. The service.c also calls __connman_ipconfig_enable() which increments ref count to 2. At some point the __connman_ipconfig_disable() is called which then decrements the ref count to 1 and then incorrectly clears ipconfig. The patch unrefs the ipconfig correctly before setting ipconfig to NULL. The patch 3 frees the IPv6 gateway address as it was never done. all three patches have been applied. Thanks. Regards Marcel ___ connman mailing list connman@connman.net http://lists.connman.net/listinfo/connman
Re: [PATCH v5] Add inotify monitoring .config file.
Hi Mohamed,
Reflect new and modify *.config to connman config list. with
patch any modified or added .config file will be read by connman
and add these configuration for new provisioning.
---
src/config.c | 171
+++---
1 files changed, 163 insertions(+), 8 deletions(-)
this looks good now. Patch has been applied. Thanks.
- if (connman_dbus_validate_ident(ident) == TRUE)
- create_config(ident);
+ if (connman_dbus_validate_ident(ident) == TRUE) {
+ struct connman_config *config;
+ config = create_config(ident);
+ if (config != 0)
+ load_config(config);
I did have a minor issue here. You need to compare to != NULL in this
case and not to 0. While the compiler does the right thing, it should
have warned you at least.
Anyhow this was so simple, that I just fixed it up before applying the
patch :)
Regards
Marcel
___
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
[PATCH resend] Fix Valgrind invalid write error for WiFi plugin
From: Leena Gunda leena.gu...@wipro.com
Fixes BMC#11684
---
g_supplicant_unregister first destroys the interface table and then invokes
system_killed callback which will trigger wifi device driver removal.
wifi_remove will now set it's interface data to NULL but the
GSupplicantInterface has already been freed and hence the issue.
Invoking the system_killed callback before destroying the interface table in
gsupplicant will fix this issue.
gsupplicant/supplicant.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 8452656..6302af0 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -2633,6 +2633,9 @@ void g_supplicant_unregister(const GSupplicantCallbacks
*callbacks)
bss_mapping = NULL;
}
+ if (system_available == TRUE)
+ callback_system_killed();
+
if (interface_table != NULL) {
g_hash_table_foreach(interface_table,
unregister_remove_interface, NULL);
@@ -2640,9 +2643,6 @@ void g_supplicant_unregister(const GSupplicantCallbacks
*callbacks)
interface_table = NULL;
}
- if (system_available == TRUE)
- callback_system_killed();
-
if (connection != NULL) {
dbus_connection_unref(connection);
connection = NULL;
--
1.7.2.2
___
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
Re: [PATCH resend] Fix memory leaks in gsupplicant interface_property
Hi Leena,
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 6302af0..1459aea 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -1272,20 +1272,32 @@ static void interface_property(const char *key,
DBusMessageIter *iter,
const char *str = NULL;
dbus_message_iter_get_basic(iter, str);
- if (str != NULL)
+ if (str != NULL) {
+ if (interface-ifname != NULL)
+ g_free(interface-ifname);
g_free always does a NULL check.
+
interface-ifname = g_strdup(str);
+ }
So something like this would be just fine:
if (str != NULL) {
g_free(interface-ifname);
interface-ifname = g_strdup(str);
}
} else if (g_strcmp0(key, Driver) == 0) {
const char *str = NULL;
dbus_message_iter_get_basic(iter, str);
- if (str != NULL)
+ if (str != NULL) {
+ if (interface-driver != NULL)
+ g_free(interface-driver);
+
interface-driver = g_strdup(str);
+ }
Same here.
} else if (g_strcmp0(key, BridgeIfname) == 0) {
const char *str = NULL;
dbus_message_iter_get_basic(iter, str);
- if (str != NULL)
+ if (str != NULL) {
+ if (interface-bridge != NULL)
+ g_free(interface-bridge);
+
interface-bridge = g_strdup(str);
+ }
And here.
} else if (g_strcmp0(key, CurrentBSS) == 0) {
interface_bss_added(iter, interface);
} else if (g_strcmp0(key, CurrentNetwork) == 0) {
Regards
Marcel
___
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
Re: [PATCH resend] Fix Valgrind invalid write error for WiFi plugin
Hi Leena, From: Leena Gunda leena.gu...@wipro.com Fixes BMC#11684 --- g_supplicant_unregister first destroys the interface table and then invokes system_killed callback which will trigger wifi device driver removal. wifi_remove will now set it's interface data to NULL but the GSupplicantInterface has already been freed and hence the issue. Invoking the system_killed callback before destroying the interface table in gsupplicant will fix this issue. this is valuable details that should be placed before --- and thus become part of the commit message. What I meant earlier is that just repeating the subject line is pointless. gsupplicant/supplicant.c |6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) Patch has been applied. Thanks. Regards Marcel ___ connman mailing list connman@connman.net http://lists.connman.net/listinfo/connman
[PATCH resend v1] Fix memory leaks in gsupplicant interface_property
From: Leena Gunda leena.gu...@wipro.com
Free the interface properties before doing a g_strdup.
Fixes BMC#11687
---
gsupplicant/supplicant.c | 12 +---
1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 8452656..eae2bef 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -1272,20 +1272,26 @@ static void interface_property(const char *key,
DBusMessageIter *iter,
const char *str = NULL;
dbus_message_iter_get_basic(iter, str);
- if (str != NULL)
+ if (str != NULL) {
+ g_free(interface-ifname);
interface-ifname = g_strdup(str);
+ }
} else if (g_strcmp0(key, Driver) == 0) {
const char *str = NULL;
dbus_message_iter_get_basic(iter, str);
- if (str != NULL)
+ if (str != NULL) {
+ g_free(interface-driver);
interface-driver = g_strdup(str);
+ }
} else if (g_strcmp0(key, BridgeIfname) == 0) {
const char *str = NULL;
dbus_message_iter_get_basic(iter, str);
- if (str != NULL)
+ if (str != NULL) {
+ g_free(interface-bridge);
interface-bridge = g_strdup(str);
+ }
} else if (g_strcmp0(key, CurrentBSS) == 0) {
interface_bss_added(iter, interface);
} else if (g_strcmp0(key, CurrentNetwork) == 0) {
--
1.7.2.2
___
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
Re: [PATCH resend v1] Fix memory leaks in gsupplicant interface_property
Hi Leena, Free the interface properties before doing a g_strdup. Fixes BMC#11687 --- gsupplicant/supplicant.c | 12 +--- 1 files changed, 9 insertions(+), 3 deletions(-) patch has been applied. Thanks. Regards Marcel ___ connman mailing list connman@connman.net http://lists.connman.net/listinfo/connman
Re: [PATCH resend] Fix missing and empty WiFi APs issue after kill/restart
Hi Leena, From: Leena Gunda leena.gu...@wipro.com Fixes BMC#10454 and #11201 --- When ConnMan is SIGKILLed and restarted WiFi plugin will reuse the existing interface. It will get a list of existing BSSs and for each BSS in the BSSs list, interface_bss_added() is called. In interface_bss_added() if there is a next DBusMessageIter iterator and if it is valid, it is assumed that the next iterator would contain the key/value pairs for the BSS properties and bss_property is invoked. But for the BSSs list the next iterator contains the object path of next BSS in the list. As a result we get no properties for the BSS and a GSupplicantNetwork is created with empty ssid etc. Also for the BSSs list, supplicant_dbus_array_foreach() is invoked with interface_bss_added() as callback. In interface_bss_added() the iter is moved to the next iterator. Now supplicant_dbus_array_foreach() again moves the iterator to next after the callback is invoked. As a result we end up moving the iterator twice and miss adding few BSS from the BSSs list. And hence the bug. interface_bss_added() is called with BSS followed by its properties only from signal_bss_added(). Whereas for CurrentBSS and BSSs list GetAll D-Bus method is invoked for fetching the BSS's properties. Separating the cases of BSS's being added with and without properties will fix the issue. gsupplicant/supplicant.c | 56 +++-- 1 files changed, 43 insertions(+), 13 deletions(-) please redo this description and put it before ---. Also break it nicely at 72 chars to that it becomes readable in git log on an 80 chars terminal. Regards Marcel ___ connman mailing list connman@connman.net http://lists.connman.net/listinfo/connman
