mirror of
https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git
synced 2026-03-05 08:50:39 +01:00
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
This commit is contained in:
Thomas Haller
parent
063f9185b9
commit
f1fba3eb02
4 changed files with 113 additions and 87 deletions
|
|
@ -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));
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
static void
|
||||
_sup_iface_last_ref (gpointer data,
|
||||
GObject *object,
|
||||
gboolean is_last_ref)
|
||||
{
|
||||
NMSupplicantManager *self = data;
|
||||
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)
|
||||
{
|
||||
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);
|
||||
|
|
|
|||
|
|
@ -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__ */
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue