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 (Patrik Flykt)
----------------------------------------------------------------------
Message: 1
Date: Mon, 15 Feb 2016 16:47:16 +0200
From: Patrik Flykt <[email protected]>
To: Naveen Singh <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Fri, 2016-02-12 at 14:08 -0800, Naveen Singh wrote:
> 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.
Well, to keep it safe, freeing it has been done right before adding it
again. Usually strduping a variable without freeing its previous
contents just causes unnecessary concern that it will leak memory.
>
> >> 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?
When network_remove() is called, it needs to be given data in the
correct struct and containing proper values, as it will pass that
structure on to its callback. The callback then accesses the various
members. If it is casted, it uses the wrong offset when calling the
supplied function callback...
> > 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.
Is the latter part taken care of?
>
> >> + }
> >> + 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.
So when we know that the network is gone from wpa_supplicant, ConnMan
needs to call a RemoveNetwork at that point? Which situations require
ConnMan to call RemoveNetwork?
> >> + 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
Patrik
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 4, Issue 18
**************************************
