wifi: fix crash due to wrong ownership handling in nm_supplicant_manager_iface_release()

nm_supplicant_manager_iface_get() would cache and reuse the supplicant
interface. But no ref-counting was in place so that the first user returning
the interface via nm_supplicant_manager_iface_release() would destroy the
instance for others.

This is broken for a very long time. Which shows that we hardly ever
have a cache-hit and usually create a new instance. So, instead of
letting nm_supplicant_manager_create_interface() check for existing
supplicant interface, always create a new instance. This also makes
sense, because we would expect that per ifname only one instance is
requested at a time. Also add an assertion that we don't return
multiple supplicant interface instances for the same ifname.

Drop nm_supplicant_manager_iface_release() in favor of requiring users
to unref the returned instance.

Also, use a GSList instead of a GHashTable for the cache.

Also, previously callers would pass @is_wireless to nm_supplicant_manager_iface_get(),
but the cache lookup did not consider that value. That doesn't matter
now as we always create a new instance.

https://bugzilla.redhat.com/show_bug.cgi?id=1298007

4 files changed, 113 insertions, 87 deletions

diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c
index 7ff6d685f3..91fd0f7396 100644
--- a/src/devices/nm-device-ethernet.c
+++ b/src/devices/nm-device-ethernet.c
@@ -474,8 +474,7 @@ supplicant_interface_release (NMDeviceEthernet *self)
 
 	if (priv->supplicant.iface) {
 		nm_supplicant_interface_disconnect (priv->supplicant.iface);
-		nm_supplicant_manager_iface_release (priv->supplicant.mgr, priv->supplicant.iface);
-		priv->supplicant.iface = NULL;
+		g_clear_object (&priv->supplicant.iface);
 	}
 }
 
@@ -784,14 +783,15 @@ supplicant_interface_init (NMDeviceEthernet *self)
 {
 	NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self);
 
-	/* Create supplicant interface */
-	priv->supplicant.iface = nm_supplicant_manager_iface_get (priv->supplicant.mgr,
-	                                                          nm_device_get_iface (NM_DEVICE (self)),
-	                                                          FALSE);
+	supplicant_interface_release (self);
+
+	priv->supplicant.iface = nm_supplicant_manager_create_interface (priv->supplicant.mgr,
+	                                                                 nm_device_get_iface (NM_DEVICE (self)),
+	                                                                 FALSE);
+
 	if (!priv->supplicant.iface) {
 		_LOGE (LOGD_DEVICE | LOGD_ETHER,
 		       "Couldn't initialize supplicant interface");
-		supplicant_interface_release (self);
 		return FALSE;
 	}
 
@@ -1633,6 +1633,8 @@ dispose (GObject *object)
 	NMDeviceEthernet *self = NM_DEVICE_ETHERNET (object);
 	NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self);
 
+	supplicant_interface_release (self);
+
 	nm_clear_g_source (&priv->pppoe_wait_id);
 
 	dcb_timeout_cleanup (NM_DEVICE (self));
diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c
index dbe6f33fb2..2df0937725 100644
--- a/src/devices/wifi/nm-device-wifi.c
+++ b/src/devices/wifi/nm-device-wifi.c
@@ -192,13 +192,12 @@ supplicant_interface_acquire (NMDeviceWifi *self)
 	NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
 
 	g_return_val_if_fail (self != NULL, FALSE);
-	/* interface already acquired? */
-	g_return_val_if_fail (priv->sup_iface == NULL, TRUE);
+	g_return_val_if_fail (!priv->sup_iface, TRUE);
 
-	priv->sup_iface = nm_supplicant_manager_iface_get (priv->sup_mgr,
-	                                                   nm_device_get_iface (NM_DEVICE (self)),
-	                                                   TRUE);
-	if (priv->sup_iface == NULL) {
+	priv->sup_iface = nm_supplicant_manager_create_interface (priv->sup_mgr,
+	                                                          nm_device_get_iface (NM_DEVICE (self)),
+	                                                          TRUE);
+	if (!priv->sup_iface) {
 		_LOGE (LOGD_WIFI, "Couldn't initialize supplicant interface");
 		return FALSE;
 	}
@@ -263,8 +262,7 @@ supplicant_interface_release (NMDeviceWifi *self)
 		/* Tell the supplicant to disconnect from the current AP */
 		nm_supplicant_interface_disconnect (priv->sup_iface);
 
-		nm_supplicant_manager_iface_release (priv->sup_mgr, priv->sup_iface);
-		priv->sup_iface = NULL;
+		g_clear_object (&priv->sup_iface);
 	}
 }
 
@@ -2195,7 +2193,7 @@ build_supplicant_config (NMDeviceWifi *self,
 	NMSettingMacRandomization mac_randomization_fallback;
 	gs_free char *svalue = NULL;
 
-	g_return_val_if_fail (self != NULL, NULL);
+	g_return_val_if_fail (priv->sup_iface, NULL);
 
 	s_wireless = nm_connection_get_setting_wireless (connection);
 	g_return_val_if_fail (s_wireless != NULL, NULL);
diff --git a/src/supplicant-manager/nm-supplicant-manager.c b/src/supplicant-manager/nm-supplicant-manager.c
index ac2b1fb867..14254266a3 100644
--- a/src/supplicant-manager/nm-supplicant-manager.c
+++ b/src/supplicant-manager/nm-supplicant-manager.c
@@ -40,7 +40,7 @@ typedef struct {
 	GCancellable *   cancellable;
 	gboolean         running;
 
-	GHashTable *      ifaces;
+	GSList          *ifaces;
 	gboolean          fast_supported;
 	NMSupplicantFeature ap_support;
 	guint             die_count_reset_id;
@@ -70,64 +70,29 @@ is_available (NMSupplicantManager *self)
 
 /********************************************************************/
 
-NMSupplicantInterface *
-nm_supplicant_manager_iface_get (NMSupplicantManager * self,
-                                 const char *ifname,
-                                 gboolean is_wireless)
-{
-	NMSupplicantManagerPrivate *priv;
-	NMSupplicantInterface *iface = NULL;
-
-	g_return_val_if_fail (NM_IS_SUPPLICANT_MANAGER (self), NULL);
-	g_return_val_if_fail (ifname != NULL, NULL);
-
-	priv = NM_SUPPLICANT_MANAGER_GET_PRIVATE (self);
-
-	iface = g_hash_table_lookup (priv->ifaces, ifname);
-	if (!iface) {
-		nm_log_dbg (LOGD_SUPPLICANT, "(%s): creating new supplicant interface", ifname);
-		iface = nm_supplicant_interface_new (ifname,
-		                                     is_wireless,
-		                                     priv->fast_supported,
-		                                     priv->ap_support);
-		g_hash_table_insert (priv->ifaces,
-		                     (char *) nm_supplicant_interface_get_ifname (iface),
-		                     iface);
-
-		/* If we're making the supplicant take a time out for a bit, don't
-		 * let the supplicant interface start immediately, just let it hang
-		 * around in INIT state until we're ready to talk to the supplicant
-		 * again.
-		 */
-		if (is_available (self))
-			nm_supplicant_interface_set_supplicant_available (iface, TRUE);
-	} else {
-		nm_log_dbg (LOGD_SUPPLICANT, "(%s): returning existing supplicant interface", ifname);
-	}
-
-	return iface;
-}
-
-void
-nm_supplicant_manager_iface_release (NMSupplicantManager *self,
-                                     NMSupplicantInterface *iface)
+static void
+_sup_iface_last_ref (gpointer data,
+                     GObject *object,
+                     gboolean is_last_ref)
 {
+	NMSupplicantManager *self = data;
 	NMSupplicantManagerPrivate *priv;
-	const char *ifname, *op;
+	NMSupplicantInterface *sup_iface = (NMSupplicantInterface *) object;
+	const char *op;
 
 	g_return_if_fail (NM_IS_SUPPLICANT_MANAGER (self));
-	g_return_if_fail (NM_IS_SUPPLICANT_INTERFACE (iface));
-
-	ifname = nm_supplicant_interface_get_ifname (iface);
-	g_assert (ifname);
+	g_return_if_fail (NM_IS_SUPPLICANT_INTERFACE (sup_iface));
+	g_return_if_fail (is_last_ref);
 
 	priv = NM_SUPPLICANT_MANAGER_GET_PRIVATE (self);
 
-	g_return_if_fail (g_hash_table_lookup (priv->ifaces, ifname) == iface);
+	if (!g_slist_find (priv->ifaces, sup_iface))
+		g_return_if_reached ();
 
 	/* Ask wpa_supplicant to remove this interface */
-	op = nm_supplicant_interface_get_object_path (iface);
-	if (priv->running && priv->proxy && op) {
+	if (   priv->running
+	    && priv->proxy
+	    && (op = nm_supplicant_interface_get_object_path (sup_iface))) {
 		g_dbus_proxy_call (priv->proxy,
 		                   "RemoveInterface",
 		                   g_variant_new ("(o)", op),
@@ -138,15 +103,69 @@ nm_supplicant_manager_iface_release (NMSupplicantManager *self,
 		                   NULL);
 	}
 
-	g_hash_table_remove (priv->ifaces, ifname);
+	priv->ifaces = g_slist_remove (priv->ifaces, sup_iface);
+	g_object_remove_toggle_ref ((GObject *) sup_iface, _sup_iface_last_ref, self);
+}
+
+/**
+ * nm_supplicant_manager_create_interface:
+ * @self: the #NMSupplicantManager
+ * @ifname: the interface for which to obtain the supplicant interface
+ * @is_wireless: whether the interface is supposed to be wireless.
+ *
+ * Note: the manager owns a reference to the instance and the only way to
+ *   get the manager to release it, is by dropping all other references
+ *   to the supplicant-interface (or destroying the manager).
+ *
+ * Retruns: (transfer-full): returns a #NMSupplicantInterface or %NULL.
+ *   Must be unrefed at the end.
+ * */
+NMSupplicantInterface *
+nm_supplicant_manager_create_interface (NMSupplicantManager *self,
+                                        const char *ifname,
+                                        gboolean is_wireless)
+{
+	NMSupplicantManagerPrivate *priv;
+	NMSupplicantInterface *iface;
+	GSList *ifaces;
+
+	g_return_val_if_fail (NM_IS_SUPPLICANT_MANAGER (self), NULL);
+	g_return_val_if_fail (ifname != NULL, NULL);
+
+	priv = NM_SUPPLICANT_MANAGER_GET_PRIVATE (self);
+
+	nm_log_dbg (LOGD_SUPPLICANT, "(%s): creating new supplicant interface", ifname);
+
+	/* assert against not requesting duplicate interfaces. */
+	for (ifaces = priv->ifaces; ifaces; ifaces = ifaces->next) {
+		if (g_strcmp0 (nm_supplicant_interface_get_ifname (ifaces->data), ifname) == 0)
+			g_return_val_if_reached (NULL);
+	}
+
+	iface = nm_supplicant_interface_new (ifname,
+	                                     is_wireless,
+	                                     priv->fast_supported,
+	                                     priv->ap_support);
+
+	priv->ifaces = g_slist_prepend (priv->ifaces, iface);
+	g_object_add_toggle_ref ((GObject *) iface, _sup_iface_last_ref, self);
+
+	/* If we're making the supplicant take a time out for a bit, don't
+	 * let the supplicant interface start immediately, just let it hang
+	 * around in INIT state until we're ready to talk to the supplicant
+	 * again.
+	 */
+	if (is_available (self))
+		nm_supplicant_interface_set_supplicant_available (iface, TRUE);
+
+	return iface;
 }
 
 static void
 update_capabilities (NMSupplicantManager *self)
 {
 	NMSupplicantManagerPrivate *priv = NM_SUPPLICANT_MANAGER_GET_PRIVATE (self);
-	NMSupplicantInterface *iface;
-	GHashTableIter hash_iter;
+	GSList *ifaces;
 	const char **array;
 	GVariant *value;
 
@@ -174,9 +193,8 @@ update_capabilities (NMSupplicantManager *self)
 	}
 
 	/* Tell all interfaces about results of the AP check */
-	g_hash_table_iter_init (&hash_iter, priv->ifaces);
-	while (g_hash_table_iter_next (&hash_iter, NULL, (gpointer) &iface))
-		nm_supplicant_interface_set_ap_support (iface, priv->ap_support);
+	for (ifaces = priv->ifaces; ifaces; ifaces = ifaces->next)
+		nm_supplicant_interface_set_ap_support (ifaces->data, priv->ap_support);
 
 	nm_log_dbg (LOGD_SUPPLICANT, "AP mode is %ssupported",
 	            (priv->ap_support == NM_SUPPLICANT_FEATURE_YES) ? "" :
@@ -202,15 +220,20 @@ static void
 availability_changed (NMSupplicantManager *self, gboolean available)
 {
 	NMSupplicantManagerPrivate *priv = NM_SUPPLICANT_MANAGER_GET_PRIVATE (self);
-	GList *ifaces, *iter;
+	GSList *ifaces, *iter;
 
-	/* priv->ifaces may be modified if availability changes; can't use GHashTableIter */
-	ifaces = g_hash_table_get_values (priv->ifaces);
+	if (!priv->ifaces)
+		return;
+
+	/* setting the supplicant as unavailable might cause the caller to unref
+	 * the supplicant (and thus remove the instance from the list of interfaces.
+	 * Delay that by taking an additional reference first. */
+	ifaces = g_slist_copy (priv->ifaces);
 	for (iter = ifaces; iter; iter = iter->next)
 		g_object_ref (iter->data);
 	for (iter = ifaces; iter; iter = iter->next)
-		nm_supplicant_interface_set_supplicant_available (NM_SUPPLICANT_INTERFACE (iter->data), available);
-	g_list_free_full (ifaces, g_object_unref);
+		nm_supplicant_interface_set_supplicant_available (iter->data, available);
+	g_slist_free_full (ifaces, g_object_unref);
 }
 
 static void
@@ -326,8 +349,6 @@ nm_supplicant_manager_init (NMSupplicantManager *self)
 {
 	NMSupplicantManagerPrivate *priv = NM_SUPPLICANT_MANAGER_GET_PRIVATE (self);
 
-	priv->ifaces = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref);
-
 	priv->cancellable = g_cancellable_new ();
 	g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM,
 	                          G_DBUS_PROXY_FLAGS_NONE,
@@ -343,7 +364,9 @@ nm_supplicant_manager_init (NMSupplicantManager *self)
 static void
 dispose (GObject *object)
 {
-	NMSupplicantManagerPrivate *priv = NM_SUPPLICANT_MANAGER_GET_PRIVATE (object);
+	NMSupplicantManager *self = (NMSupplicantManager *) object;
+	NMSupplicantManagerPrivate *priv = NM_SUPPLICANT_MANAGER_GET_PRIVATE (self);
+	GSList *ifaces;
 
 	nm_clear_g_source (&priv->die_count_reset_id);
 
@@ -352,7 +375,13 @@ dispose (GObject *object)
 		g_clear_object (&priv->cancellable);
 	}
 
-	g_clear_pointer (&priv->ifaces, g_hash_table_unref);
+	if (priv->ifaces) {
+		for (ifaces = priv->ifaces; ifaces; ifaces = ifaces->next)
+			g_object_remove_toggle_ref (ifaces->data, _sup_iface_last_ref, self);
+		g_slist_free (priv->ifaces);
+		priv->ifaces = NULL;
+	}
+
 	g_clear_object (&priv->proxy);
 
 	G_OBJECT_CLASS (nm_supplicant_manager_parent_class)->dispose (object);
diff --git a/src/supplicant-manager/nm-supplicant-manager.h b/src/supplicant-manager/nm-supplicant-manager.h
index d7456467de..b0ce6a523f 100644
--- a/src/supplicant-manager/nm-supplicant-manager.h
+++ b/src/supplicant-manager/nm-supplicant-manager.h
@@ -49,11 +49,8 @@ GType nm_supplicant_manager_get_type (void);
 
 NMSupplicantManager *nm_supplicant_manager_get (void);
 
-NMSupplicantInterface *nm_supplicant_manager_iface_get (NMSupplicantManager *mgr,
-                                                        const char *ifname,
-                                                        gboolean is_wireless);
-
-void nm_supplicant_manager_iface_release (NMSupplicantManager *mgr,
-                                          NMSupplicantInterface *iface);
+NMSupplicantInterface *nm_supplicant_manager_create_interface (NMSupplicantManager *mgr,
+                                                               const char *ifname,
+                                                               gboolean is_wireless);
 
 #endif /* __NETWORKMANAGER_SUPPLICANT_MANAGER_H__ */