From 2a26562ec8c1d824dc0f64397b1e1b452baded50 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 27 Apr 2020 08:02:11 +0200 Subject: [PATCH 1/9] shared: add nm_gbytes_hash() and nm_gbytes_equal() --- shared/nm-glib-aux/nm-hash-utils.c | 11 +++++++++++ shared/nm-glib-aux/nm-hash-utils.h | 3 +++ 2 files changed, 14 insertions(+) diff --git a/shared/nm-glib-aux/nm-hash-utils.c b/shared/nm-glib-aux/nm-hash-utils.c index 0a701d06e1..232c62c036 100644 --- a/shared/nm-glib-aux/nm-hash-utils.c +++ b/shared/nm-glib-aux/nm-hash-utils.c @@ -264,6 +264,17 @@ nm_ppdirect_equal (gconstpointer a, gconstpointer b) /*****************************************************************************/ +guint +nm_gbytes_hash (gconstpointer p) +{ + GBytes *ptr = (GBytes *) p; + gconstpointer arr; + gsize len; + + arr = g_bytes_get_data (ptr, &len); + return nm_hash_mem (792701303u, arr, len); +} + guint nm_pgbytes_hash (gconstpointer p) { diff --git a/shared/nm-glib-aux/nm-hash-utils.h b/shared/nm-glib-aux/nm-hash-utils.h index 9f2e976678..be69bfa8dc 100644 --- a/shared/nm-glib-aux/nm-hash-utils.h +++ b/shared/nm-glib-aux/nm-hash-utils.h @@ -315,6 +315,9 @@ gboolean nm_ppdirect_equal (gconstpointer a, gconstpointer b); /*****************************************************************************/ +guint nm_gbytes_hash (gconstpointer p); +#define nm_gbytes_equal g_bytes_equal + guint nm_pgbytes_hash (gconstpointer p); gboolean nm_pgbytes_equal (gconstpointer a, gconstpointer b); From 32664c72a5a61e1b2a156436d9132cbbc928b9c0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 27 Apr 2020 08:02:40 +0200 Subject: [PATCH 2/9] shared: add nm_gbytes_get_empty() singleton getter --- shared/nm-glib-aux/nm-shared-utils.c | 18 ++++++++++++++++++ shared/nm-glib-aux/nm-shared-utils.h | 2 ++ 2 files changed, 20 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 61e3e26716..5dd099a7c9 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -397,6 +397,24 @@ truncate: /*****************************************************************************/ +GBytes * +nm_gbytes_get_empty (void) +{ + static GBytes *bytes = NULL; + GBytes *b; + +again: + b = g_atomic_pointer_get (&bytes); + if (G_UNLIKELY (!b)) { + b = g_bytes_new_static ("", 0); + if (!g_atomic_pointer_compare_and_exchange (&bytes, NULL, b)) { + g_bytes_unref (b); + goto again; + } + } + return b; +} + /** * nm_utils_gbytes_equals: * @bytes: (allow-none): a #GBytes array to compare. Note that diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index 9200f80fdf..d5990c2d54 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -389,6 +389,8 @@ nm_utils_is_separator (const char c) /*****************************************************************************/ +GBytes *nm_gbytes_get_empty (void); + static inline gboolean nm_gbytes_equal0 (GBytes *a, GBytes *b) { From ee7fbc954ec6ca4f85e384083f8065271147f53e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 26 Apr 2020 15:46:53 +0200 Subject: [PATCH 3/9] shared/glib: prevent users to use g_cancellable_reset() When handling a GCancellable, you make decisions based on when the cancelled property of a GCancellable changes. Correctly handling a cancellable becoming uncancelled again is really complicated, nor is it clear what it even means: should the flipping be treated as cancellation or not? Probably if the cancelled property gets reset, you already start aborting and there is no way back. So, you would want that a cancellation is always handled. But it's hard to implement that correctly, and it's odd to claim something was cancelled, if g_cancellable_is_cancelled() doesn't agree (anymore). Avoid such problems by preventing users to call g_cancellable_reset(). --- shared/nm-glib-aux/nm-glib.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/shared/nm-glib-aux/nm-glib.h b/shared/nm-glib-aux/nm-glib.h index 4ecba9ffca..7a5b8edd7d 100644 --- a/shared/nm-glib-aux/nm-glib.h +++ b/shared/nm-glib-aux/nm-glib.h @@ -622,4 +622,12 @@ g_hash_table_steal_extended (GHashTable *hash_table, /*****************************************************************************/ +__attribute__((__deprecated__("Don't use g_cancellable_reset(). Create a new cancellable instead."))) +void _nm_g_cancellable_reset (GCancellable *cancellable); + +#undef g_cancellable_reset +#define g_cancellable_reset(cancellable) _nm_g_cancellable_reset(cancellable) + +/*****************************************************************************/ + #endif /* __NM_GLIB_H__ */ From 800ac28ccab6fad6278c5455d45a8ec950319e80 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 26 Apr 2020 14:44:41 +0200 Subject: [PATCH 4/9] device: add nm_device_get_manager() NMDevice already has access to the NMSettings singleton. It is permissible that NMDevice *knows* about NMManager. The current alternative is emitting GObject signals like NM_DEVICE_AUTH_REQUEST, pretending that NMDevice and NMManager would be completely independent, or that there could be anybody else handling the request aside NMManager. No, NMManager and NMDevice may know each other and refer to each other. Just like NMDevice also knows and refers to NMSettings. --- src/devices/nm-device-private.h | 2 ++ src/devices/nm-device.c | 10 +++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/devices/nm-device-private.h b/src/devices/nm-device-private.h index 8d539026e7..f9bc4125b8 100644 --- a/src/devices/nm-device-private.h +++ b/src/devices/nm-device-private.h @@ -46,6 +46,8 @@ void nm_device_arp_announce (NMDevice *self); NMSettings *nm_device_get_settings (NMDevice *self); +NMManager *nm_device_get_manager (NMDevice *self); + gboolean nm_device_set_ip_ifindex (NMDevice *self, int ifindex); gboolean nm_device_set_ip_iface (NMDevice *self, const char *iface); diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index d400d90617..4a24c40bc5 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -578,6 +578,7 @@ typedef struct _NMDevicePrivate { NMMetered metered; NMSettings *settings; + NMManager *manager; NMNetns *netns; @@ -913,6 +914,12 @@ nm_device_get_settings (NMDevice *self) return NM_DEVICE_GET_PRIVATE (self)->settings; } +NMManager * +nm_device_get_manager (NMDevice *self) +{ + return NM_DEVICE_GET_PRIVATE (self)->manager; +} + NMNetns * nm_device_get_netns (NMDevice *self) { @@ -17452,8 +17459,8 @@ constructed (GObject *object) g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP6_ROUTE_CHANGED, G_CALLBACK (device_ipx_changed), self); g_signal_connect (platform, NM_PLATFORM_SIGNAL_LINK_CHANGED, G_CALLBACK (link_changed_cb), self); + priv->manager = g_object_ref (NM_MANAGER_GET); priv->settings = g_object_ref (NM_SETTINGS_GET); - g_assert (priv->settings); g_signal_connect (priv->settings, NM_SETTINGS_SIGNAL_CONNECTION_ADDED, @@ -17610,6 +17617,7 @@ finalize (GObject *object) /* for testing, NMDeviceTest does not invoke NMDevice::constructed, * and thus @settings might be unset. */ nm_g_object_unref (priv->settings); + nm_g_object_unref (priv->manager); nm_g_object_unref (priv->concheck_mgr); From ef7fd9e4e35359ecf83f5987816013c960d91cf1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 26 Apr 2020 15:40:40 +0200 Subject: [PATCH 5/9] auth: natively support GCancellable in NMAuthChain We want that our asynchronous operations are cancellable. In fact, NMAuthChain is already (manually) cancellable by the user calling nm_auth_chain_destroy(). However, sometimes we have a GCancellable at hand, so the callers would have to register to the cancellable themselves. Instead, support setting a cancellable to the NMAuthChain, that aborts the request and invokes the callback. It does so always on an idle handler. Also, the user may only set the cancellable once, and only before starting the first call. --- src/nm-auth-utils.c | 160 ++++++++++++++++++++++++++++++++++++-------- src/nm-auth-utils.h | 4 ++ 2 files changed, 137 insertions(+), 27 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index ca2870db72..663739b457 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -27,11 +27,19 @@ struct _NMAuthChain { GDBusMethodInvocation *context; NMAuthSubject *subject; + GCancellable *cancellable; + + /* if set, it also means that the chain is already started and was cancelled. */ + GSource *cancellable_idle_source; + NMAuthChainResultFunc done_func; gpointer user_data; + gulong cancellable_id; + guint num_pending_auth_calls; + bool is_started:1; bool is_destroyed:1; bool is_finishing:1; }; @@ -77,6 +85,82 @@ _ASSERT_call (AuthCall *call) /*****************************************************************************/ +static void +_done_and_destroy (NMAuthChain *self) +{ + self->is_finishing = TRUE; + self->done_func (self, self->context, self->user_data); + nm_assert (self->is_finishing); + _auth_chain_destroy (self); +} + +static gboolean +_cancellable_idle_cb (gpointer user_data) +{ + NMAuthChain *self = user_data; + AuthCall *call; + + nm_assert (g_cancellable_is_cancelled (self->cancellable)); + nm_assert (self->cancellable_idle_source); + + c_list_for_each_entry (call, &self->auth_call_lst_head, auth_call_lst) { + if (call->call_id) { + self->num_pending_auth_calls--; + nm_auth_manager_check_authorization_cancel (g_steal_pointer (&call->call_id)); + } + } + + _done_and_destroy (self); + return G_SOURCE_REMOVE; +} + +static void +_cancellable_on_idle (NMAuthChain *self) +{ + if (self->cancellable_idle_source) + return; + + self->cancellable_idle_source = nm_g_idle_source_new (G_PRIORITY_DEFAULT, + _cancellable_idle_cb, + self, + NULL); + g_source_attach (self->cancellable_idle_source, NULL); +} + +GCancellable * +nm_auth_chain_get_cancellable (NMAuthChain *self) +{ + return self->cancellable; +} + +static void +_cancellable_cancelled (GCancellable *cancellable, + NMAuthChain *self) +{ + _cancellable_on_idle (self); +} + +void +nm_auth_chain_set_cancellable (NMAuthChain *self, + GCancellable *cancellable) +{ + g_return_if_fail (self); + g_return_if_fail (G_IS_CANCELLABLE (cancellable)); + + /* after the chain is started, the cancellable can no longer be changed. + * No need to handle the complexity of swapping the cancellable *after* + * requests are already started. */ + g_return_if_fail (!self->is_started); + nm_assert (c_list_is_empty (&self->auth_call_lst_head)); + + /* also no need to allow setting different cancellables. */ + g_return_if_fail (!self->cancellable); + + self->cancellable = g_object_ref (cancellable); +} + +/*****************************************************************************/ + static void auth_call_free (AuthCall *call) { @@ -87,7 +171,7 @@ auth_call_free (AuthCall *call) call->chain->num_pending_auth_calls--; nm_auth_manager_check_authorization_cancel (call->call_id); } - g_slice_free (AuthCall, call); + nm_g_slice_free (call); } static AuthCall * @@ -255,6 +339,12 @@ nm_auth_chain_get_result (NMAuthChain *self, const char *permission) nm_assert (!auth_call->call_id); + if (self->cancellable_idle_source) { + /* already cancelled. We always return unknown (even if we happen to + * have already received the response. */ + return NM_AUTH_CALL_RESULT_UNKNOWN; + } + return auth_call->result; } @@ -314,10 +404,7 @@ pk_call_cb (NMAuthManager *auth_manager, if (call->chain->num_pending_auth_calls == 0) { /* we are on an idle-handler or a clean call-stack (non-reentrant) so it's safe * to invoke the callback right away. */ - self->is_finishing = TRUE; - self->done_func (self, self->context, self->user_data); - nm_assert (self->is_finishing); - _auth_chain_destroy (self); + _done_and_destroy (self); } } @@ -350,23 +437,36 @@ nm_auth_chain_add_call_unsafe (NMAuthChain *self, g_return_if_fail (!self->is_finishing); g_return_if_fail (!self->is_destroyed); g_return_if_fail (permission && *permission); - nm_assert ( nm_auth_subject_get_subject_type (self->subject) - == NM_AUTH_SUBJECT_TYPE_UNIX_PROCESS - || nm_auth_subject_get_subject_type (self->subject) - == NM_AUTH_SUBJECT_TYPE_INTERNAL); + nm_assert (NM_IN_SET (nm_auth_subject_get_subject_type (self->subject), NM_AUTH_SUBJECT_TYPE_UNIX_PROCESS, + NM_AUTH_SUBJECT_TYPE_INTERNAL)); /* duplicate permissions are not supported, also because nm_auth_chain_get_result() * can only return one-permission. */ nm_assert (!_find_auth_call (self, permission)); - call = g_slice_new (AuthCall); + if (!self->is_started) { + self->is_started = TRUE; + nm_assert (!self->cancellable_id); + if (self->cancellable) { + if (g_cancellable_is_cancelled (self->cancellable)) { + /* the operation is already cancelled. Schedule the callback on idle. */ + _cancellable_on_idle (self); + } else { + self->cancellable_id = g_signal_connect (self->cancellable, + "cancelled", + G_CALLBACK (_cancellable_cancelled), + self); + } + } + } + call = g_slice_new (AuthCall); *call = (AuthCall) { .chain = self, .call_id = NULL, .result = NM_AUTH_CALL_RESULT_UNKNOWN, - /* we don't clone the permission string. It's the callers responsiblity. */ + /* we don't clone the permission string. It's the callers responsibility. */ .permission = permission, }; @@ -375,14 +475,19 @@ nm_auth_chain_add_call_unsafe (NMAuthChain *self, * call. */ c_list_link_front (&self->auth_call_lst_head, &call->auth_call_lst); - call->call_id = nm_auth_manager_check_authorization (nm_auth_manager_get (), - self->subject, - permission, - allow_interaction, - pk_call_cb, - call); + if (self->cancellable_idle_source) { + /* already cancelled. No need to actually start the request. */ + nm_assert (call->result == NM_AUTH_CALL_RESULT_UNKNOWN); + } else { + call->call_id = nm_auth_manager_check_authorization (nm_auth_manager_get (), + self->subject, + permission, + allow_interaction, + pk_call_cb, + call); - self->num_pending_auth_calls++; + self->num_pending_auth_calls++; + } _ASSERT_call (call); @@ -427,10 +532,8 @@ nm_auth_chain_new_subject (NMAuthSubject *subject, NMAuthChain *self; g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL); - nm_assert ( nm_auth_subject_get_subject_type (subject) - == NM_AUTH_SUBJECT_TYPE_UNIX_PROCESS - || nm_auth_subject_get_subject_type (subject) - == NM_AUTH_SUBJECT_TYPE_INTERNAL); + nm_assert (NM_IN_SET (nm_auth_subject_get_subject_type (subject), NM_AUTH_SUBJECT_TYPE_UNIX_PROCESS, + NM_AUTH_SUBJECT_TYPE_INTERNAL)); nm_assert (done_func); self = g_slice_new (NMAuthChain); @@ -489,6 +592,9 @@ _auth_chain_destroy (NMAuthChain *self) nm_clear_g_object (&self->subject); nm_clear_g_object (&self->context); + nm_clear_g_signal_handler (self->cancellable, &self->cancellable_id); + nm_clear_g_source_inst (&self->cancellable_idle_source); + /* we must first destry all AuthCall instances before ChainData. The reason is * that AuthData.permission is not cloned and the lifetime of the string must * be ensured by the caller. A sensible thing to do for the caller is attach the @@ -499,7 +605,9 @@ _auth_chain_destroy (NMAuthChain *self) while ((chain_data = c_list_first_entry (&self->data_lst_head, ChainData, data_lst))) chain_data_free (chain_data); - g_slice_free (NMAuthChain, self); + nm_g_object_unref (self->cancellable); + + nm_g_slice_free (self); } /****************************************************************************** @@ -517,10 +625,8 @@ nm_auth_is_subject_in_acl (NMConnection *connection, g_return_val_if_fail (connection, FALSE); g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), FALSE); - nm_assert ( nm_auth_subject_get_subject_type (subject) - == NM_AUTH_SUBJECT_TYPE_INTERNAL - || nm_auth_subject_get_subject_type (subject) - == NM_AUTH_SUBJECT_TYPE_UNIX_PROCESS); + nm_assert (NM_IN_SET (nm_auth_subject_get_subject_type (subject), NM_AUTH_SUBJECT_TYPE_UNIX_PROCESS, + NM_AUTH_SUBJECT_TYPE_INTERNAL)); if (nm_auth_subject_get_subject_type (subject) == NM_AUTH_SUBJECT_TYPE_INTERNAL) return TRUE; diff --git a/src/nm-auth-utils.h b/src/nm-auth-utils.h index 978d607aed..a6c6d65965 100644 --- a/src/nm-auth-utils.h +++ b/src/nm-auth-utils.h @@ -27,6 +27,10 @@ NMAuthChain *nm_auth_chain_new_subject (NMAuthSubject *subject, NMAuthChainResultFunc done_func, gpointer user_data); +GCancellable *nm_auth_chain_get_cancellable (NMAuthChain *self); +void nm_auth_chain_set_cancellable (NMAuthChain *self, + GCancellable *cancellable); + gpointer nm_auth_chain_get_data (NMAuthChain *chain, const char *tag); gpointer nm_auth_chain_steal_data (NMAuthChain *chain, const char *tag); From d935692bc7bcb774aa9c27840265687d1b79c729 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 26 Apr 2020 17:41:55 +0200 Subject: [PATCH 6/9] auth: track NMAuthChain data in array instead of CList It's about as complicated to track a CList as it is to track an allocated array. The latter requires fewer allocations and has better locality. That makes it preferable. --- src/nm-auth-utils.c | 88 ++++++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 45 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 663739b457..b9efff4805 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -16,11 +16,19 @@ /*****************************************************************************/ +typedef struct { + const char *tag; + gpointer data; + GDestroyNotify destroy; +} ChainData; + struct _NMAuthChain { CList parent_lst; - CList data_lst_head; + ChainData *data_arr; + guint data_len; + guint data_alloc; CList auth_call_lst_head; @@ -188,29 +196,16 @@ _find_auth_call (NMAuthChain *self, const char *permission) /*****************************************************************************/ -typedef struct { - CList data_lst; - const char *tag; - gpointer data; - GDestroyNotify destroy; -} ChainData; - -static void -chain_data_free (ChainData *chain_data) -{ - c_list_unlink_stale (&chain_data->data_lst); - if (chain_data->destroy) - chain_data->destroy (chain_data->data); - g_slice_free (ChainData, chain_data); -} - static ChainData * _get_data (NMAuthChain *self, const char *tag) { - ChainData *chain_data; + guint i; - c_list_for_each_entry (chain_data, &self->data_lst_head, data_lst) { - if (nm_streq (chain_data->tag, tag)) + for (i = 0; i < self->data_len; i++) { + ChainData *chain_data = &self->data_arr[i]; + + if ( chain_data->tag + && nm_streq (chain_data->tag, tag)) return chain_data; } return NULL; @@ -243,7 +238,6 @@ gpointer nm_auth_chain_steal_data (NMAuthChain *self, const char *tag) { ChainData *chain_data; - gpointer value; g_return_val_if_fail (self, NULL); g_return_val_if_fail (tag, NULL); @@ -252,12 +246,13 @@ nm_auth_chain_steal_data (NMAuthChain *self, const char *tag) if (!chain_data) return NULL; - value = chain_data->data; - - /* Make sure the destroy handler isn't called when freeing */ + /* Make sure the destroy handler isn't called when freeing. + * + * We don't bother to really remove the element from the array. + * Just mark the entry as unused by clearing the tag. */ chain_data->destroy = NULL; - chain_data_free (chain_data); - return value; + chain_data->tag = NULL; + return chain_data->data; } /** @@ -268,7 +263,7 @@ nm_auth_chain_steal_data (NMAuthChain *self, const char *tag) * and nothing is attached. * @data_destroy: (allow-none): the destroy function for the data pointer. * - * @tag string is not cloned and must outlife @self. That is why + * @tag string is not cloned and must outlive @self. That is why * the function is "unsafe". Use nm_auth_chain_set_data() with a C literal * instead. * @@ -285,12 +280,8 @@ nm_auth_chain_set_data_unsafe (NMAuthChain *self, g_return_if_fail (self); g_return_if_fail (tag); - /* we should not track a large number of elements via a linked list. If this becomes - * necessary, revert the code to use GHashTable again. */ - nm_assert (c_list_length (&self->data_lst_head) < 25); - - /* The tag must not yet exist. Otherwise we'd have to first search the linked - * list for an existing entry. */ + /* The tag must not yet exist. Otherwise we'd have to first search the + * list for an existing entry. That usage pattern is not supported. */ nm_assert (!_get_data (self, tag)); if (!data) { @@ -302,17 +293,20 @@ nm_auth_chain_set_data_unsafe (NMAuthChain *self, return; } - chain_data = g_slice_new (ChainData); + if (self->data_len + 1 > self->data_alloc) { + if (self->data_alloc == 0) + self->data_alloc = 8; + else + self->data_alloc *= 2; + self->data_arr = g_realloc (self->data_arr, sizeof (self->data_arr[0]) * self->data_alloc); + } + + chain_data = &self->data_arr[self->data_len++]; *chain_data = (ChainData) { .tag = tag, .data = data, .destroy = data_destroy, }; - - /* we assert that no duplicate tags are added. But still, add the new - * element to the front, so that it would shadow the duplicate element - * in the list. */ - c_list_link_front (&self->data_lst_head, &chain_data->data_lst); } /*****************************************************************************/ @@ -494,7 +488,8 @@ nm_auth_chain_add_call_unsafe (NMAuthChain *self, /* we track auth-calls in a linked list. If we end up requesting too many permissions this * becomes inefficient. If that ever happens, consider a more efficient data structure for * a large number of requests. */ - nm_assert (self->num_pending_auth_calls < 25); + nm_assert (c_list_length (&self->auth_call_lst_head) < 25); + G_STATIC_ASSERT_EXPR (NM_CLIENT_PERMISSION_LAST < 25); } /*****************************************************************************/ @@ -543,7 +538,6 @@ nm_auth_chain_new_subject (NMAuthSubject *subject, .context = nm_g_object_ref (context), .subject = g_object_ref (subject), .parent_lst = C_LIST_INIT (self->parent_lst), - .data_lst_head = C_LIST_INIT (self->data_lst_head), .auth_call_lst_head = C_LIST_INIT (self->auth_call_lst_head), }; return self; @@ -585,7 +579,6 @@ static void _auth_chain_destroy (NMAuthChain *self) { AuthCall *call; - ChainData *chain_data; c_list_unlink (&self->parent_lst); @@ -595,15 +588,20 @@ _auth_chain_destroy (NMAuthChain *self) nm_clear_g_signal_handler (self->cancellable, &self->cancellable_id); nm_clear_g_source_inst (&self->cancellable_idle_source); - /* we must first destry all AuthCall instances before ChainData. The reason is + /* we must first destroy all AuthCall instances before ChainData. The reason is * that AuthData.permission is not cloned and the lifetime of the string must * be ensured by the caller. A sensible thing to do for the caller is attach the * permission string via nm_auth_chain_set_data(). Hence, first free the AuthCall. */ while ((call = c_list_first_entry (&self->auth_call_lst_head, AuthCall, auth_call_lst))) auth_call_free (call); - while ((chain_data = c_list_first_entry (&self->data_lst_head, ChainData, data_lst))) - chain_data_free (chain_data); + while (self->data_len > 0) { + ChainData *chain_data = &self->data_arr[--self->data_len]; + + if (chain_data->destroy) + chain_data->destroy (chain_data->data); + } + g_free (self->data_arr); nm_g_object_unref (self->cancellable); From b50702775f1b7a56ad81fe9be58294a29e805c8f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 26 Apr 2020 13:59:13 +0200 Subject: [PATCH 7/9] device: implement "auth-request" as async operation nm_manager_device_auth_request() GObject signals only complicate the code and are less efficient. Also, NM_DEVICE_AUTH_REQUEST signal really invoked an asynchronous request. Of course, fundamentally emitting a signal *is* the same as calling a method. However, implementing this as signal is really not nice nor best practice. For one, there is a (negligible) overhead emitting a GObject signal. But what is worse, GObject signals are not as strongly typed and make it harder to understand what happens. The signal had the appearance of providing some special decoupling of NMDevice and NMManager. Of course, in practice, they were not more decoupled (both forms are the same in nature), but it was harder to understand how they work together. Add and call a method nm_manager_device_auth_request() instead. This has the notion of invoking an asynchronous method. Also, never invoke the callback synchronously and provide a cancellable. Like every asynchronous operation, it *must* be cancellable, and callers should make sure to provide a mechanism to abort. --- src/devices/nm-device-private.h | 11 +++ src/devices/nm-device.c | 105 +++++++++++++++------------ src/devices/nm-device.h | 7 -- src/devices/wifi/nm-device-iwd.c | 16 ++--- src/devices/wifi/nm-device-wifi.c | 16 ++--- src/nm-manager.c | 115 ++++++++++++++++++++---------- src/nm-manager.h | 12 ++++ src/nm-types.h | 6 ++ 8 files changed, 183 insertions(+), 105 deletions(-) diff --git a/src/devices/nm-device-private.h b/src/devices/nm-device-private.h index f9bc4125b8..4e260da2f3 100644 --- a/src/devices/nm-device-private.h +++ b/src/devices/nm-device-private.h @@ -205,4 +205,15 @@ gboolean nm_device_match_parent_hwaddr (NMDevice *device, NMConnection *connection, gboolean fail_if_no_hwaddr); +/*****************************************************************************/ + +void nm_device_auth_request (NMDevice *self, + GDBusMethodInvocation *context, + NMConnection *connection, + const char *permission, + gboolean allow_interaction, + GCancellable *cancellable, + NMManagerDeviceAuthRequestFunc callback, + gpointer user_data); + #endif /* NM_DEVICE_PRIVATE_H */ diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 4a24c40bc5..27b1b600b1 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -192,7 +192,6 @@ typedef struct { enum { STATE_CHANGED, AUTOCONNECT_ALLOWED, - AUTH_REQUEST, IP4_CONFIG_CHANGED, IP6_CONFIG_CHANGED, IP6_PREFIX_DELEGATED, @@ -6265,6 +6264,27 @@ dnsmasq_state_changed_cb (NMDnsMasqManager *manager, guint32 status, gpointer us } } +void +nm_device_auth_request (NMDevice *self, + GDBusMethodInvocation *context, + NMConnection *connection, + const char *permission, + gboolean allow_interaction, + GCancellable *cancellable, + NMManagerDeviceAuthRequestFunc callback, + gpointer user_data) +{ + nm_manager_device_auth_request (nm_device_get_manager (self), + self, + context, + connection, + permission, + allow_interaction, + cancellable, + callback, + user_data); +} + /*****************************************************************************/ static void @@ -12303,13 +12323,14 @@ impl_device_reapply (NMDBusObject *obj, } else reapply_data = NULL; - g_signal_emit (self, signals[AUTH_REQUEST], 0, - invocation, - nm_device_get_applied_connection (self), - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - reapply_cb, - reapply_data); + nm_device_auth_request (self, + invocation, + nm_device_get_applied_connection (self), + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + NULL, + reapply_cb, + reapply_data); } /*****************************************************************************/ @@ -12346,13 +12367,14 @@ get_applied_connection_cb (NMDevice *self, if (applied_connection != user_data) { /* The applied connection changed due to a race. Reauthenticate. */ - g_signal_emit (self, signals[AUTH_REQUEST], 0, - context, - applied_connection, - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - get_applied_connection_cb, - applied_connection /* no need take a ref. We will not dereference this pointer. */); + nm_device_auth_request (self, + context, + applied_connection, + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + NULL, + get_applied_connection_cb, + applied_connection /* no need take a ref. We will not dereference this pointer. */); return; } @@ -12399,13 +12421,14 @@ impl_device_get_applied_connection (NMDBusObject *obj, return; } - g_signal_emit (self, signals[AUTH_REQUEST], 0, - invocation, - applied_connection, - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - get_applied_connection_cb, - applied_connection /* no need take a ref. We will not dereference this pointer. */); + nm_device_auth_request (self, + invocation, + applied_connection, + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + NULL, + get_applied_connection_cb, + applied_connection /* no need take a ref. We will not dereference this pointer. */); } /*****************************************************************************/ @@ -12573,13 +12596,14 @@ impl_device_disconnect (NMDBusObject *obj, connection = nm_device_get_applied_connection (self); nm_assert (connection); - g_signal_emit (self, signals[AUTH_REQUEST], 0, - invocation, - connection, - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - disconnect_cb, - NULL); + nm_device_auth_request (self, + invocation, + connection, + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + NULL, + disconnect_cb, + NULL); } static void @@ -12625,13 +12649,14 @@ impl_device_delete (NMDBusObject *obj, return; } - g_signal_emit (self, signals[AUTH_REQUEST], 0, - invocation, - NULL, - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - delete_cb, - NULL); + nm_device_auth_request (self, + invocation, + NULL, + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + NULL, + delete_cb, + NULL); } static void @@ -18014,14 +18039,6 @@ nm_device_class_init (NMDeviceClass *klass) autoconnect_allowed_accumulator, NULL, NULL, G_TYPE_BOOLEAN, 0); - signals[AUTH_REQUEST] = - g_signal_new (NM_DEVICE_AUTH_REQUEST, - G_OBJECT_CLASS_TYPE (object_class), - G_SIGNAL_RUN_FIRST, - 0, NULL, NULL, NULL, - /* context, connection, permission, allow_interaction, callback, user_data */ - G_TYPE_NONE, 6, G_TYPE_DBUS_METHOD_INVOCATION, NM_TYPE_CONNECTION, G_TYPE_STRING, G_TYPE_BOOLEAN, G_TYPE_POINTER, G_TYPE_POINTER); - signals[IP4_CONFIG_CHANGED] = g_signal_new (NM_DEVICE_IP4_CONFIG_CHANGED, G_OBJECT_CLASS_TYPE (object_class), diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index c18301b275..8d51c707dd 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -115,7 +115,6 @@ nm_device_state_reason_check (NMDeviceStateReason reason) #define NM_DEVICE_HAS_PENDING_ACTION "has-pending-action" /* Internal only */ /* Internal signals */ -#define NM_DEVICE_AUTH_REQUEST "auth-request" #define NM_DEVICE_IP4_CONFIG_CHANGED "ip4-config-changed" #define NM_DEVICE_IP6_CONFIG_CHANGED "ip6-config-changed" #define NM_DEVICE_IP6_PREFIX_DELEGATED "ip6-prefix-delegated" @@ -455,12 +454,6 @@ typedef struct _NMDeviceClass { } NMDeviceClass; -typedef void (*NMDeviceAuthRequestFunc) (NMDevice *device, - GDBusMethodInvocation *context, - NMAuthSubject *subject, - GError *error, - gpointer user_data); - GType nm_device_get_type (void); struct _NMDedupMultiIndex *nm_device_get_multi_index (NMDevice *self); diff --git a/src/devices/wifi/nm-device-iwd.c b/src/devices/wifi/nm-device-iwd.c index 3dccd1cb94..e2e705fd2e 100644 --- a/src/devices/wifi/nm-device-iwd.c +++ b/src/devices/wifi/nm-device-iwd.c @@ -1061,14 +1061,14 @@ _nm_device_iwd_request_scan (NMDeviceIwd *self, return; } - g_signal_emit_by_name (device, - NM_DEVICE_AUTH_REQUEST, - invocation, - NULL, - NM_AUTH_PERMISSION_WIFI_SCAN, - TRUE, - dbus_request_scan_cb, - options ? g_variant_ref (options) : NULL); + nm_device_auth_request (device, + invocation, + NULL, + NM_AUTH_PERMISSION_WIFI_SCAN, + TRUE, + NULL, + dbus_request_scan_cb, + nm_g_variant_ref (options)); } static gboolean diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index bf3e6ad27a..8aee27f4ce 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -1323,14 +1323,14 @@ _nm_device_wifi_request_scan (NMDeviceWifi *self, return; } - g_signal_emit_by_name (device, - NM_DEVICE_AUTH_REQUEST, - invocation, - NULL, - NM_AUTH_PERMISSION_WIFI_SCAN, - TRUE, - dbus_request_scan_cb, - g_steal_pointer (&ssids)); + nm_device_auth_request (device, + invocation, + NULL, + NM_AUTH_PERMISSION_WIFI_SCAN, + TRUE, + NULL, + dbus_request_scan_cb, + g_steal_pointer (&ssids)); } static gboolean diff --git a/src/nm-manager.c b/src/nm-manager.c index 1f3ba1f661..bbe501259f 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2375,8 +2375,9 @@ device_auth_done_cb (NMAuthChain *chain, gs_free_error GError *error = NULL; NMAuthCallResult result; NMDevice *device; + GCancellable *cancellable; const char *permission; - NMDeviceAuthRequestFunc callback; + NMManagerDeviceAuthRequestFunc callback; NMAuthSubject *subject; nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); @@ -2390,18 +2391,26 @@ device_auth_done_cb (NMAuthChain *chain, device = nm_auth_chain_get_data (chain, "device"); nm_assert (NM_IS_DEVICE (device)); + cancellable = nm_auth_chain_get_cancellable (chain); + nm_assert (!cancellable || G_IS_CANCELLABLE (cancellable)); + result = nm_auth_chain_get_result (chain, permission); subject = nm_auth_chain_get_subject (chain); - if (result != NM_AUTH_CALL_RESULT_YES) { - _LOGD (LOGD_CORE, "%s request failed: not authorized", permission); - error = g_error_new (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "%s request failed: not authorized", - permission); - } + if ( cancellable + && g_cancellable_set_error_if_cancelled (cancellable, &error)) { + /* pass. */ + } else { + if (result != NM_AUTH_CALL_RESULT_YES) { + _LOGD (LOGD_CORE, "%s request failed: not authorized", permission); + error = g_error_new (NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + "%s request failed: not authorized", + permission); + } - nm_assert (error || (result == NM_AUTH_CALL_RESULT_YES)); + nm_assert (error || (result == NM_AUTH_CALL_RESULT_YES)); + } callback (device, context, @@ -2411,28 +2420,53 @@ device_auth_done_cb (NMAuthChain *chain, } static void -device_auth_request_cb (NMDevice *device, - GDBusMethodInvocation *context, - NMConnection *connection, - const char *permission, - gboolean allow_interaction, - NMDeviceAuthRequestFunc callback, - gpointer user_data, - NMManager *self) +_device_auth_done_fail_on_idle (gpointer user_data, GCancellable *cancellable) +{ + gs_unref_object NMManager *self = NULL; + gs_unref_object NMDevice *device = NULL; + gs_unref_object GDBusMethodInvocation *context = NULL; + gs_unref_object NMAuthSubject *subject = NULL; + gs_free_error GError *error_original = NULL; + gs_free_error GError *error_cancelled = NULL; + NMManagerDeviceAuthRequestFunc callback; + gpointer callback_user_data; + + nm_utils_user_data_unpack (&self, &device, &context, &subject, &error_original, &callback, &callback_user_data); + + g_cancellable_set_error_if_cancelled (cancellable, &error_cancelled); + + callback (device, + context, + subject, + error_cancelled ?: error_original, + callback_user_data); +} + +void +nm_manager_device_auth_request (NMManager *self, + NMDevice *device, + GDBusMethodInvocation *context, + NMConnection *connection, + const char *permission, + gboolean allow_interaction, + GCancellable *cancellable, + NMManagerDeviceAuthRequestFunc callback, + gpointer user_data) { NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - GError *error = NULL; - NMAuthSubject *subject = NULL; + gs_free_error GError *error = NULL; + gs_unref_object NMAuthSubject *subject = NULL; NMAuthChain *chain; char *permission_dup; /* Validate the caller */ subject = nm_dbus_manager_new_auth_subject_from_context (context); if (!subject) { - error = g_error_new_literal (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - NM_UTILS_ERROR_MSG_REQ_UID_UKNOWN); - goto done; + g_set_error_literal (&error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + NM_UTILS_ERROR_MSG_REQ_UID_UKNOWN); + goto fail_on_idle; } /* Ensure the subject has permissions for this connection */ @@ -2442,17 +2476,21 @@ device_auth_request_cb (NMDevice *device, NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, &error)) - goto done; + goto fail_on_idle; /* Validate the request */ chain = nm_auth_chain_new_subject (subject, context, device_auth_done_cb, self); if (!chain) { - error = g_error_new_literal (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - NM_UTILS_ERROR_MSG_REQ_AUTH_FAILED); - goto done; + g_set_error (&error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + NM_UTILS_ERROR_MSG_REQ_AUTH_FAILED); + goto fail_on_idle; } + if (cancellable) + nm_auth_chain_set_cancellable (chain, cancellable); + permission_dup = g_strdup (permission); c_list_link_tail (&priv->auth_lst_head, nm_auth_chain_parent_lst_list (chain)); @@ -2461,13 +2499,18 @@ device_auth_request_cb (NMDevice *device, nm_auth_chain_set_data (chain, "user-data", user_data, NULL); nm_auth_chain_set_data (chain, "perm", permission_dup /* transfer ownership */, g_free); nm_auth_chain_add_call_unsafe (chain, permission_dup, allow_interaction); + return; -done: - if (error) - callback (device, context, subject, error, user_data); - - g_clear_object (&subject); - g_clear_error (&error); +fail_on_idle: + nm_utils_invoke_on_idle (cancellable, + _device_auth_done_fail_on_idle, + nm_utils_user_data_pack (g_object_ref (self), + g_object_ref (device), + g_object_ref (context), + g_steal_pointer (&subject), + g_steal_pointer (&error), + callback, + user_data)); } static gboolean @@ -3130,10 +3173,6 @@ add_device (NMManager *self, NMDevice *device, GError **error) G_CALLBACK (manager_device_state_changed), self); - g_signal_connect (device, NM_DEVICE_AUTH_REQUEST, - G_CALLBACK (device_auth_request_cb), - self); - g_signal_connect (device, NM_DEVICE_REMOVED, G_CALLBACK (device_removed_cb), self); diff --git a/src/nm-manager.h b/src/nm-manager.h index 5873abd24b..ab08eaa839 100644 --- a/src/nm-manager.h +++ b/src/nm-manager.h @@ -193,4 +193,16 @@ NMMetered nm_manager_get_metered (NMManager *self); void nm_manager_notify_device_availibility_maybe_changed (NMManager *self); +/*****************************************************************************/ + +void nm_manager_device_auth_request (NMManager *self, + NMDevice *device, + GDBusMethodInvocation *context, + NMConnection *connection, + const char *permission, + gboolean allow_interaction, + GCancellable *cancellable, + NMManagerDeviceAuthRequestFunc callback, + gpointer user_data); + #endif /* __NETWORKMANAGER_MANAGER_H__ */ diff --git a/src/nm-types.h b/src/nm-types.h index d2f6fb4093..d2c999fada 100644 --- a/src/nm-types.h +++ b/src/nm-types.h @@ -40,6 +40,12 @@ typedef struct _NMSleepMonitor NMSleepMonitor; typedef struct _NMLldpListener NMLldpListener; typedef struct _NMConfigDeviceStateData NMConfigDeviceStateData; +typedef void (*NMManagerDeviceAuthRequestFunc) (NMDevice *device, + GDBusMethodInvocation *context, + NMAuthSubject *subject, + GError *error, + gpointer user_data); + struct _NMDedupMultiIndex; typedef struct _NMRefString NMRefString; From a7476ff082c6f1c13c46447b61360093f4aa82d1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 27 Apr 2020 08:03:38 +0200 Subject: [PATCH 8/9] supplicant: log changes to max-scan-ssids of NMSupplicantInterface --- src/supplicant/nm-supplicant-interface.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/supplicant/nm-supplicant-interface.c b/src/supplicant/nm-supplicant-interface.c index 099c3bf2ca..700563624a 100644 --- a/src/supplicant/nm-supplicant-interface.c +++ b/src/supplicant/nm-supplicant-interface.c @@ -1204,13 +1204,18 @@ parse_capabilities (NMSupplicantInterface *self, GVariant *capabilities) if (g_variant_lookup (capabilities, "MaxScanSSID", "i", &max_scan_ssids)) { /* We need active scan and SSID probe capabilities to care about MaxScanSSIDs */ - if (max_scan_ssids > 0 && have_active && have_ssid) { + if ( max_scan_ssids > 0 + && have_active + && have_ssid) { /* wpa_supplicant's NM_WPAS_MAX_SCAN_SSIDS value is 16, but for speed * and to ensure we don't disclose too many SSIDs from the hidden * list, we'll limit to 5. */ - priv->max_scan_ssids = CLAMP (max_scan_ssids, 0, 5); - _LOGD ("supports %d scan SSIDs", priv->max_scan_ssids); + max_scan_ssids = CLAMP (max_scan_ssids, 0, 5); + if (max_scan_ssids != priv->max_scan_ssids) { + priv->max_scan_ssids = max_scan_ssids; + _LOGD ("supports %d scan SSIDs", priv->max_scan_ssids); + } } } } From f6e438860bdb03166bbf38d61e115e960e5c60c7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 28 Apr 2020 14:56:06 +0200 Subject: [PATCH 9/9] wifi: express SCAN_RAND_MAC_ADDRESS_EXPIRE time in seconds We commonly use already seconds and milliseconds scales for computing timeouts. Reduce the number of difference scales and don't also use minutes. --- src/devices/wifi/nm-device-wifi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index 8aee27f4ce..f14e2b25e7 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -48,7 +48,7 @@ _LOG_DECLARE_SELF(NMDeviceWifi); #define SCAN_INTERVAL_SEC_STEP 20 #define SCAN_INTERVAL_SEC_MAX 120 -#define SCAN_RAND_MAC_ADDRESS_EXPIRE_MIN 5 +#define SCAN_RAND_MAC_ADDRESS_EXPIRE_SEC (5*60) /*****************************************************************************/ @@ -1169,7 +1169,7 @@ _hw_addr_set_scanning (NMDeviceWifi *self, gboolean do_reset) * We don't bother with to update the MAC address exactly when * it expires, instead on the next scan request, we will generate * a new one.*/ - priv->hw_addr_scan_expire = now + (SCAN_RAND_MAC_ADDRESS_EXPIRE_MIN * 60); + priv->hw_addr_scan_expire = now + SCAN_RAND_MAC_ADDRESS_EXPIRE_SEC; generate_mac_address_mask = nm_config_data_get_device_config (NM_CONFIG_GET_DATA, "wifi.scan-generate-mac-address-mask",