From b04a9c90eb0a866f2d730cc4777b4f670220ddc7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Apr 2017 15:46:56 +0200 Subject: proxy: introduce call-id for clearing pacmanager configuration nm_pacrunner_manager_remove() required a "tag" argument. It was a bug for callers trying to remove a configuration for a non-existing tag. That effectively means, the caller must keep track of whether a certain "tag" is pending. The caller also must remember the tag -- a tag that he must choose uniquely in the first place. Turn that around and have nm_pacrunner_manager_send() return a (non NULL) call-id. This call-id may later be used to remove the configuration. Apparently, previously the tracking of the "tag" was not always correct and we hit the assertion in nm_pacrunner_manager_remove(). https://bugzilla.redhat.com/show_bug.cgi?id=1444374 --- src/devices/nm-device.c | 55 +++++++++---------- src/nm-pacrunner-manager.c | 131 +++++++++++++++++++++++++------------------- src/nm-pacrunner-manager.h | 22 +++++--- src/vpn/nm-vpn-connection.c | 33 ++++++----- 4 files changed, 137 insertions(+), 104 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 160e5a7259..2b17db97b6 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -323,7 +323,7 @@ typedef struct _NMDevicePrivate { /* Proxy Configuration */ NMProxyConfig *proxy_config; NMPacrunnerManager *pacrunner_manager; - bool proxy_config_sent; + NMPacrunnerCallId *pacrunner_call_id; /* IP4 configuration info */ NMIP4Config * ip4_config; /* Combined config from VPN, settings, and device */ @@ -8880,23 +8880,32 @@ nm_device_reactivate_ip6_config (NMDevice *self, } static void -reactivate_proxy_config (NMDevice *self) +_pacrunner_manager_send (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - if (!priv->proxy_config_sent) - return; + nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, + &priv->pacrunner_call_id); - nm_pacrunner_manager_remove (priv->pacrunner_manager, - nm_device_get_ip_iface (self)); + if (!priv->pacrunner_manager) + priv->pacrunner_manager = g_object_ref (nm_pacrunner_manager_get ()); + + priv->pacrunner_call_id = nm_pacrunner_manager_send (priv->pacrunner_manager, + nm_device_get_ip_iface (self), + priv->proxy_config, + NULL, + NULL); +} + +static void +reactivate_proxy_config (NMDevice *self) +{ + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + if (!priv->pacrunner_call_id) + return; nm_device_set_proxy_config (self, priv->dhcp4.pac_url); - nm_pacrunner_manager_send (priv->pacrunner_manager, - nm_device_get_ip_iface (self), - nm_device_get_ip_iface (self), - priv->proxy_config, - NULL, - NULL); + _pacrunner_manager_send (self); } static gboolean @@ -12565,11 +12574,8 @@ _set_state_full (NMDevice *self, } } - if (priv->proxy_config_sent) { - nm_pacrunner_manager_remove (priv->pacrunner_manager, - nm_device_get_ip_iface (self)); - priv->proxy_config_sent = FALSE; - } + nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, + &priv->pacrunner_call_id); break; case NM_DEVICE_STATE_DISCONNECTED: if ( priv->queued_act_request @@ -12594,15 +12600,8 @@ _set_state_full (NMDevice *self, req, NULL, NULL, NULL); - if (priv->proxy_config) { - nm_pacrunner_manager_send (priv->pacrunner_manager, - nm_device_get_ip_iface (self), - nm_device_get_ip_iface (self), - priv->proxy_config, - NULL, - NULL); - priv->proxy_config_sent = TRUE; - } + if (priv->proxy_config) + _pacrunner_manager_send (self); break; case NM_DEVICE_STATE_FAILED: /* Usually upon failure the activation chain is interrupted in @@ -13608,8 +13607,6 @@ nm_device_init (NMDevice *self) priv->ip6_saved_properties = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_free); priv->sys_iface_state = NM_DEVICE_SYS_IFACE_STATE_EXTERNAL; - priv->pacrunner_manager = g_object_ref (nm_pacrunner_manager_get ()); - priv->default_route.v4_is_assumed = TRUE; priv->default_route.v6_is_assumed = TRUE; @@ -13734,6 +13731,8 @@ dispose (GObject *object) dispatcher_cleanup (self); + nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, + &priv->pacrunner_call_id); g_clear_object (&priv->pacrunner_manager); _cleanup_generic_pre (self, CLEANUP_TYPE_KEEP); diff --git a/src/nm-pacrunner-manager.c b/src/nm-pacrunner-manager.c index d4a5b5ab9a..65ac888e33 100644 --- a/src/nm-pacrunner-manager.c +++ b/src/nm-pacrunner-manager.c @@ -36,14 +36,15 @@ static void pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointe /*****************************************************************************/ -typedef struct { - char *tag; +struct _NMPacrunnerCallId { NMPacrunnerManager *manager; GVariant *args; char *path; guint refcount; bool removed; -} Config; +}; + +typedef struct _NMPacrunnerCallId Config; typedef struct { char *iface; @@ -77,13 +78,12 @@ NM_DEFINE_SINGLETON_GETTER (NMPacrunnerManager, nm_pacrunner_manager_get, NM_TYP /*****************************************************************************/ static Config * -config_new (NMPacrunnerManager *manager, char *tag, GVariant *args) +config_new (NMPacrunnerManager *manager, GVariant *args) { Config *config; config = g_slice_new0 (Config); config->manager = manager; - config->tag = tag; config->args = g_variant_ref_sink (args); config->refcount = 1; @@ -106,7 +106,6 @@ config_unref (Config *config) g_assert (config->refcount > 0); if (config->refcount == 1) { - g_free (config->tag); g_variant_unref (config->args); g_free (config->path); g_slice_free (Config, config); @@ -233,12 +232,12 @@ pacrunner_send_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data) priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); if (!variant) { - _LOGD ("send config for '%s' failed: %s", config->tag, error->message); + _LOGD ("send config for '%p' failed: %s", config, error->message); } else { g_variant_get (variant, "(&o)", &path); config->path = g_strdup (path); - _LOGD ("successfully sent config for '%s'", config->tag); + _LOGD ("successfully sent config for '%p'", config); if (config->removed) { g_dbus_proxy_call (priv->pacrunner, @@ -262,7 +261,7 @@ pacrunner_send_config (NMPacrunnerManager *self, Config *config) if (priv->pacrunner) { gs_free char *args_str = NULL; - _LOGT ("sending proxy config for '%s': %s", config->tag, + _LOGT ("sending proxy config for '%p': %s", config, (args_str = g_variant_print (config->args, FALSE))); config_ref (config); @@ -328,15 +327,21 @@ pacrunner_proxy_cb (GObject *source, GAsyncResult *res, gpointer user_data) * nm_pacrunner_manager_send: * @self: the #NMPacrunnerManager * @iface: the iface for the connection or %NULL - * @tag: unique configuration identifier * @proxy_config: proxy config of the connection * @ip4_config: IP4 config of the connection to extract domain info from * @ip6_config: IP6 config of the connection to extract domain info from + * + * Returns: a #NMPacrunnerCallId call id. The function cannot + * fail and always returns a non NULL pointer. The call-id may + * be used to remove the configuration later via nm_pacrunner_manager_remove(). + * Note that the call-id does not keep the @self instance alive. + * If you plan to remove the configuration later, you must keep + * the instance alive long enough. You can remove the configuration + * at most once using this call call-id. */ -void +NMPacrunnerCallId * nm_pacrunner_manager_send (NMPacrunnerManager *self, const char *iface, - const char *tag, NMProxyConfig *proxy_config, NMIP4Config *ip4_config, NMIP6Config *ip6_config) @@ -348,8 +353,8 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self, GPtrArray *domains; Config *config; - g_return_if_fail (NM_IS_PACRUNNER_MANAGER (self)); - g_return_if_fail (proxy_config); + g_return_val_if_fail (NM_IS_PACRUNNER_MANAGER (self), NULL); + g_return_val_if_fail (proxy_config, NULL); priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); @@ -401,8 +406,7 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self, } } - config = config_new (self, g_strdup (tag), - g_variant_new ("(a{sv})", &proxy_data)); + config = config_new (self, g_variant_new ("(a{sv})", &proxy_data)); priv->configs = g_list_append (priv->configs, config); /* Send if pacrunner is available on bus, otherwise @@ -410,6 +414,8 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self, * sent when pacrunner appears. */ pacrunner_send_config (self, config); + + return config; } static void @@ -429,9 +435,9 @@ pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data) self = NM_PACRUNNER_MANAGER (config->manager); if (!ret) - _LOGD ("couldn't remove config for '%s': %s", config->tag, error->message); + _LOGD ("couldn't remove config for '%p': %s", config, error->message); else - _LOGD ("successfully removed config for '%s'", config->tag); + _LOGD ("successfully removed config for '%p'", config); config_unref (config); } @@ -439,49 +445,64 @@ pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data) /** * nm_pacrunner_manager_remove: * @self: the #NMPacrunnerManager - * @iface: the iface for the connection to be removed - * from pacrunner + * @call_id: the call-id obtained from nm_pacrunner_manager_send() */ void -nm_pacrunner_manager_remove (NMPacrunnerManager *self, const char *tag) +nm_pacrunner_manager_remove (NMPacrunnerManager *self, NMPacrunnerCallId *call_id) { - NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); + NMPacrunnerManagerPrivate *priv; + Config *config; GList *list; - g_return_if_fail (tag); - - _LOGT ("removing config for '%s'", tag); - - for (list = g_list_first (priv->configs); list; list = g_list_next (list)) { - Config *config = list->data; - - if (nm_streq (config->tag, tag)) { - if (priv->pacrunner) { - if (!config->path) { - /* send() failed or is still pending. Mark the item as - * removed, so that we ask pacrunner to drop it when the - * send() completes. - */ - config->removed = TRUE; - config_unref (config); - } else { - g_dbus_proxy_call (priv->pacrunner, - "DestroyProxyConfiguration", - g_variant_new ("(o)", config->path), - G_DBUS_CALL_FLAGS_NO_AUTO_START, - -1, - priv->pacrunner_cancellable, - (GAsyncReadyCallback) pacrunner_remove_done, - config); - } - } else - config_unref (config); - priv->configs = g_list_delete_link (priv->configs, list); - return; + g_return_if_fail (NM_IS_PACRUNNER_MANAGER (self)); + g_return_if_fail (call_id); + + config = call_id; + priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); + + _LOGT ("removing config for '%p'", config); + + list = g_list_find (priv->configs, config); + if (!list) + g_return_if_reached (); + + if (priv->pacrunner) { + if (!config->path) { + /* send() failed or is still pending. Mark the item as + * removed, so that we ask pacrunner to drop it when the + * send() completes. + */ + config->removed = TRUE; + config_unref (config); + } else { + g_dbus_proxy_call (priv->pacrunner, + "DestroyProxyConfiguration", + g_variant_new ("(o)", config->path), + G_DBUS_CALL_FLAGS_NO_AUTO_START, + -1, + priv->pacrunner_cancellable, + (GAsyncReadyCallback) pacrunner_remove_done, + config); } - } - /* bug, remove() should always match a previous send() for a given tag */ - g_return_if_reached (); + } else + config_unref (config); + priv->configs = g_list_delete_link (priv->configs, list); +} + +gboolean +nm_pacrunner_manager_remove_clear (NMPacrunnerManager *self, + NMPacrunnerCallId **p_call_id) +{ + g_return_val_if_fail (p_call_id, FALSE); + + /* if we have no call-id, allow for %NULL */ + g_return_val_if_fail ((!self && !*p_call_id) || NM_IS_PACRUNNER_MANAGER (self), FALSE); + + if (!*p_call_id) + return FALSE; + nm_pacrunner_manager_remove (self, + g_steal_pointer (p_call_id)); + return TRUE; } /*****************************************************************************/ diff --git a/src/nm-pacrunner-manager.h b/src/nm-pacrunner-manager.h index 4f6ad15857..3080c4f5fd 100644 --- a/src/nm-pacrunner-manager.h +++ b/src/nm-pacrunner-manager.h @@ -15,7 +15,8 @@ * with this program; if not, write to the Free Software Foundation, Inc., * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. * - * (C) Copyright 2016 Atul Anand . + * Copyright 2016 Atul Anand . + * Copyright 2016 - 2017 Red Hat, Inc. */ #ifndef __NETWORKMANAGER_PACRUNNER_MANAGER_H__ @@ -30,17 +31,22 @@ typedef struct _NMPacrunnerManagerClass NMPacrunnerManagerClass; +typedef struct _NMPacrunnerCallId NMPacrunnerCallId; + GType nm_pacrunner_manager_get_type (void); NMPacrunnerManager *nm_pacrunner_manager_get (void); -void nm_pacrunner_manager_send (NMPacrunnerManager *self, - const char *iface, - const char *tag, - NMProxyConfig *proxy_config, - NMIP4Config *ip4_config, - NMIP6Config *ip6_config); +NMPacrunnerCallId *nm_pacrunner_manager_send (NMPacrunnerManager *self, + const char *iface, + NMProxyConfig *proxy_config, + NMIP4Config *ip4_config, + NMIP6Config *ip6_config); + +void nm_pacrunner_manager_remove (NMPacrunnerManager *self, + NMPacrunnerCallId *call_id); -void nm_pacrunner_manager_remove (NMPacrunnerManager *self, const char *tag); +gboolean nm_pacrunner_manager_remove_clear (NMPacrunnerManager *self, + NMPacrunnerCallId **p_call_id); #endif /* __NETWORKMANAGER_PACRUNNER_MANAGER_H__ */ diff --git a/src/vpn/nm-vpn-connection.c b/src/vpn/nm-vpn-connection.c index dba3546ff0..ecc820685c 100644 --- a/src/vpn/nm-vpn-connection.c +++ b/src/vpn/nm-vpn-connection.c @@ -128,7 +128,8 @@ typedef struct { GVariant *connect_hash; guint connect_timeout; NMProxyConfig *proxy_config; - gboolean proxy_config_sent; + NMPacrunnerManager *pacrunner_manager; + NMPacrunnerCallId *pacrunner_call_id; gboolean has_ip4; NMIP4Config *ip4_config; guint32 ip4_internal_gw; @@ -559,13 +560,18 @@ _set_vpn_state (NMVpnConnection *self, NULL); if (priv->proxy_config) { - nm_pacrunner_manager_send (nm_pacrunner_manager_get (), - priv->ip_iface, - nm_connection_get_uuid (applied), - priv->proxy_config, - priv->ip4_config, - priv->ip6_config); - priv->proxy_config_sent = TRUE; + nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, + &priv->pacrunner_call_id); + if (!priv->pacrunner_manager) { + /* the pending call doesn't keep NMPacrunnerManager alive. + * Take a reference to it. */ + priv->pacrunner_manager = g_object_ref (nm_pacrunner_manager_get ()); + } + priv->pacrunner_call_id = nm_pacrunner_manager_send (priv->pacrunner_manager, + priv->ip_iface, + priv->proxy_config, + priv->ip4_config, + priv->ip6_config); } break; case STATE_DEACTIVATING: @@ -596,11 +602,8 @@ _set_vpn_state (NMVpnConnection *self, } } - if (priv->proxy_config_sent) { - nm_pacrunner_manager_remove (nm_pacrunner_manager_get(), - nm_connection_get_uuid (applied)); - priv->proxy_config_sent = FALSE; - } + nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, + &priv->pacrunner_call_id); break; case STATE_FAILED: case STATE_DISCONNECTED: @@ -2649,6 +2652,10 @@ dispose (GObject *object) fw_call_cleanup (self); + nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, + &priv->pacrunner_call_id); + g_clear_object (&priv->pacrunner_manager); + G_OBJECT_CLASS (nm_vpn_connection_parent_class)->dispose (object); } -- cgit v1.2.1