Hi Harish,
On Thu, Sep 28, 2017 at 9:09 AM, Harish Jenny K N
<harish_kandiga(a)mentor.com> wrote:
Changing of security type of one BSS is resulting in removing of
GSupplicantNetwork and other BSSs objects in the group.
The corresponding entries for other BSS in the same group are not
removed in other hash tables like bss_mapping and interface->bss_mapping.
This commit only deletes the network when there is a single entry in
network->bss_table during change in security for one bss.
This commit also avoids accessing any deleted bss object when reply
is got from async dbus call, by deleting any pending dbus calls
associated with the bss object.
---
gsupplicant/supplicant.c | 40 +++++++++++++++++++++++++++++++---------
1 file changed, 31 insertions(+), 9 deletions(-)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 4f79012..7d42404 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -755,6 +755,8 @@ static void remove_bss(gpointer data)
{
struct g_supplicant_bss *bss = data;
+ supplicant_dbus_property_call_cancel_all(bss);
+
g_free(bss->path);
g_free(bss);
}
@@ -2064,7 +2066,7 @@ static void interface_bss_added_without_keys(DBusMessageIter
*iter,
supplicant_dbus_property_get_all(bss->path,
SUPPLICANT_INTERFACE ".BSS",
- bss_property, bss, NULL);
+ bss_property, bss, bss);
bss_compute_security(bss);
if (add_or_replace_bss_to_network(bss) < 0)
@@ -2747,7 +2749,8 @@ static void signal_bss_changed(const char *path, DBusMessageIter
*iter)
if (old_security != bss->security) {
struct g_supplicant_bss *new_bss;
- SUPPLICANT_DBG("New network security for %s", bss->ssid);
+ SUPPLICANT_DBG("New network security for %s with path %s",
+ bss->ssid, bss->path);
/* Security change policy:
* - we first copy the current bss into a new one with
This description is not valid anymore.
@@ -2767,15 +2770,34 @@ static void signal_bss_changed(const char
*path, DBusMessageIter *iter)
memcpy(new_bss, bss, sizeof(struct g_supplicant_bss));
new_bss->path = g_strdup(bss->path);
- g_hash_table_remove(interface->network_table, network->group);
+ /* Clear the old bss pointer and remove the network completely
+ * if there are no more BSSs in the bss table. The new bss will
+ * be added either to an existing network or an additional
+ * network will be created */
I would mix this comment with the one above into a unique one where is
briefly described how things are done now:
/*
* Security change policy:
* - Create a copy ....
* - Remove old BSS's pointers ...
* - Remove network only if there are no more BSSs ...
* ...
*/
+ if (network->best_bss == bss) {
+ network->best_bss = NULL;
+ network->signal = BSS_UNKNOWN_STRENGTH;
+ }
+
+ g_hash_table_remove(bss_mapping, path);
+
+ g_hash_table_remove(interface->bss_mapping, path);
+ g_hash_table_remove(network->bss_table, path);
+
+ update_network_signal(network);
+
+ if (g_hash_table_size(network->bss_table) == 0)
+ g_hash_table_remove(interface->network_table,
+ network->group);
if (add_or_replace_bss_to_network(new_bss) < 0) {
- /* Remove entries in hash tables to handle the
- * failure in add_or_replace_bss_to_network
- */
- g_hash_table_remove(bss_mapping, path);
- g_hash_table_remove(interface->bss_mapping, path);
- g_hash_table_remove(network->bss_table, path);
+ /* Prevent a memory leak on failure in
+ * add_or_replace_bss_to_network */
+ SUPPLICANT_DBG("Failed to add bss %s to network
table",
+ new_bss->path);
+ g_free(new_bss->path);
+ g_free(new_bss);
}
return;
--
The patch looks fine. I created a simple scenario to test this use
case and the patch does what is intended to.
Thanks,
Jose Blanquicet