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. Re: [PATCH] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called (Naveen Singh)
----------------------------------------------------------------------
Message: 1
Date: Fri, 12 Feb 2016 14:08:21 -0800
From: Naveen Singh <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called
Message-ID:
<CAGTDzKnTNcg7=aN+eCYyHAG76=wq2MGmTKfTuhCGbz=ax4q...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
On Fri, Feb 12, 2016 at 1:11 AM, Patrik Flykt
<[email protected]> wrote:
>
> Hi,
>
> On Sun, 2016-02-07 at 00:06 -0800, Naveen Singh wrote:
>> 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 | 139
>> ++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 89 insertions(+), 50 deletions(-)
>>
>> diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
>> index 342cb01..b954e91 100644
>> --- a/gsupplicant/supplicant.c
>> +++ b/gsupplicant/supplicant.c
>> @@ -249,6 +249,51 @@ 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;
>> +};
>> +
>> +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;
>> + bool network_remove_in_progress;
>> + 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 +3521,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 +4113,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);
>
> Don't we now have a potential memory leak?
>
interface->network_path is now freed in network_remove_result. Idea is
that as soon as we removed the network we free the path. Earlier we
were freeing the path just before we were adding the new path. Now we
add the path from add_network_result and free it from
netwrok_remove_result.
>> supplicant_dbus_method_call(data->interface->path,
>> @@ -4656,7 +4663,7 @@ int
>> g_supplicant_interface_connect(GSupplicantInterface *interface,
>> void *user_data)
>> {
>> struct interface_connect_data *data;
>> - int ret;
>> + int ret = 0;
>>
>> if (!interface)
>> return -EINVAL;
>> @@ -4685,12 +4692,33 @@ 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) {
>> + data->network_remove_in_progress = TRUE;
>> + network_remove((struct interface_data *)data);
>
> With 'data' being of type struct interface_connect_data, this cast is
> very dangerous. It may work for a time, until someone makes changes to
> the data structures and then...
>
I did that and had to change the structure so that i do not have to
allocate the memory twice. So if I understood you correctly what you
want is that we have two local pointers in the function struct
interface_data * data; and struct interface_data_connect * connectdata
and allocate one or another based on whether we have a valid network
path or not?
> Is there any possibility that interface->network_path is already
> pointing to the correct network and not the previous one? So that we
> don't unnecessary remove the network just to add it again? Is the easy
> way out to check that data->path != interface->network_path?
>
Unfortunately there is no easy way. Because at connman layer the
network path does not reveal the SSID name. It is just a path having
an id. So it can happen that same id can be used by multiple networks
(SSID). So it is safe to do a remove network if path is not NULL.
This leak exists only in case clients are getting disconnected by AP
(by sending a deauth). If we are doing a connect/disconnect test b/w
two services we do not have any leak because connman ensures that it
disconnects with the first network before connecting with the second
one. The act of disconnecting with first one is going to remove the
network.
> If the network was disconnected with -ECONNABORTED in
> interface_disconnect_result(), do we need to still call NetworkRemove?
> Same applies to signal_network_removed().
>
This is to handle the case where connman is disconnecting from service
and when the service is disconnected we remove the network. As I
mentioned connman was removing the network correctly when the
disconnect is initiated from application. signal_network_removed is
when supplicant removes any network as a part of probably aging a scan
result.
>> + }
>> + else {
>
> On the same line, please.
>
So basically what I am trying to do here is that before we add a
network (which would end up creating a memory in wpa_supplicant) I see
if the network_path is non NULL or not? If it is non-NULL, it means
that the disconnect happened because of an incoming deauth and we did
not do a RemoveNetwork. So before we call AddNetwork we do a
removeNetwork and then once it is successful we proceed with the
AddNetwork.
>> + 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);
>> @@ -4716,12 +4744,23 @@ 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);
>> -
>> - dbus_free(data);
>> + if (data->network_remove_in_progress == TRUE) {
>> + data->network_remove_in_progress = FALSE;
>> + supplicant_dbus_method_call(data->interface->path,
>> + SUPPLICANT_INTERFACE ".Interface", "AddNetwork",
>> + interface_add_network_params,
>> + interface_add_network_result, data,
>> + data->interface);
>> + }
>> + else {
>> + g_free(data->path);
>> + if (data->callback)
>> + data->callback(result, data->interface,
>> data->user_data);
>> + dbus_free(data);
>> + }
>> }
>>
>> static void network_remove_params(DBusMessageIter *iter, void *user_data)
>
>
> Patrik
>
I will make the change and send you the modified patch for the structure issue?
Regards
Naveen
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 4, Issue 17
**************************************
