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);
>
>