Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. [PATCH v2] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called (Naveen Singh)
2. Re: [RFC v2] connection: Move online service to ready if
gateway is removed (Naveen Singh)
3. Re: [PATCH v2] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called (Tomasz Bursztyka)
----------------------------------------------------------------------
Message: 1
Date: Tue, 16 Feb 2016 23:24:11 -0800
From: Naveen Singh <[email protected]>
To: [email protected]
Subject: [PATCH v2] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called
Message-ID:
<[email protected]>
From: nasingh <[email protected]>
Connman did not call netwok_remove in case AP deauthenticated client causing
wpa_s to re-allocate the ssid pointer even if the next connection attempt
is for the same SSID. This change ensures that at the time of connection
(DBUS Method call AddNetwork) if the network is found not removed, it calls
the dbus API to remove the network and once network is removed, proceed with
the connection.
Tested by running a deauth loop script at the AP end and ensure that wpa_s does
not allocate memory for struct wpa_ssid for all the subsequent
connection attempts. During the test memory usage of wpa_s is monitored.
---
gsupplicant/supplicant.c | 158 +++++++++++++++++++++++++++++++++--------------
1 file changed, 110 insertions(+), 48 deletions(-)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 342cb01..d13467e 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -249,6 +249,50 @@ struct _GSupplicantGroup {
GSList *members;
};
+struct interface_data {
+ GSupplicantInterface *interface;
+ char *path; /* Interface path cannot be taken from interface (above) as
+ * it might have been freed already.
+ */
+ GSupplicantInterfaceCallback callback;
+ void *user_data;
+ bool network_remove_in_progress;
+ GSupplicantSSID *ssid;
+};
+
+struct interface_create_data {
+ char *ifname;
+ char *driver;
+ char *bridge;
+ GSupplicantInterface *interface;
+ GSupplicantInterfaceCallback callback;
+ void *user_data;
+};
+
+struct interface_connect_data {
+ GSupplicantInterface *interface;
+ char *path;
+ GSupplicantInterfaceCallback callback;
+ void *user_data;
+ union {
+ GSupplicantSSID *ssid;
+ GSupplicantPeerParams *peer;
+ };
+};
+
+struct interface_scan_data {
+ GSupplicantInterface *interface;
+ char *path;
+ GSupplicantInterfaceCallback callback;
+ GSupplicantScanParams *scan_params;
+ void *user_data;
+};
+
+static int network_remove(struct interface_data *data);
+static void network_remove_params(DBusMessageIter *iter, void *user_data);
+static void network_remove_result(const char *error,
+ DBusMessageIter *iter, void *user_data);
+
static inline void debug(const char *format, ...)
{
char str[256];
@@ -3476,43 +3520,6 @@ GSupplicantPeer
*g_supplicant_interface_peer_lookup(GSupplicantInterface *interf
return peer;
}
-struct interface_data {
- GSupplicantInterface *interface;
- char *path; /* Interface path cannot be taken from interface (above) as
- * it might have been freed already.
- */
- GSupplicantInterfaceCallback callback;
- void *user_data;
-};
-
-struct interface_create_data {
- char *ifname;
- char *driver;
- char *bridge;
- GSupplicantInterface *interface;
- GSupplicantInterfaceCallback callback;
- void *user_data;
-};
-
-struct interface_connect_data {
- GSupplicantInterface *interface;
- char *path;
- GSupplicantInterfaceCallback callback;
- union {
- GSupplicantSSID *ssid;
- GSupplicantPeerParams *peer;
- };
- void *user_data;
-};
-
-struct interface_scan_data {
- GSupplicantInterface *interface;
- char *path;
- GSupplicantInterfaceCallback callback;
- GSupplicantScanParams *scan_params;
- void *user_data;
-};
-
static void interface_create_data_free(struct interface_create_data *data)
{
g_free(data->ifname);
@@ -4105,7 +4112,6 @@ static void interface_add_network_result(const char
*error,
SUPPLICANT_DBG("PATH: %s", path);
- g_free(interface->network_path);
interface->network_path = g_strdup(path);
supplicant_dbus_method_call(data->interface->path,
@@ -4656,7 +4662,8 @@ int g_supplicant_interface_connect(GSupplicantInterface
*interface,
void *user_data)
{
struct interface_connect_data *data;
- int ret;
+ struct interface_data *intf_data;
+ int ret = 0;
if (!interface)
return -EINVAL;
@@ -4685,12 +4692,45 @@ int g_supplicant_interface_connect(GSupplicantInterface
*interface,
SUPPLICANT_INTERFACE ".Interface.WPS",
"ProcessCredentials", DBUS_TYPE_BOOLEAN_AS_STRING,
wps_process_credentials, wps_start, data, interface);
- } else
- ret = supplicant_dbus_method_call(interface->path,
- SUPPLICANT_INTERFACE ".Interface", "AddNetwork",
- interface_add_network_params,
- interface_add_network_result, data,
- interface);
+ } else {
+ /* By the time there is a request for connect and the network
+ * path is not NULL it means that connman has not removed the
+ * previous network pointer. This can happen in the case AP
+ * deauthenticated client and connman does not remove the
+ * previously connected network pointer. This causes supplicant
+ * to reallocate the memory for struct wpa_ssid again even if it
+ * is the same SSID. This causes memory usage of wpa_supplicnat
+ * to go high. The idea here is that if the previously connected
+ * network is not removed at the time of next connection attempt
+ * check if the network path is not NULL. In case it is non-NULL
+ * first remove the network and then once removal is
successful, add
+ * the network.
+ */
+
+ if (interface->network_path != NULL) {
+ g_free(data->path);
+ dbus_free(data);
+
+ intf_data = dbus_malloc0(sizeof(*intf_data));
+ if (!intf_data)
+ return -ENOMEM;
+
+ intf_data->interface = interface;
+ intf_data->path = g_strdup(interface->path);
+ intf_data->callback = callback;
+ intf_data->ssid = ssid;
+ intf_data->user_data = user_data;
+ intf_data->network_remove_in_progress = TRUE;
+ network_remove(intf_data);
+ }
+ else {
+ ret = supplicant_dbus_method_call(interface->path,
+ SUPPLICANT_INTERFACE ".Interface",
"AddNetwork",
+ interface_add_network_params,
+ interface_add_network_result, data,
+ interface);
+ }
+ }
if (ret < 0) {
g_free(data->path);
@@ -4705,6 +4745,7 @@ static void network_remove_result(const char *error,
DBusMessageIter *iter, void *user_data)
{
struct interface_data *data = user_data;
+ struct interface_connect_data * connect_data;
int result = 0;
SUPPLICANT_DBG("");
@@ -4716,11 +4757,32 @@ static void network_remove_result(const char *error,
result = -ECONNABORTED;
}
- g_free(data->path);
+ g_free(data->interface->network_path);
+ data->interface->network_path = NULL;
- if (data->callback)
- data->callback(result, data->interface, data->user_data);
+ if (data->network_remove_in_progress == TRUE) {
+ data->network_remove_in_progress = FALSE;
+ connect_data = dbus_malloc0(sizeof(*connect_data));
+ if (!connect_data)
+ return;
+
+ connect_data->interface = data->interface;
+ connect_data->path = data->path;
+ connect_data->callback = data->callback;
+ connect_data->ssid = data->ssid;
+ connect_data->user_data = data->user_data;
+ supplicant_dbus_method_call(data->interface->path,
+ SUPPLICANT_INTERFACE ".Interface", "AddNetwork",
+ interface_add_network_params,
+ interface_add_network_result, connect_data,
+ connect_data->interface);
+ }
+ else {
+ g_free(data->path);
+ if (data->callback)
+ data->callback(result, data->interface,
data->user_data);
+ }
dbus_free(data);
}
--
2.7.0.rc3.207.g0ac5344
------------------------------
Message: 2
Date: Tue, 16 Feb 2016 23:27:35 -0800
From: Naveen Singh <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected]
Subject: Re: [RFC v2] connection: Move online service to ready if
gateway is removed
Message-ID:
<CAGTDzK=LOhQVWgAK-sc1dgjYahjhh1242XZbut2wT6=-2-4...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
On Fri, Feb 12, 2016 at 3:16 AM, Patrik Flykt
<[email protected]> wrote:
> When the default gateway is removed, move the service to state ready
> if the respective ipconfig was connected.
>
> Based on a patch by Naveen Singh.
> ---
>
> Coming back to the issue, downgrading the state to ready when removing the
> gateway does seem the most stable thing to do. This location to do the state
> transition was earlier proposed by Naveen Singh. In this variant the ipconfig
> state is set to ready only if it is already connected.
>
> It would be better to listen on the default routes added and removed
> when setting the state, but this has quite a few implications on testing.
> So let's try with this as a much easier working solution.
>
> Please test,
>
> Patrik
>
>
> src/connection.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/connection.c b/src/connection.c
> index 6b005e7..5234043 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -963,6 +963,11 @@ void __connman_connection_gateway_remove(struct
> connman_service *service,
> if (data)
> set_default_gateway(data, type);
> }
> +
> + if (__connman_service_is_connected_state(service, type))
> + __connman_service_ipconfig_indicate_state(service,
> + CONNMAN_SERVICE_STATE_READY,
> + type);
> }
>
> bool __connman_connection_update_gateway(void)
> --
> 2.1.4
>
> _______________________________________________
> connman mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/connman
Hi Patrik
We will get back to you with our test-result
Regards
Naveen
------------------------------
Message: 3
Date: Wed, 17 Feb 2016 09:28:02 +0100
From: Tomasz Bursztyka <[email protected]>
To: [email protected]
Subject: Re: [PATCH v2] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Hi,
>
> if (!interface)
> return -EINVAL;
> @@ -4685,12 +4692,45 @@ int
> g_supplicant_interface_connect(GSupplicantInterface *interface,
> SUPPLICANT_INTERFACE ".Interface.WPS",
> "ProcessCredentials", DBUS_TYPE_BOOLEAN_AS_STRING,
> wps_process_credentials, wps_start, data, interface);
> - } else
> - ret = supplicant_dbus_method_call(interface->path,
> - SUPPLICANT_INTERFACE ".Interface", "AddNetwork",
> - interface_add_network_params,
> - interface_add_network_result, data,
> - interface);
> + } else {
> + /* By the time there is a request for connect and the network
> + * path is not NULL it means that connman has not removed the
> + * previous network pointer. This can happen in the case AP
> + * deauthenticated client and connman does not remove the
> + * previously connected network pointer. This causes supplicant
> + * to reallocate the memory for struct wpa_ssid again even if it
> + * is the same SSID. This causes memory usage of wpa_supplicnat
> + * to go high. The idea here is that if the previously connected
> + * network is not removed at the time of next connection attempt
> + * check if the network path is not NULL. In case it is non-NULL
> + * first remove the network and then once removal is
> successful, add
> + * the network.
> + */
> +
> + if (interface->network_path != NULL) {
> + g_free(data->path);
> + dbus_free(data);
> +
> + intf_data = dbus_malloc0(sizeof(*intf_data));
> + if (!intf_data)
> + return -ENOMEM;
> +
> + intf_data->interface = interface;
> + intf_data->path = g_strdup(interface->path);
> + intf_data->callback = callback;
> + intf_data->ssid = ssid;
> + intf_data->user_data = user_data;
> + intf_data->network_remove_in_progress = TRUE;
> + network_remove(intf_data);
> + }
> + else {
Don't break the line.
> + ret = supplicant_dbus_method_call(interface->path,
> + SUPPLICANT_INTERFACE ".Interface",
> "AddNetwork",
> + interface_add_network_params,
> + interface_add_network_result, data,
> + interface);
> + }
> + }
>
> if (ret < 0) {
> g_free(data->path);
> @@ -4705,6 +4745,7 @@ static void network_remove_result(const char *error,
> DBusMessageIter *iter, void *user_data)
> {
> struct interface_data *data = user_data;
> + struct interface_connect_data * connect_data;
> int result = 0;
>
> SUPPLICANT_DBG("");
> @@ -4716,11 +4757,32 @@ static void network_remove_result(const char *error,
> result = -ECONNABORTED;
> }
>
> - g_free(data->path);
> + g_free(data->interface->network_path);
> + data->interface->network_path = NULL;
>
> - if (data->callback)
> - data->callback(result, data->interface, data->user_data);
> + if (data->network_remove_in_progress == TRUE) {
> + data->network_remove_in_progress = FALSE;
> + connect_data = dbus_malloc0(sizeof(*connect_data));
> + if (!connect_data)
> + return;
> +
> + connect_data->interface = data->interface;
> + connect_data->path = data->path;
> + connect_data->callback = data->callback;
> + connect_data->ssid = data->ssid;
> + connect_data->user_data = data->user_data;
>
> + supplicant_dbus_method_call(data->interface->path,
> + SUPPLICANT_INTERFACE ".Interface", "AddNetwork",
> + interface_add_network_params,
> + interface_add_network_result, connect_data,
> + connect_data->interface);
> + }
> + else {
Here as well
> + g_free(data->path);
> + if (data->callback)
> + data->callback(result, data->interface,
> data->user_data);
> + }
> dbus_free(data);
> }
>
Tomasz
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 4, Issue 20
**************************************
