From 759927f99e9927b5564cfbba8465378ce2426e9e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Apr 2019 21:47:55 +0200 Subject: [PATCH 01/10] dns: fix meanleak in nm-dns-systemd-resolved's call_done() Fixes: 818023c257ca ('dns/resolved: add systemd-resolved backend') --- src/dns/nm-dns-systemd-resolved.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 4718e74037..64e7bdb864 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -121,7 +121,7 @@ _interface_config_free (InterfaceConfig *config) static void call_done (GObject *source, GAsyncResult *r, gpointer user_data) { - GVariant *v; + gs_unref_variant GVariant *v = NULL; gs_free_error GError *error = NULL; NMDnsSystemdResolved *self = (NMDnsSystemdResolved *) user_data; From 6f663b8f8e943b3330a9be7c31e3bbf98a99d149 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Apr 2019 10:07:06 +0200 Subject: [PATCH 02/10] dns: log about what NMDnsSystemdResolved is doing --- src/dns/nm-dns-systemd-resolved.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 64e7bdb864..fddd7603f2 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -129,7 +129,7 @@ call_done (GObject *source, GAsyncResult *r, gpointer user_data) if (!v) { if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; - _LOGW ("Failed: %s", error->message); + _LOGW ("send-updates failed: %s", error->message); } } @@ -270,8 +270,13 @@ send_updates (NMDnsSystemdResolved *self) nm_clear_g_cancellable (&priv->update_cancellable); - if (!priv->resolve) + if (!priv->resolve) { + _LOGT ("send-updates: D-Bus proxy not ready"); return; + } + + _LOGT ("send-updates: start %lu requests", + c_list_length (&priv->request_queue_lst_head)); priv->update_cancellable = g_cancellable_new (); @@ -375,11 +380,13 @@ resolved_proxy_created (GObject *source, GAsyncResult *r, gpointer user_data) priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); g_clear_object (&priv->init_cancellable); if (!resolve) { - _LOGW ("failed to connect to resolved via DBus: %s", error->message); + _LOGW ("failure to create D-Bus proxy for systemd-resolved: %s", error->message); g_signal_emit_by_name (self, NM_DNS_PLUGIN_FAILED); return; } + _LOGT ("D-Bus proxy for systemd-resolved created"); + priv->resolve = resolve; send_updates (self); } From 308e9e69fa821d2de072981c262523b292dadefc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Apr 2019 10:23:50 +0200 Subject: [PATCH 03/10] dns: ratelimit warnings about failure to send updates to NMDnsSystemdResolved As we frequently send updates to systemd-resolved and for each update send multiple messages, it can happen that we log a large number of warnings if they all fail. Rate limit the warnings to only warn once (until the failure is recovered). Currently, if systemd-resolved is not installed (or disabled) we already fail once to create the D-Bus proxy (and never retry). That should be fixed, to create the proxy with G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START_AT_CONSTRUCTION. If we allow creating the proxy we would repeatedly try to send messages and they would all fail. This is one example, where we need to ratelimit the warning. --- src/dns/nm-dns-systemd-resolved.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index fddd7603f2..2da58e11e9 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -66,6 +66,7 @@ typedef struct { GCancellable *init_cancellable; GCancellable *update_cancellable; CList request_queue_lst_head; + bool send_updates_warn_ratelimited:1; } NMDnsSystemdResolvedPrivate; struct _NMDnsSystemdResolved { @@ -124,13 +125,23 @@ call_done (GObject *source, GAsyncResult *r, gpointer user_data) gs_unref_variant GVariant *v = NULL; gs_free_error GError *error = NULL; NMDnsSystemdResolved *self = (NMDnsSystemdResolved *) user_data; + NMDnsSystemdResolvedPrivate *priv; v = g_dbus_proxy_call_finish (G_DBUS_PROXY (source), r, &error); + if ( !v + && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + return; + + priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); + if (!v) { - if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) - return; - _LOGW ("send-updates failed: %s", error->message); - } + if (!priv->send_updates_warn_ratelimited) { + priv->send_updates_warn_ratelimited = TRUE; + _LOGW ("send-updates failed to update systemd-resolved: %s", error->message); + } else + _LOGD ("send-updates failed: %s", error->message); + } else + priv->send_updates_warn_ratelimited = FALSE; } static void From 7ae434b37c9d886807acede55c675b6c82ad6db3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Apr 2019 10:19:32 +0200 Subject: [PATCH 04/10] dns: only update systemd-resolved when it exists Previously, we would create the D-Bus proxy without %G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START_AT_CONSTRUCTION flag. That means, when systemd-resolved was not available or masked, the creation of the D-Bus proxy would fail with dns-sd-resolved[0x561905dc92d0]: failure to create D-Bus proxy for systemd-resolved: Error calling StartServiceByName for org.freedesktop.resolve1: GDBus.Error:org.freedesktop.systemd1.NoSuchUnit: Unit dbus-org.freedesktop.resolve1.service not found. and never retried. Now, when creating the D-Bus proxy don't autostart the instance. Instead, each D-Bus call will try to poke and start the service. There is a problem however: if systemd-resolved is not available, then we must not constantly trying to start it, because it results in a slur or syslog messages from dbus-daemon: dbus-daemon[991]: [system] Activating via systemd: service name='org.freedesktop.resolve1' unit='dbus-org.freedesktop.resolve1.service' requested by ':1.23' (uid=0 pid=1012 comm="/usr/bin/NetworkManager --no-daemon ") dbus-daemon[991]: [system] Activation via systemd failed for unit 'dbus-org.freedesktop.resolve1.service': Unit dbus-org.freedesktop.resolve1.service not found. dbus-daemon[991]: [system] Activating via systemd: service name='org.freedesktop.resolve1' unit='dbus-org.freedesktop.resolve1.service' requested by ':1.23' (uid=0 pid=1012 comm="/usr/bin/NetworkManager --no-daemon ") Avoid that by watching the name owner. But, since systemd-resolved is D-Bus activated, watching the name owner alone is not enough to know whether we should try to autostart the service. Instead: - if we have a name owner, assume the service runs and we send the update - if we have no name owner, and we did not recently try to start the service by name, poke it via "StartServiceByName". The idea is, that in total we only try this once and remember a previous attempt in priv->try_start_blocked. - if we get a name-owner, priv->try_start_blocked gets reset. Either it was us who started the service, or somebody else. Either way, we are good to send updates again. The nice thing is that we only try once to start resolved and only generate one logging message from dbus-daemon about failure to do so. But still, after blocking start on failure, when somebody else starts resolved, we notice it and start using it again. --- src/dns/nm-dns-systemd-resolved.c | 108 +++++++++++++++++++++++++----- 1 file changed, 93 insertions(+), 15 deletions(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 2da58e11e9..140776a598 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -42,6 +42,7 @@ #include "nm-setting-connection.h" #include "devices/nm-device.h" #include "NetworkManagerUtils.h" +#include "nm-dbus-compat.h" #define SYSTEMD_RESOLVED_DBUS_SERVICE "org.freedesktop.resolve1" #define SYSTEMD_RESOLVED_DBUS_PATH "/org/freedesktop/resolve1" @@ -67,6 +68,8 @@ typedef struct { GCancellable *update_cancellable; CList request_queue_lst_head; bool send_updates_warn_ratelimited:1; + bool try_start_blocked:1; + bool dbus_has_owner:1; } NMDnsSystemdResolvedPrivate; struct _NMDnsSystemdResolved { @@ -185,12 +188,11 @@ static void free_pending_updates (NMDnsSystemdResolved *self) { NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); - RequestItem *request_item, *request_item_safe; + RequestItem *request_item; - c_list_for_each_entry_safe (request_item, - request_item_safe, - &priv->request_queue_lst_head, - request_queue_lst) + while ((request_item = c_list_first_entry (&priv->request_queue_lst_head, + RequestItem, + request_queue_lst))) _request_item_free (request_item); } @@ -277,24 +279,62 @@ static void send_updates (NMDnsSystemdResolved *self) { NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); - RequestItem *request_item, *request_item_safe; + RequestItem *request_item; - nm_clear_g_cancellable (&priv->update_cancellable); + if (c_list_is_empty (&priv->request_queue_lst_head)) { + /* nothing to do. */ + return; + } if (!priv->resolve) { _LOGT ("send-updates: D-Bus proxy not ready"); return; } + if (!priv->dbus_has_owner) { + if (priv->try_start_blocked) { + /* we have no name owner and we already tried poking the service to + * autostart. */ + _LOGT ("send-updates: no name owner"); + return; + } + + _LOGT ("send-updates: no name owner. Try start service..."); + priv->try_start_blocked = TRUE; + + g_dbus_connection_call (g_dbus_proxy_get_connection (priv->resolve), + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "StartServiceByName", + g_variant_new ("(su)", SYSTEMD_RESOLVED_DBUS_SERVICE, 0u), + G_VARIANT_TYPE ("(u)"), + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + NULL, + NULL); + return; + } + _LOGT ("send-updates: start %lu requests", c_list_length (&priv->request_queue_lst_head)); + nm_clear_g_cancellable (&priv->update_cancellable); + priv->update_cancellable = g_cancellable_new (); - c_list_for_each_entry_safe (request_item, - request_item_safe, - &priv->request_queue_lst_head, - request_queue_lst) { + while ((request_item = c_list_first_entry (&priv->request_queue_lst_head, + RequestItem, + request_queue_lst))) { + /* Above we explicitly call "StartServiceByName" trying to avoid D-Bus activating systmd-resolved + * multiple times. There is still a race, were we might hit this line although actually + * the service just quit this very moment. In that case, we would try to D-Bus activate the + * service multiple times during each call (something we wanted to avoid). + * + * But this is hard to avoid, because we'd have to check the error failure to detect the reason + * and retry. The race is not critical, because at worst it results in logging a warning + * about failure to start systemd.resolved. */ g_dbus_proxy_call (priv->resolve, request_item->operation, request_item->argument, @@ -375,6 +415,34 @@ get_name (NMDnsPlugin *plugin) /*****************************************************************************/ +static void +name_owner_changed (NMDnsSystemdResolved *self) +{ + NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); + gs_free char *owner = NULL; + + owner = g_dbus_proxy_get_name_owner (priv->resolve); + + if (!owner) + _LOGT ("D-Bus name for systemd-resolved has no owner"); + else + _LOGT ("D-Bus name for systemd-resolved has owner %s", owner); + + priv->dbus_has_owner = !!owner; + if (owner) + priv->try_start_blocked = FALSE; + + send_updates (self); +} + +static void +name_owner_changed_cb (GObject *object, + GParamSpec *pspec, + gpointer user_data) +{ + name_owner_changed (user_data); +} + static void resolved_proxy_created (GObject *source, GAsyncResult *r, gpointer user_data) { @@ -398,8 +466,12 @@ resolved_proxy_created (GObject *source, GAsyncResult *r, gpointer user_data) _LOGT ("D-Bus proxy for systemd-resolved created"); + g_signal_connect (resolve, "notify::g-name-owner", + G_CALLBACK (name_owner_changed_cb), self); + priv->resolve = resolve; - send_updates (self); + + name_owner_changed (self); } /*****************************************************************************/ @@ -413,8 +485,9 @@ nm_dns_systemd_resolved_init (NMDnsSystemdResolved *self) priv->init_cancellable = g_cancellable_new (); g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES | - G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES + | G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS + | G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START_AT_CONSTRUCTION, NULL, SYSTEMD_RESOLVED_DBUS_SERVICE, SYSTEMD_RESOLVED_DBUS_PATH, @@ -437,7 +510,12 @@ dispose (GObject *object) NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); free_pending_updates (self); - g_clear_object (&priv->resolve); + + if (priv->resolve) { + g_signal_handlers_disconnect_by_func (priv->resolve, name_owner_changed_cb, self); + g_clear_object (&priv->resolve); + } + nm_clear_g_cancellable (&priv->init_cancellable); nm_clear_g_cancellable (&priv->update_cancellable); From 2ab90719a213a3373546469c4c89907a4bf09f16 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Apr 2019 11:34:30 +0200 Subject: [PATCH 05/10] connectivity: avoid D-Bus activating systemd-resolved when we know it's not used Every (failed) attempt to D-Bus activate a service results in log-messages from dbus-daemon. It must be avoided to spam the logs that way. Let connectivity check not only ask whether systemd-resolved is enabled (and NetworkManager would like to push information there), but also whether it looks like the service is actually available. That is, either it has a name-owner or it's not blocked from starting. The previous workaround was to configure main.systemd-resolved=no in NetworkManager.conf. But that requires explict configuration. --- src/dns/nm-dns-manager.c | 11 +++++++++-- src/dns/nm-dns-systemd-resolved.c | 16 ++++++++++++++++ src/dns/nm-dns-systemd-resolved.h | 2 ++ src/nm-connectivity.c | 13 ++++++++++++- 4 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c index c7c561c459..27c3e7109c 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -347,13 +347,20 @@ gboolean nm_dns_manager_has_systemd_resolved (NMDnsManager *self) { NMDnsManagerPrivate *priv; + NMDnsSystemdResolved *plugin = NULL; g_return_val_if_fail (NM_IS_DNS_MANAGER (self), FALSE); priv = NM_DNS_MANAGER_GET_PRIVATE (self); - return priv->sd_resolve_plugin - || NM_IS_DNS_SYSTEMD_RESOLVED (priv->plugin); + if (priv->sd_resolve_plugin) { + nm_assert (!NM_IS_DNS_SYSTEMD_RESOLVED (priv->plugin)); + plugin = NM_DNS_SYSTEMD_RESOLVED (priv->sd_resolve_plugin); + } else if (NM_IS_DNS_SYSTEMD_RESOLVED (priv->plugin)) + plugin = NM_DNS_SYSTEMD_RESOLVED (priv->plugin); + + return plugin + && nm_dns_systemd_resolved_is_running (plugin); } /*****************************************************************************/ diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 140776a598..a1cf4f89df 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -476,6 +476,22 @@ resolved_proxy_created (GObject *source, GAsyncResult *r, gpointer user_data) /*****************************************************************************/ +gboolean +nm_dns_systemd_resolved_is_running (NMDnsSystemdResolved *self) +{ + NMDnsSystemdResolvedPrivate *priv; + + g_return_val_if_fail (NM_IS_DNS_SYSTEMD_RESOLVED (self), FALSE); + + priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); + + return priv->resolve + && ( priv->dbus_has_owner + || !priv->try_start_blocked); +} + +/*****************************************************************************/ + static void nm_dns_systemd_resolved_init (NMDnsSystemdResolved *self) { diff --git a/src/dns/nm-dns-systemd-resolved.h b/src/dns/nm-dns-systemd-resolved.h index 800a60c154..b79ff5e4ec 100644 --- a/src/dns/nm-dns-systemd-resolved.h +++ b/src/dns/nm-dns-systemd-resolved.h @@ -36,4 +36,6 @@ GType nm_dns_systemd_resolved_get_type (void); NMDnsPlugin *nm_dns_systemd_resolved_new (void); +gboolean nm_dns_systemd_resolved_is_running (NMDnsSystemdResolved *self); + #endif /* __NETWORKMANAGER_DNS_SYSTEMD_RESOLVED_H__ */ diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index b72413d2f3..2aab23862b 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -840,7 +840,18 @@ nm_connectivity_check_start (NMConnectivity *self, * * Yes, this makes NMConnectivity singleton dependent on NMDnsManager singleton. * Well, not really: it makes connectivity-check-start dependent on NMDnsManager - * which merely means, not to start a connectivity check, late during shutdown. */ + * which merely means, not to start a connectivity check, late during shutdown. + * + * NMDnsSystemdResolved tries to D-Bus activate systemd-resolved only once, + * to not spam syslog with failures messages from dbus-daemon. + * Note that unless NMDnsSystemdResolved tried and failed to start systemd-resolved, + * it guesses that systemd-resolved is activatable and returns %TRUE here. That + * means, while NMDnsSystemdResolved would not try to D-Bus activate systemd-resolved + * more than once, NMConnectivity might -- until NMDnsSystemdResolved tried itself + * and noticed that systemd-resolved is not available. + * This is relatively cumbersome to avoid, because we would have to go through + * NMDnsSystemdResolved trying to asynchronously start the service, to ensure there + * is only one attempt to start the service. */ has_systemd_resolved = nm_dns_manager_has_systemd_resolved (nm_dns_manager_get ()); if (has_systemd_resolved) { From b3a76da96dd5ddd5d8e5c9ff8157f0eab64c3f4f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Apr 2019 16:24:50 +0200 Subject: [PATCH 06/10] dbus/trivial: rename field for D-Bus connection/proxy in NMDBusManagerPrivate The terms "connection" and "proxy" are used all over the place. Rename the fields, to give them a more unique name. --- src/nm-dbus-manager.c | 49 ++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/src/nm-dbus-manager.c b/src/nm-dbus-manager.c index 00fa6617b5..c6f6dde710 100644 --- a/src/nm-dbus-manager.c +++ b/src/nm-dbus-manager.c @@ -78,8 +78,9 @@ typedef struct { NMDBusManagerSetPropertyHandler set_property_handler; gpointer set_property_handler_data; - GDBusConnection *connection; - GDBusProxy *proxy; + GDBusConnection *main_dbus_connection; + GDBusProxy *main_dbus_proxy; + guint objmgr_registration_id; bool started:1; bool shutting_down:1; @@ -450,7 +451,7 @@ _bus_get_unix_pid (NMDBusManager *self, guint32 unix_pid = G_MAXUINT32; gs_unref_variant GVariant *ret = NULL; - ret = _nm_dbus_proxy_call_sync (NM_DBUS_MANAGER_GET_PRIVATE (self)->proxy, + ret = _nm_dbus_proxy_call_sync (NM_DBUS_MANAGER_GET_PRIVATE (self)->main_dbus_proxy, "GetConnectionUnixProcessID", g_variant_new ("(s)", sender), G_VARIANT_TYPE ("(u)"), @@ -474,7 +475,7 @@ _bus_get_unix_user (NMDBusManager *self, guint32 unix_uid = G_MAXUINT32; gs_unref_variant GVariant *ret = NULL; - ret = _nm_dbus_proxy_call_sync (NM_DBUS_MANAGER_GET_PRIVATE (self)->proxy, + ret = _nm_dbus_proxy_call_sync (NM_DBUS_MANAGER_GET_PRIVATE (self)->main_dbus_proxy, "GetConnectionUnixUser", g_variant_new ("(s)", sender), G_VARIANT_TYPE ("(u)"), @@ -762,7 +763,7 @@ nm_dbus_manager_get_connection (NMDBusManager *self) { g_return_val_if_fail (NM_IS_DBUS_MANAGER (self), NULL); - return NM_DBUS_MANAGER_GET_PRIVATE (self)->connection; + return NM_DBUS_MANAGER_GET_PRIVATE (self)->main_dbus_connection; } /*****************************************************************************/ @@ -943,7 +944,7 @@ _obj_register (NMDBusManager *self, GVariantBuilder builder; nm_assert (c_list_is_empty (&obj->internal.registration_lst_head)); - nm_assert (priv->connection); + nm_assert (priv->main_dbus_connection); nm_assert (priv->started); n_klasses = 0; @@ -980,7 +981,7 @@ _obj_register (NMDBusManager *self, reg_data = g_malloc0 (sizeof (RegistrationData) + (sizeof (PropertyCacheData) * prop_len)); - registration_id = g_dbus_connection_register_object (priv->connection, + registration_id = g_dbus_connection_register_object (priv->main_dbus_connection, obj->internal.path, NM_UNCONST_PTR (GDBusInterfaceInfo, &interface_info->parent), &dbus_vtable, @@ -1019,7 +1020,7 @@ _obj_register (NMDBusManager *self, * * In general, it's ok to export an object with frozen signals. But you better make sure * that all properties are in a self-consistent state when exporting the object. */ - g_dbus_connection_emit_signal (priv->connection, + g_dbus_connection_emit_signal (priv->main_dbus_connection, NULL, OBJECT_MANAGER_SERVER_BASE_PATH, interface_info_objmgr.name, @@ -1040,7 +1041,7 @@ _obj_unregister (NMDBusManager *self, nm_assert (NM_IS_DBUS_OBJECT (obj)); - if (!priv->connection) { + if (!priv->main_dbus_connection) { /* nothing to do for the moment. */ nm_assert (c_list_is_empty (&obj->internal.registration_lst_head)); return; @@ -1059,7 +1060,7 @@ _obj_unregister (NMDBusManager *self, "s", interface_info->parent.name); c_list_unlink_stale (®_data->registration_lst); - if (!g_dbus_connection_unregister_object (priv->connection, reg_data->registration_id)) + if (!g_dbus_connection_unregister_object (priv->main_dbus_connection, reg_data->registration_id)) nm_assert_not_reached (); if (interface_info->parent.properties) { @@ -1071,7 +1072,7 @@ _obj_unregister (NMDBusManager *self, g_free (reg_data); } - g_dbus_connection_emit_signal (priv->connection, + g_dbus_connection_emit_signal (priv->main_dbus_connection, NULL, OBJECT_MANAGER_SERVER_BASE_PATH, interface_info_objmgr.name, @@ -1122,7 +1123,7 @@ _nm_dbus_manager_obj_export (NMDBusObject *obj) nm_assert_not_reached (); c_list_link_tail (&priv->objects_lst_head, &obj->internal.objects_lst); - if (priv->connection && priv->started) + if (priv->main_dbus_connection && priv->started) _obj_register (self, obj); } @@ -1239,7 +1240,7 @@ _nm_dbus_manager_obj_notify (NMDBusObject *obj, } g_variant_builder_init (&invalidated_builder, G_VARIANT_TYPE ("as")); - g_dbus_connection_emit_signal (priv->connection, + g_dbus_connection_emit_signal (priv->main_dbus_connection, NULL, obj->internal.path, "org.freedesktop.DBus.Properties", @@ -1255,7 +1256,7 @@ _nm_dbus_manager_obj_notify (NMDBusObject *obj, /* this is a special interface: it has a legacy PropertiesChanged signal, * however, contrary to other interfaces with ~regular~ legacy signals, * we only notify about properties that actually belong to this interface. */ - g_dbus_connection_emit_signal (priv->connection, + g_dbus_connection_emit_signal (priv->main_dbus_connection, NULL, obj->internal.path, nm_interface_info_device_statistics.parent.name, @@ -1292,7 +1293,7 @@ _nm_dbus_manager_obj_notify (NMDBusObject *obj, const NMDBusInterfaceInfoExtended *interface_info = _reg_data_get_interface_info (reg_data); if (interface_info->legacy_property_changed) { - g_dbus_connection_emit_signal (priv->connection, + g_dbus_connection_emit_signal (priv->main_dbus_connection, NULL, obj->internal.path, interface_info->parent.name, @@ -1321,12 +1322,12 @@ _nm_dbus_manager_obj_emit_signal (NMDBusObject *obj, self = obj->internal.bus_manager; priv = NM_DBUS_MANAGER_GET_PRIVATE (self); - if (!priv->connection || !priv->started) { + if (!priv->main_dbus_connection || !priv->started) { nm_g_variant_unref_floating (args); return; } - g_dbus_connection_emit_signal (priv->connection, + g_dbus_connection_emit_signal (priv->main_dbus_connection, NULL, obj->internal.path, interface_info->parent.name, @@ -1473,7 +1474,7 @@ nm_dbus_manager_get_dbus_connection (NMDBusManager *self) { g_return_val_if_fail (NM_IS_DBUS_MANAGER (self), NULL); - return NM_DBUS_MANAGER_GET_PRIVATE (self)->connection; + return NM_DBUS_MANAGER_GET_PRIVATE (self)->main_dbus_connection; } void @@ -1487,7 +1488,7 @@ nm_dbus_manager_start (NMDBusManager *self, g_return_if_fail (NM_IS_DBUS_MANAGER (self)); priv = NM_DBUS_MANAGER_GET_PRIVATE (self); - if (!priv->connection) { + if (!priv->main_dbus_connection) { /* Do nothing. We're presumably in the configure-and-quit mode. */ return; } @@ -1581,8 +1582,8 @@ nm_dbus_manager_acquire_bus (NMDBusManager *self) } priv->objmgr_registration_id = registration_id; - priv->connection = g_steal_pointer (&connection); - priv->proxy = g_steal_pointer (&proxy); + priv->main_dbus_connection = g_steal_pointer (&connection); + priv->main_dbus_proxy = g_steal_pointer (&proxy); _LOGI ("acquired D-Bus service \"%s\"", NM_DBUS_SERVICE); @@ -1640,12 +1641,12 @@ dispose (GObject *object) private_server_free (s); if (priv->objmgr_registration_id) { - g_dbus_connection_unregister_object (priv->connection, + g_dbus_connection_unregister_object (priv->main_dbus_connection, nm_steal_int (&priv->objmgr_registration_id)); } - g_clear_object (&priv->proxy); - g_clear_object (&priv->connection); + g_clear_object (&priv->main_dbus_proxy); + g_clear_object (&priv->main_dbus_connection); G_OBJECT_CLASS (nm_dbus_manager_parent_class)->dispose (object); } From 4058b01c423659c046e713b7b82c8973e3c0d4f4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Apr 2019 16:27:45 +0200 Subject: [PATCH 07/10] dbus: remove unused function nm_dbus_manager_get_connection() nm_dbus_manager_get_connection() was unused. Also, we already have nm_dbus_manager_get_dbus_connection() which does the same (and is used). --- src/nm-dbus-manager.c | 10 ---------- src/nm-dbus-manager.h | 2 -- 2 files changed, 12 deletions(-) diff --git a/src/nm-dbus-manager.c b/src/nm-dbus-manager.c index c6f6dde710..6e78645557 100644 --- a/src/nm-dbus-manager.c +++ b/src/nm-dbus-manager.c @@ -758,16 +758,6 @@ nm_dbus_manager_new_proxy (NMDBusManager *self, /*****************************************************************************/ -GDBusConnection * -nm_dbus_manager_get_connection (NMDBusManager *self) -{ - g_return_val_if_fail (NM_IS_DBUS_MANAGER (self), NULL); - - return NM_DBUS_MANAGER_GET_PRIVATE (self)->main_dbus_connection; -} - -/*****************************************************************************/ - static const NMDBusInterfaceInfoExtended * _reg_data_get_interface_info (RegistrationData *reg_data) { diff --git a/src/nm-dbus-manager.h b/src/nm-dbus-manager.h index 89acd7c855..330a689314 100644 --- a/src/nm-dbus-manager.h +++ b/src/nm-dbus-manager.h @@ -61,8 +61,6 @@ void nm_dbus_manager_stop (NMDBusManager *self); gboolean nm_dbus_manager_is_stopping (NMDBusManager *self); -GDBusConnection *nm_dbus_manager_get_connection (NMDBusManager *self); - gpointer nm_dbus_manager_lookup_object (NMDBusManager *self, const char *path); void _nm_dbus_manager_obj_export (NMDBusObject *obj); From 5e77b2d6604245b5b63891e9e02a320df7e4b93a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Apr 2019 16:32:26 +0200 Subject: [PATCH 08/10] dbus: don't use GDBusProxy in NMDBusManager Unnecessary overhead that simplifies nothing. --- src/nm-dbus-manager.c | 79 ++++++++++++++++++++++--------------------- 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/src/nm-dbus-manager.c b/src/nm-dbus-manager.c index 6e78645557..0b689f9365 100644 --- a/src/nm-dbus-manager.c +++ b/src/nm-dbus-manager.c @@ -79,7 +79,6 @@ typedef struct { gpointer set_property_handler_data; GDBusConnection *main_dbus_connection; - GDBusProxy *main_dbus_proxy; guint objmgr_registration_id; bool started:1; @@ -448,15 +447,21 @@ _bus_get_unix_pid (NMDBusManager *self, gulong *out_pid, GError **error) { + NMDBusManagerPrivate *priv = NM_DBUS_MANAGER_GET_PRIVATE (self); guint32 unix_pid = G_MAXUINT32; gs_unref_variant GVariant *ret = NULL; - ret = _nm_dbus_proxy_call_sync (NM_DBUS_MANAGER_GET_PRIVATE (self)->main_dbus_proxy, - "GetConnectionUnixProcessID", - g_variant_new ("(s)", sender), - G_VARIANT_TYPE ("(u)"), - G_DBUS_CALL_FLAGS_NONE, 2000, - NULL, error); + ret = g_dbus_connection_call_sync (priv->main_dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "GetConnectionUnixProcessID", + g_variant_new ("(s)", sender), + G_VARIANT_TYPE ("(u)"), + G_DBUS_CALL_FLAGS_NONE, + 2000, + NULL, + error); if (!ret) return FALSE; @@ -472,15 +477,21 @@ _bus_get_unix_user (NMDBusManager *self, gulong *out_user, GError **error) { + NMDBusManagerPrivate *priv = NM_DBUS_MANAGER_GET_PRIVATE (self); guint32 unix_uid = G_MAXUINT32; gs_unref_variant GVariant *ret = NULL; - ret = _nm_dbus_proxy_call_sync (NM_DBUS_MANAGER_GET_PRIVATE (self)->main_dbus_proxy, - "GetConnectionUnixUser", - g_variant_new ("(s)", sender), - G_VARIANT_TYPE ("(u)"), - G_DBUS_CALL_FLAGS_NONE, 2000, - NULL, error); + ret = g_dbus_connection_call_sync (priv->main_dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "GetConnectionUnixUser", + g_variant_new ("(s)", sender), + G_VARIANT_TYPE ("(u)"), + G_DBUS_CALL_FLAGS_NONE, + 2000, + NULL, + error); if (!ret) return FALSE; @@ -1498,7 +1509,6 @@ nm_dbus_manager_acquire_bus (NMDBusManager *self) gs_free_error GError *error = NULL; gs_unref_variant GVariant *ret = NULL; gs_unref_object GDBusConnection *connection = NULL; - gs_unref_object GDBusProxy *proxy = NULL; guint32 result; guint registration_id; @@ -1521,20 +1531,6 @@ nm_dbus_manager_acquire_bus (NMDBusManager *self) g_dbus_connection_set_exit_on_close (connection, FALSE); - proxy = g_dbus_proxy_new_sync (connection, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES - | G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS, - NULL, - DBUS_SERVICE_DBUS, - DBUS_PATH_DBUS, - DBUS_INTERFACE_DBUS, - NULL, - &error); - if (!proxy) { - _LOGE ("fatal failure to initialize D-Bus: %s", error->message); - return FALSE; - } - registration_id = g_dbus_connection_register_object (connection, OBJECT_MANAGER_SERVER_BASE_PATH, NM_UNCONST_PTR (GDBusInterfaceInfo, &interface_info_objmgr), @@ -1547,15 +1543,22 @@ nm_dbus_manager_acquire_bus (NMDBusManager *self) return FALSE; } - ret = _nm_dbus_proxy_call_sync (proxy, - "RequestName", - g_variant_new ("(su)", - NM_DBUS_SERVICE, - DBUS_NAME_FLAG_DO_NOT_QUEUE), - G_VARIANT_TYPE ("(u)"), - G_DBUS_CALL_FLAGS_NONE, -1, - NULL, - &error); + ret = g_dbus_connection_call_sync (connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "RequestName", + g_variant_new ("(su)", + NM_DBUS_SERVICE, + DBUS_NAME_FLAG_DO_NOT_QUEUE), + G_VARIANT_TYPE ("(u)"), + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + &error); + if (!ret) + return FALSE; + if (!ret) { _LOGE ("fatal failure to acquire D-Bus service \"%s"": %s", NM_DBUS_SERVICE, error->message); @@ -1573,7 +1576,6 @@ nm_dbus_manager_acquire_bus (NMDBusManager *self) priv->objmgr_registration_id = registration_id; priv->main_dbus_connection = g_steal_pointer (&connection); - priv->main_dbus_proxy = g_steal_pointer (&proxy); _LOGI ("acquired D-Bus service \"%s\"", NM_DBUS_SERVICE); @@ -1635,7 +1637,6 @@ dispose (GObject *object) nm_steal_int (&priv->objmgr_registration_id)); } - g_clear_object (&priv->main_dbus_proxy); g_clear_object (&priv->main_dbus_connection); G_OBJECT_CLASS (nm_dbus_manager_parent_class)->dispose (object); From 5d86f605263200667f2c848f89f3b8831f20bb65 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Apr 2019 21:45:44 +0200 Subject: [PATCH 09/10] dns: use GDBusConnection instead of GDBusProxy in "nm-dns-systemd-resolved.c" The proxy does nothing for us, except overhead. We can directly subscribe to "NameOwnerChanged" signals on the GDBusConnection. Also, instead of asynchronously creating the GDBusProxy, asynchronously call "GetNameOwner". That's what the proxy does anyway. GDBusConnection is actually a decent API. We don't need another layer on top of that, for functionality that we don't use. Also, don't use G_BUS_TYPE_SYSTEM, but use the GDBusConnection that also the bus-manager uses. For all practical purposes, that is the connection was want to use also in NMDnsSystemdResolved. --- src/dns/nm-dns-systemd-resolved.c | 166 +++++++++++++++++++----------- 1 file changed, 106 insertions(+), 60 deletions(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index a1cf4f89df..573f047b47 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -44,8 +44,9 @@ #include "NetworkManagerUtils.h" #include "nm-dbus-compat.h" -#define SYSTEMD_RESOLVED_DBUS_SERVICE "org.freedesktop.resolve1" -#define SYSTEMD_RESOLVED_DBUS_PATH "/org/freedesktop/resolve1" +#define SYSTEMD_RESOLVED_DBUS_SERVICE "org.freedesktop.resolve1" +#define SYSTEMD_RESOLVED_MANAGER_IFACE "org.freedesktop.resolve1.Manager" +#define SYSTEMD_RESOLVED_DBUS_PATH "/org/freedesktop/resolve1" /*****************************************************************************/ @@ -63,13 +64,14 @@ typedef struct { /*****************************************************************************/ typedef struct { - GDBusProxy *resolve; - GCancellable *init_cancellable; - GCancellable *update_cancellable; + GDBusConnection *dbus_connection; + GCancellable *cancellable; CList request_queue_lst_head; + guint name_owner_changed_id; bool send_updates_warn_ratelimited:1; bool try_start_blocked:1; bool dbus_has_owner:1; + bool dbus_initied:1; } NMDnsSystemdResolvedPrivate; struct _NMDnsSystemdResolved { @@ -130,7 +132,7 @@ call_done (GObject *source, GAsyncResult *r, gpointer user_data) NMDnsSystemdResolved *self = (NMDnsSystemdResolved *) user_data; NMDnsSystemdResolvedPrivate *priv; - v = g_dbus_proxy_call_finish (G_DBUS_PROXY (source), r, &error); + v = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), r, &error); if ( !v && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; @@ -286,8 +288,8 @@ send_updates (NMDnsSystemdResolved *self) return; } - if (!priv->resolve) { - _LOGT ("send-updates: D-Bus proxy not ready"); + if (!priv->dbus_initied) { + _LOGT ("send-updates: D-Bus connection not ready"); return; } @@ -302,7 +304,7 @@ send_updates (NMDnsSystemdResolved *self) _LOGT ("send-updates: no name owner. Try start service..."); priv->try_start_blocked = TRUE; - g_dbus_connection_call (g_dbus_proxy_get_connection (priv->resolve), + g_dbus_connection_call (priv->dbus_connection, DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, @@ -320,9 +322,9 @@ send_updates (NMDnsSystemdResolved *self) _LOGT ("send-updates: start %lu requests", c_list_length (&priv->request_queue_lst_head)); - nm_clear_g_cancellable (&priv->update_cancellable); + nm_clear_g_cancellable (&priv->cancellable); - priv->update_cancellable = g_cancellable_new (); + priv->cancellable = g_cancellable_new (); while ((request_item = c_list_first_entry (&priv->request_queue_lst_head, RequestItem, @@ -335,14 +337,18 @@ send_updates (NMDnsSystemdResolved *self) * But this is hard to avoid, because we'd have to check the error failure to detect the reason * and retry. The race is not critical, because at worst it results in logging a warning * about failure to start systemd.resolved. */ - g_dbus_proxy_call (priv->resolve, - request_item->operation, - request_item->argument, - G_DBUS_CALL_FLAGS_NONE, - -1, - priv->update_cancellable, - call_done, - self); + g_dbus_connection_call (priv->dbus_connection, + SYSTEMD_RESOLVED_DBUS_SERVICE, + SYSTEMD_RESOLVED_DBUS_PATH, + SYSTEMD_RESOLVED_MANAGER_IFACE, + request_item->operation, + request_item->argument, + NULL, + G_DBUS_CALL_FLAGS_NONE, + -1, + priv->cancellable, + call_done, + self); _request_item_free (request_item); } } @@ -416,12 +422,12 @@ get_name (NMDnsPlugin *plugin) /*****************************************************************************/ static void -name_owner_changed (NMDnsSystemdResolved *self) +name_owner_changed (NMDnsSystemdResolved *self, + const char *owner) { NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); - gs_free char *owner = NULL; - owner = g_dbus_proxy_get_name_owner (priv->resolve); + owner = nm_str_not_empty (owner); if (!owner) _LOGT ("D-Bus name for systemd-resolved has no owner"); @@ -436,42 +442,64 @@ name_owner_changed (NMDnsSystemdResolved *self) } static void -name_owner_changed_cb (GObject *object, - GParamSpec *pspec, +name_owner_changed_cb (GDBusConnection *connection, + const char *sender_name, + const char *object_path, + const char *interface_name, + const char *signal_name, + GVariant *parameters, gpointer user_data) { - name_owner_changed (user_data); + NMDnsSystemdResolved *self = user_data; + NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); + const char *new_owner; + + if (!g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(sss)"))) + return; + + g_variant_get (parameters, + "(&s&s&s)", + NULL, + NULL, + &new_owner); + + if (!priv->dbus_initied) { + /* There was a race and we got a NameOwnerChanged signal before GetNameOwner + * returns. */ + priv->dbus_initied = TRUE; + nm_clear_g_cancellable (&priv->cancellable); + } + + name_owner_changed (user_data, new_owner); } static void -resolved_proxy_created (GObject *source, GAsyncResult *r, gpointer user_data) +get_name_owner_cb (GObject *source, + GAsyncResult *res, + gpointer user_data) { - NMDnsSystemdResolved *self = (NMDnsSystemdResolved *) user_data; + NMDnsSystemdResolved *self; NMDnsSystemdResolvedPrivate *priv; + gs_unref_variant GVariant *ret = NULL; gs_free_error GError *error = NULL; - GDBusProxy *resolve; + const char *owner = NULL; - resolve = g_dbus_proxy_new_finish (r, &error); - if ( !resolve + ret = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), res, &error); + if ( !ret && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; + if (ret) + g_variant_get (ret, "(&s)", &owner); + + self = user_data; priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); - g_clear_object (&priv->init_cancellable); - if (!resolve) { - _LOGW ("failure to create D-Bus proxy for systemd-resolved: %s", error->message); - g_signal_emit_by_name (self, NM_DNS_PLUGIN_FAILED); - return; - } - _LOGT ("D-Bus proxy for systemd-resolved created"); + g_clear_object (&priv->cancellable); - g_signal_connect (resolve, "notify::g-name-owner", - G_CALLBACK (name_owner_changed_cb), self); + priv->dbus_initied = TRUE; - priv->resolve = resolve; - - name_owner_changed (self); + name_owner_changed (self, owner); } /*****************************************************************************/ @@ -485,7 +513,7 @@ nm_dns_systemd_resolved_is_running (NMDnsSystemdResolved *self) priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); - return priv->resolve + return priv->dbus_initied && ( priv->dbus_has_owner || !priv->try_start_blocked); } @@ -499,18 +527,35 @@ nm_dns_systemd_resolved_init (NMDnsSystemdResolved *self) c_list_init (&priv->request_queue_lst_head); - priv->init_cancellable = g_cancellable_new (); - g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES - | G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS - | G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START_AT_CONSTRUCTION, - NULL, - SYSTEMD_RESOLVED_DBUS_SERVICE, - SYSTEMD_RESOLVED_DBUS_PATH, - SYSTEMD_RESOLVED_DBUS_SERVICE ".Manager", - priv->init_cancellable, - resolved_proxy_created, - self); + priv->dbus_connection = nm_g_object_ref (nm_dbus_manager_get_dbus_connection (nm_dbus_manager_get ())); + if (!priv->dbus_connection) { + _LOGD ("no D-Bus connection"); + return; + } + + priv->name_owner_changed_id = g_dbus_connection_signal_subscribe (priv->dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_INTERFACE_DBUS, + "NameOwnerChanged", + DBUS_PATH_DBUS, + SYSTEMD_RESOLVED_DBUS_SERVICE, + G_DBUS_SIGNAL_FLAGS_NONE, + name_owner_changed_cb, + self, + NULL); + priv->cancellable = g_cancellable_new (); + g_dbus_connection_call (priv->dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "GetNameOwner", + g_variant_new ("(s)", SYSTEMD_RESOLVED_DBUS_SERVICE), + G_VARIANT_TYPE ("(s)"), + G_DBUS_CALL_FLAGS_NONE, + -1, + priv->cancellable, + get_name_owner_cb, + self); } NMDnsPlugin * @@ -527,13 +572,14 @@ dispose (GObject *object) free_pending_updates (self); - if (priv->resolve) { - g_signal_handlers_disconnect_by_func (priv->resolve, name_owner_changed_cb, self); - g_clear_object (&priv->resolve); + if (priv->name_owner_changed_id != 0) { + g_dbus_connection_signal_unsubscribe (priv->dbus_connection, + nm_steal_int (&priv->name_owner_changed_id)); } - nm_clear_g_cancellable (&priv->init_cancellable); - nm_clear_g_cancellable (&priv->update_cancellable); + nm_clear_g_cancellable (&priv->cancellable); + + g_clear_object (&priv->dbus_connection); G_OBJECT_CLASS (nm_dns_systemd_resolved_parent_class)->dispose (object); } From e04dc445ec00f8e82bbc1b9ef614b50c906d6606 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Apr 2019 11:57:55 +0200 Subject: [PATCH 10/10] dbus: cache GetConnectionUnixProcessID and GetConnectionUnixUser We call GetConnectionUnixProcessID and GetConnectionUnixUser *a lot*. And we do so synchronously. Both is a problem. To avoid the first problem, cache the last few requests with each cached value being valid for one second. On a quick test, this saves 98% of the requests: 59 GetConnectionUnixProcessID(*) 3201 GetConnectionUnixProcessID(*) (served from cache) 59 GetConnectionUnixUser(*) 3201 GetConnectionUnixUser(*) (served from cache) Note that now as we serve requests from the cache, it might be the case that the D-Bus endpoint already disconnected. Previously, the request would have failed but now we return the cached user-id and process-id. This problem is mitigated by only caching the values for up to one second. Also, it's not really a problem because we cache sender names. Those are supposed to be unique and not repeat. So, even if the peer already disconnected, it is still true that the corresponding PID/UID was as we have cached it. We don't use this API for checking whether the peer is still connected, but what UID/PID it has/had. That answer is still correct for the cached value after the peer disconnected. --- src/nm-auth-subject.c | 7 +- src/nm-dbus-manager.c | 172 +++++++++++++++++++++++++++++++----------- src/nm-dbus-manager.h | 4 +- 3 files changed, 134 insertions(+), 49 deletions(-) diff --git a/src/nm-auth-subject.c b/src/nm-auth-subject.c index dff331a8d8..5599201ef7 100644 --- a/src/nm-auth-subject.c +++ b/src/nm-auth-subject.c @@ -173,9 +173,10 @@ _new_unix_process (GDBusMethodInvocation *context, GDBusMessage *message) { NMAuthSubject *self; - gboolean success = FALSE; - gulong pid = 0, uid = 0; - gs_free char *dbus_sender = NULL; + const char *dbus_sender = NULL; + gulong uid = 0; + gulong pid = 0; + gboolean success; g_return_val_if_fail (context || (connection && message), NULL); diff --git a/src/nm-dbus-manager.c b/src/nm-dbus-manager.c index 0b689f9365..1c4be50d54 100644 --- a/src/nm-dbus-manager.c +++ b/src/nm-dbus-manager.c @@ -28,6 +28,7 @@ #include #include "c-list/src/c-list.h" +#include "nm-utils/nm-c-list.h" #include "nm-dbus-interface.h" #include "nm-core-internal.h" #include "nm-dbus-compat.h" @@ -43,6 +44,17 @@ /*****************************************************************************/ +typedef struct { + CList caller_info_lst; + gulong uid; + gulong pid; + gint64 uid_checked_at; + gint64 pid_checked_at; + bool uid_valid:1; + bool pid_valid:1; + char sender[0]; +} CallerInfo; + typedef struct { GVariant *value; } PropertyCacheData; @@ -80,6 +92,8 @@ typedef struct { GDBusConnection *main_dbus_connection; + CList caller_info_lst_head; + guint objmgr_registration_id; bool started:1; bool shutting_down:1; @@ -441,11 +455,17 @@ private_server_get_connection_by_owner (PrivateServer *s, const char *owner) /*****************************************************************************/ +static void +_caller_info_free (CallerInfo *caller_info) +{ + c_list_unlink_stale (&caller_info->caller_info_lst); + g_free (caller_info); +} + static gboolean _bus_get_unix_pid (NMDBusManager *self, const char *sender, - gulong *out_pid, - GError **error) + gulong *out_pid) { NMDBusManagerPrivate *priv = NM_DBUS_MANAGER_GET_PRIVATE (self); guint32 unix_pid = G_MAXUINT32; @@ -461,7 +481,7 @@ _bus_get_unix_pid (NMDBusManager *self, G_DBUS_CALL_FLAGS_NONE, 2000, NULL, - error); + NULL); if (!ret) return FALSE; @@ -474,8 +494,7 @@ _bus_get_unix_pid (NMDBusManager *self, static gboolean _bus_get_unix_user (NMDBusManager *self, const char *sender, - gulong *out_user, - GError **error) + gulong *out_user) { NMDBusManagerPrivate *priv = NM_DBUS_MANAGER_GET_PRIVATE (self); guint32 unix_uid = G_MAXUINT32; @@ -491,7 +510,7 @@ _bus_get_unix_user (NMDBusManager *self, G_DBUS_CALL_FLAGS_NONE, 2000, NULL, - error); + NULL); if (!ret) return FALSE; @@ -501,34 +520,102 @@ _bus_get_unix_user (NMDBusManager *self, return TRUE; } -/** - * _get_caller_info(): - * - * Given a GDBus method invocation, or a GDBusConnection + GDBusMessage, - * return the sender and the UID of the sender. - */ +static const CallerInfo * +_get_caller_info_ensure (NMDBusManager *self, + const char *sender, + gboolean ensure_uid, + gboolean ensure_pid) +{ + NMDBusManagerPrivate *priv = NM_DBUS_MANAGER_GET_PRIVATE (self); + CallerInfo *caller_info; + CallerInfo *ci; + gint64 now_ns; + gsize num; + +#define CALLER_INFO_MAX_AGE (NM_UTILS_NS_PER_SECOND * 1) + + /* Linear search the cache for the sender. + * + * The number of cached caller-infos is limited. Hence, it's O(1) and + * the list is reasonably short. + * Also, the entire caching assumes that we repeatedly ask for the + * same sender. That means, we expect to find the right caller info + * at the front of the list. */ + num = 1; + caller_info = NULL; + c_list_for_each_entry (ci, &priv->caller_info_lst_head, caller_info_lst) { + if (nm_streq (sender, ci->sender)) { + caller_info = ci; + break; + } + num++; + } + + if (caller_info) + nm_c_list_move_front (&priv->caller_info_lst_head, &caller_info->caller_info_lst); + else { + gsize l = strlen (sender) + 1; + + caller_info = g_malloc (sizeof (CallerInfo) + l); + *caller_info = (CallerInfo) { + .uid_checked_at = - CALLER_INFO_MAX_AGE, + .pid_checked_at = - CALLER_INFO_MAX_AGE, + }; + memcpy (caller_info->sender, sender, l); + c_list_link_front (&priv->caller_info_lst_head, &caller_info->caller_info_lst); + + /* only cache the last few entries. */ + while (TRUE) { + nm_assert (num > 0 && num == c_list_length (&priv->caller_info_lst_head)); + if (num-- <= 5) + break; + _caller_info_free (c_list_last_entry (&priv->caller_info_lst_head, CallerInfo, caller_info_lst)); + } + } + + now_ns = nm_utils_get_monotonic_timestamp_ns (); + + if ( ensure_uid + && (now_ns - caller_info->uid_checked_at) > CALLER_INFO_MAX_AGE) { + caller_info->uid_checked_at = now_ns; + if (!(caller_info->uid_valid = _bus_get_unix_user (self, sender, &caller_info->uid))) + caller_info->uid = G_MAXULONG; + } + + if ( ensure_pid + && (now_ns - caller_info->pid_checked_at) > CALLER_INFO_MAX_AGE) { + caller_info->pid_checked_at = now_ns; + if (!(caller_info->pid_valid = _bus_get_unix_pid (self, sender, &caller_info->pid))) + caller_info->pid = G_MAXULONG; + } + + return caller_info; +} + static gboolean _get_caller_info (NMDBusManager *self, GDBusMethodInvocation *context, GDBusConnection *connection, GDBusMessage *message, - char **out_sender, + const char **out_sender, gulong *out_uid, gulong *out_pid) { NMDBusManagerPrivate *priv = NM_DBUS_MANAGER_GET_PRIVATE (self); + const CallerInfo *caller_info; const char *sender; if (context) { + nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); connection = g_dbus_method_invocation_get_connection (context); /* only bus connections will have a sender */ sender = g_dbus_method_invocation_get_sender (context); } else { - g_assert (message); + nm_assert (G_IS_DBUS_MESSAGE (message)); sender = g_dbus_message_get_sender (message); } - g_assert (connection); + nm_assert (G_IS_DBUS_CONNECTION (connection)); if (!sender) { PrivateServer *s; @@ -537,10 +624,8 @@ _get_caller_info (NMDBusManager *self, c_list_for_each_entry (s, &priv->private_servers_lst_head, private_servers_lst) { sender = private_server_get_connection_owner (s, connection); if (sender) { - if (out_uid) - *out_uid = 0; - if (out_sender) - *out_sender = g_strdup (sender); + NM_SET_OUT (out_uid, 0); + NM_SET_OUT (out_sender, sender); if (out_pid) { GCredentials *creds; @@ -559,35 +644,29 @@ _get_caller_info (NMDBusManager *self, return TRUE; } } + NM_SET_OUT (out_sender, NULL); + NM_SET_OUT (out_uid, G_MAXULONG); + NM_SET_OUT (out_pid, G_MAXULONG); return FALSE; } - /* Bus connections always have a sender */ - g_assert (sender); - if (out_uid) { - if (!_bus_get_unix_user (self, sender, out_uid, NULL)) { - *out_uid = G_MAXULONG; - return FALSE; - } - } + caller_info = _get_caller_info_ensure (self, sender, !!out_uid, !!out_pid); - if (out_pid) { - if (!_bus_get_unix_pid (self, sender, out_pid, NULL)) { - *out_pid = G_MAXULONG; - return FALSE; - } - } - - if (out_sender) - *out_sender = g_strdup (sender); + NM_SET_OUT (out_sender, caller_info->sender); + NM_SET_OUT (out_uid, caller_info->uid); + NM_SET_OUT (out_pid, caller_info->pid); + if (out_uid && !caller_info->uid_valid) + return FALSE; + if (out_pid && !caller_info->pid_valid) + return FALSE; return TRUE; } gboolean nm_dbus_manager_get_caller_info (NMDBusManager *self, GDBusMethodInvocation *context, - char **out_sender, + const char **out_sender, gulong *out_uid, gulong *out_pid) { @@ -598,7 +677,7 @@ gboolean nm_dbus_manager_get_caller_info_from_message (NMDBusManager *self, GDBusConnection *connection, GDBusMessage *message, - char **out_sender, + const char **out_sender, gulong *out_uid, gulong *out_pid) { @@ -659,8 +738,8 @@ nm_dbus_manager_get_unix_user (NMDBusManager *self, gulong *out_uid) { NMDBusManagerPrivate *priv = NM_DBUS_MANAGER_GET_PRIVATE (self); + const CallerInfo *caller_info; PrivateServer *s; - GError *error = NULL; g_return_val_if_fail (sender != NULL, FALSE); g_return_val_if_fail (out_uid != NULL, FALSE); @@ -677,13 +756,12 @@ nm_dbus_manager_get_unix_user (NMDBusManager *self, } /* Otherwise, a bus connection */ - if (!_bus_get_unix_user (self, sender, out_uid, &error)) { - _LOGW ("failed to get unix user for dbus sender '%s': %s", - sender, error->message); - g_error_free (error); + caller_info = _get_caller_info_ensure (self, sender, TRUE, FALSE); + *out_uid = caller_info->uid; + if (!caller_info->uid_valid) { + _LOGW ("failed to get unix user for dbus sender '%s'", sender); return FALSE; } - return TRUE; } @@ -1613,6 +1691,8 @@ nm_dbus_manager_init (NMDBusManager *self) c_list_init (&priv->private_servers_lst_head); c_list_init (&priv->objects_lst_head); priv->objects_by_path = g_hash_table_new ((GHashFunc) _objects_by_path_hash, (GEqualFunc) _objects_by_path_equal); + + c_list_init (&priv->caller_info_lst_head); } static void @@ -1621,6 +1701,7 @@ dispose (GObject *object) NMDBusManager *self = NM_DBUS_MANAGER (object); NMDBusManagerPrivate *priv = NM_DBUS_MANAGER_GET_PRIVATE (self); PrivateServer *s, *s_safe; + CallerInfo *caller_info; /* All exported NMDBusObject instances keep the manager alive, so we don't * expect any remaining objects. */ @@ -1640,6 +1721,9 @@ dispose (GObject *object) g_clear_object (&priv->main_dbus_connection); G_OBJECT_CLASS (nm_dbus_manager_parent_class)->dispose (object); + + while ((caller_info = c_list_first_entry (&priv->caller_info_lst_head, CallerInfo, caller_info_lst))) + _caller_info_free (caller_info); } static void diff --git a/src/nm-dbus-manager.h b/src/nm-dbus-manager.h index 330a689314..2de6ff0ba7 100644 --- a/src/nm-dbus-manager.h +++ b/src/nm-dbus-manager.h @@ -75,7 +75,7 @@ void _nm_dbus_manager_obj_emit_signal (NMDBusObject *obj, gboolean nm_dbus_manager_get_caller_info (NMDBusManager *self, GDBusMethodInvocation *context, - char **out_sender, + const char **out_sender, gulong *out_uid, gulong *out_pid); @@ -95,7 +95,7 @@ gboolean nm_dbus_manager_get_unix_user (NMDBusManager *self, gboolean nm_dbus_manager_get_caller_info_from_message (NMDBusManager *self, GDBusConnection *connection, GDBusMessage *message, - char **out_sender, + const char **out_sender, gulong *out_uid, gulong *out_pid);