Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe 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] iwd: fix crash related to initialization of network objects
(Alvin Šipraga)
2. Re: [PATCH] iwd: fix crash related to initialization of network objects
(Alvin Šipraga)
----------------------------------------------------------------------
Date: Fri, 12 Jun 2020 17:27:54 +0200
From: Alvin Šipraga <[email protected]>
Subject: [PATCH] iwd: fix crash related to initialization of network
objects
To: [email protected]
Cc: Alvin Šipraga <[email protected]>
Message-ID: <[email protected]>
Content-Type: text/plain
org.freedesktop.DBus.ObjectManager.GetManagedObjects returns a
dictionary of <objpath, ...>, which is iterated over in order to
initialize the different objects exposed by iwd. The order of iteration
with respect to the keys (object path) is not guaranteed.
Network objects in particular can only be properly initialized if the
corresponding device object has already been created. If not, then a
crash is possible when, for example, the iwd plugin handles changes to
the properties of the network object. In general, the code assumes that
the network object is properly initializated.
Address this problem by storing iwd_network objects without a
corresponding device object in a temporary pending_networks hash table.
When devices are created, the pending_network hash table is then
traveresed and any networks belonging to it are reinitialized suitably.
---
plugins/iwd.c | 77 +++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 68 insertions(+), 9 deletions(-)
diff --git a/plugins/iwd.c b/plugins/iwd.c
index bf6a2c26..21ee7b33 100644
--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -42,6 +42,7 @@ static GDBusProxy *agent_proxy;
static GHashTable *adapters;
static GHashTable *devices;
static GHashTable *networks;
+static GHashTable *pending_networks;
static GHashTable *known_networks;
static GHashTable *stations;
static GHashTable *access_points;
@@ -758,14 +759,14 @@ static char *create_identifier(const char *path, const
char *security)
return identifier;
}
-static void add_network(const char *path, struct iwd_network *iwdn)
+static bool add_network(const char *path, struct iwd_network *iwdn)
{
struct iwd_device *iwdd;
char *identifier;
iwdd = g_hash_table_lookup(devices, iwdn->device);
if (!iwdd)
- return;
+ return false;
identifier = create_identifier(path, iwdn->type);
iwdn->network = connman_network_create(identifier,
@@ -782,7 +783,7 @@ static void add_network(const char *path, struct
iwd_network *iwdn)
if (connman_device_add_network(iwdd->device, iwdn->network) < 0) {
connman_network_unref(iwdn->network);
iwdn->network = NULL;
- return;
+ return false;
}
iwdn->iwdd = iwdd;
@@ -790,6 +791,8 @@ static void add_network(const char *path, struct
iwd_network *iwdn)
connman_network_set_group(iwdn->network, identifier);
g_free(identifier);
+
+ return true;
}
static void remove_network(struct iwd_network *iwdn)
@@ -1240,6 +1243,8 @@ static void create_adapter(GDBusProxy *proxy)
adapter_property_change, NULL);
}
+static gboolean match_pending_network(gpointer key, gpointer value, gpointer
user_data);
+
static void create_device(GDBusProxy *proxy)
{
const char *path = g_dbus_proxy_get_path(proxy);
@@ -1277,6 +1282,9 @@ static void create_device(GDBusProxy *proxy)
device_property_change, NULL);
add_device(path, iwdd);
+
+ g_hash_table_foreach_remove(pending_networks, match_pending_network,
+ iwdd);
}
static void unregister_agent();
@@ -1413,7 +1421,7 @@ static void iwd_is_out(DBusConnection *conn, void
*user_data)
}
}
-static void create_network(GDBusProxy *proxy)
+static bool create_network(GDBusProxy *proxy)
{
const char *path = g_dbus_proxy_get_path(proxy);
struct iwd_network *iwdn;
@@ -1422,7 +1430,7 @@ static void create_network(GDBusProxy *proxy)
if (!iwdn) {
connman_error("Out of memory creating IWD network");
- return;
+ return false;
}
iwdn->path = g_strdup(path);
@@ -1433,7 +1441,7 @@ static void create_network(GDBusProxy *proxy)
if (!iwdn->proxy) {
connman_error("Cannot create IWD network watcher %s", path);
g_hash_table_remove(networks, path);
- return;
+ return false;
}
iwdn->device = g_strdup(proxy_get_string(proxy, "Device"));
@@ -1446,10 +1454,55 @@ static void create_network(GDBusProxy *proxy)
iwdn->device, iwdn->name, iwdn->type, iwdn->connected,
iwdn->known_network);
- g_dbus_proxy_set_property_watch(iwdn->proxy,
- network_property_change, NULL);
+ /*
+ * If the device corresponding to this network has not been added yet,
+ * add_network() will fail. In this case, treat the network as as
+ * pending, and match it when the device has been added.
+ */
+ if (!add_network(path, iwdn)) {
+ /*
+ * Skip this step if the network is already pending, so that
+ * pending_networks does not get modified.
+ */
+ if (g_hash_table_contains(pending_networks, iwdn->path)) {
+ g_hash_table_remove(networks, path);
+ } else {
+ g_hash_table_steal(networks, path);
+ g_hash_table_replace(pending_networks, iwdn->path,
+ iwdn);
+ DBG("network '%s' is pending", iwdn->name);
+ }
+
+ return false;
+ }
+
+ g_dbus_proxy_set_property_watch(iwdn->proxy, network_property_change,
+ NULL);
- add_network(path, iwdn);
+ return true;
+}
+
+static gboolean match_pending_network(gpointer key, gpointer value, gpointer
user_data)
+{
+ struct iwd_network *iwdn = value;
+ struct iwd_device *iwdd = user_data;
+ GDBusProxy *proxy;
+
+ DBG("network '%s' on device '%s' match device '%s'", iwdn->name,
iwdn->device,
+ iwdd->path);
+
+ if (strcmp(iwdn->device, iwdd->path))
+ return FALSE;
+
+ if (!create_network(iwdn->proxy)) {
+ connman_error("failed to match network '%s' with device '%s'",
+ iwdn->name, iwdd->path);
+ return FALSE;
+ }
+
+ DBG("linked device '%s' with network '%s'", iwdd->name, iwdn->name);
+
+ return TRUE;
}
struct auto_connect_cb_data {
@@ -1734,6 +1787,9 @@ static int iwd_init(void)
networks = g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
network_free);
+ pending_networks = g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
+ network_free);
+
known_networks = g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
known_network_free);
@@ -1782,6 +1838,9 @@ out:
if (networks)
g_hash_table_destroy(networks);
+ if (pending_networks)
+ g_hash_table_destroy(pending_networks);
+
if (known_networks)
g_hash_table_destroy(known_networks);
--
2.26.2
------------------------------
Date: Fri, 12 Jun 2020 17:34:35 +0200
From: Alvin Šipraga <[email protected]>
Subject: Re: [PATCH] iwd: fix crash related to initialization of
network objects
To: [email protected]
Cc: Alvin Šipraga <[email protected]>
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hello again,
In response to my recent email, I made this patch which addresses the
crash I was experiencing. It is not very pretty, but it seems that there
is little control over the order of iteration over the dictionary
returned by D-Bus, so something has to be done to work around that. This
was the least invasive way I could find to fix things.
It is a little bit brittle because create_network() is called (via
match_pending_network()) in a g_hash_table_foreach_remove() loop. Such
loops do not allow modification of the underlying hash table
(pending_networks), and so there is some deliberate logic in
create_network() to make things safe.
Any feedback would be welcome.
Kind regards,
Alvin
On 6/12/20 5:27 PM, Alvin Šipraga wrote:
> org.freedesktop.DBus.ObjectManager.GetManagedObjects returns a
> dictionary of <objpath, ...>, which is iterated over in order to
> initialize the different objects exposed by iwd. The order of iteration
> with respect to the keys (object path) is not guaranteed.
>
> Network objects in particular can only be properly initialized if the
> corresponding device object has already been created. If not, then a
> crash is possible when, for example, the iwd plugin handles changes to
> the properties of the network object. In general, the code assumes that
> the network object is properly initializated.
>
> Address this problem by storing iwd_network objects without a
> corresponding device object in a temporary pending_networks hash table.
> When devices are created, the pending_network hash table is then
> traveresed and any networks belonging to it are reinitialized suitably.
> ---
> plugins/iwd.c | 77 +++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 68 insertions(+), 9 deletions(-)
>
> diff --git a/plugins/iwd.c b/plugins/iwd.c
> index bf6a2c26..21ee7b33 100644
> --- a/plugins/iwd.c
> +++ b/plugins/iwd.c
> @@ -42,6 +42,7 @@ static GDBusProxy *agent_proxy;
> static GHashTable *adapters;
> static GHashTable *devices;
> static GHashTable *networks;
> +static GHashTable *pending_networks;
> static GHashTable *known_networks;
> static GHashTable *stations;
> static GHashTable *access_points;
> @@ -758,14 +759,14 @@ static char *create_identifier(const char *path, const
> char *security)
> return identifier;
> }
>
> -static void add_network(const char *path, struct iwd_network *iwdn)
> +static bool add_network(const char *path, struct iwd_network *iwdn)
> {
> struct iwd_device *iwdd;
> char *identifier;
>
> iwdd = g_hash_table_lookup(devices, iwdn->device);
> if (!iwdd)
> - return;
> + return false;
>
> identifier = create_identifier(path, iwdn->type);
> iwdn->network = connman_network_create(identifier,
> @@ -782,7 +783,7 @@ static void add_network(const char *path, struct
> iwd_network *iwdn)
> if (connman_device_add_network(iwdd->device, iwdn->network) < 0) {
> connman_network_unref(iwdn->network);
> iwdn->network = NULL;
> - return;
> + return false;
> }
> iwdn->iwdd = iwdd;
>
> @@ -790,6 +791,8 @@ static void add_network(const char *path, struct
> iwd_network *iwdn)
> connman_network_set_group(iwdn->network, identifier);
>
> g_free(identifier);
> +
> + return true;
> }
>
> static void remove_network(struct iwd_network *iwdn)
> @@ -1240,6 +1243,8 @@ static void create_adapter(GDBusProxy *proxy)
> adapter_property_change, NULL);
> }
>
> +static gboolean match_pending_network(gpointer key, gpointer value, gpointer
> user_data);
> +
> static void create_device(GDBusProxy *proxy)
> {
> const char *path = g_dbus_proxy_get_path(proxy);
> @@ -1277,6 +1282,9 @@ static void create_device(GDBusProxy *proxy)
> device_property_change, NULL);
>
> add_device(path, iwdd);
> +
> + g_hash_table_foreach_remove(pending_networks, match_pending_network,
> + iwdd);
> }
>
> static void unregister_agent();
> @@ -1413,7 +1421,7 @@ static void iwd_is_out(DBusConnection *conn, void
> *user_data)
> }
> }
>
> -static void create_network(GDBusProxy *proxy)
> +static bool create_network(GDBusProxy *proxy)
> {
> const char *path = g_dbus_proxy_get_path(proxy);
> struct iwd_network *iwdn;
> @@ -1422,7 +1430,7 @@ static void create_network(GDBusProxy *proxy)
>
> if (!iwdn) {
> connman_error("Out of memory creating IWD network");
> - return;
> + return false;
> }
>
> iwdn->path = g_strdup(path);
> @@ -1433,7 +1441,7 @@ static void create_network(GDBusProxy *proxy)
> if (!iwdn->proxy) {
> connman_error("Cannot create IWD network watcher %s", path);
> g_hash_table_remove(networks, path);
> - return;
> + return false;
> }
>
> iwdn->device = g_strdup(proxy_get_string(proxy, "Device"));
> @@ -1446,10 +1454,55 @@ static void create_network(GDBusProxy *proxy)
> iwdn->device, iwdn->name, iwdn->type, iwdn->connected,
> iwdn->known_network);
>
> - g_dbus_proxy_set_property_watch(iwdn->proxy,
> - network_property_change, NULL);
> + /*
> + * If the device corresponding to this network has not been added yet,
> + * add_network() will fail. In this case, treat the network as as
> + * pending, and match it when the device has been added.
> + */
> + if (!add_network(path, iwdn)) {
> + /*
> + * Skip this step if the network is already pending, so that
> + * pending_networks does not get modified.
> + */
> + if (g_hash_table_contains(pending_networks, iwdn->path)) {
> + g_hash_table_remove(networks, path);
> + } else {
> + g_hash_table_steal(networks, path);
> + g_hash_table_replace(pending_networks, iwdn->path,
> + iwdn);
> + DBG("network '%s' is pending", iwdn->name);
> + }
> +
> + return false;
> + }
> +
> + g_dbus_proxy_set_property_watch(iwdn->proxy, network_property_change,
> + NULL);
>
> - add_network(path, iwdn);
> + return true;
> +}
> +
> +static gboolean match_pending_network(gpointer key, gpointer value, gpointer
> user_data)
> +{
> + struct iwd_network *iwdn = value;
> + struct iwd_device *iwdd = user_data;
> + GDBusProxy *proxy;
> +
> + DBG("network '%s' on device '%s' match device '%s'", iwdn->name,
> iwdn->device,
> + iwdd->path);
> +
> + if (strcmp(iwdn->device, iwdd->path))
> + return FALSE;
> +
> + if (!create_network(iwdn->proxy)) {
> + connman_error("failed to match network '%s' with device '%s'",
> + iwdn->name, iwdd->path);
> + return FALSE;
> + }
> +
> + DBG("linked device '%s' with network '%s'", iwdd->name, iwdn->name);
> +
> + return TRUE;
> }
>
> struct auto_connect_cb_data {
> @@ -1734,6 +1787,9 @@ static int iwd_init(void)
> networks = g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
> network_free);
>
> + pending_networks = g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
> + network_free);
> +
> known_networks = g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
> known_network_free);
>
> @@ -1782,6 +1838,9 @@ out:
> if (networks)
> g_hash_table_destroy(networks);
>
> + if (pending_networks)
> + g_hash_table_destroy(pending_networks);
> +
> if (known_networks)
> g_hash_table_destroy(known_networks);
>
>
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list -- [email protected]
To unsubscribe send an email to [email protected]
------------------------------
End of connman Digest, Vol 56, Issue 3
**************************************
