From 09fb7877a9ba3b14a98d0e3e40981ae7996d56b3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 30 Jun 2021 22:02:51 +0200 Subject: [PATCH 1/7] build: fix linking libnm-log-null into different test programs We require these, otherwise we can get a linker error about _nm_utils_monotonic_timestamp_initialized symbol being undefined. --- Makefile.am | 3 +++ src/core/ppp/meson.build | 1 + src/libnm-core-impl/tests/meson.build | 2 +- src/libnmc-setting/tests/meson.build | 1 + src/nm-cloud-setup/tests/meson.build | 1 + src/nm-dispatcher/meson.build | 1 + src/nm-dispatcher/tests/meson.build | 1 + src/nm-initrd-generator/meson.build | 2 +- src/nm-online/meson.build | 1 + src/nmcli/meson.build | 2 ++ 10 files changed, 13 insertions(+), 2 deletions(-) diff --git a/Makefile.am b/Makefile.am index fd1342a925..8897d70d88 100644 --- a/Makefile.am +++ b/Makefile.am @@ -803,6 +803,7 @@ src_libnm_client_aux_extern_tests_test_libnm_client_aux_LDADD = \ src/libnm-core-aux-extern/libnm-core-aux-extern.la \ src/libnm-core-aux-intern/libnm-core-aux-intern.la \ src/libnm-glib-aux/libnm-glib-aux.la \ + src/libnm-log-null/libnm-log-null.la \ src/libnm-std-aux/libnm-std-aux.la \ src/c-siphash/libc-siphash.la \ src/libnm-client-impl/libnm.la \ @@ -4584,6 +4585,7 @@ src_nm_dispatcher_tests_test_dispatcher_envp_LDFLAGS = \ src_nm_dispatcher_tests_test_dispatcher_envp_LDADD = \ src/nm-dispatcher/libnm-dispatcher-core.la \ src/libnm-glib-aux/libnm-glib-aux.la \ + src/libnm-log-null/libnm-log-null.la \ src/libnm-std-aux/libnm-std-aux.la \ src/c-siphash/libc-siphash.la \ src/libnm-client-impl/libnm.la \ @@ -4778,6 +4780,7 @@ src_libnmc_setting_tests_test_libnmc_setting_LDADD = \ src/libnm-core-aux-intern/libnm-core-aux-intern.la \ src/libnm-base/libnm-base.la \ src/libnm-glib-aux/libnm-glib-aux.la \ + src/libnm-log-null/libnm-log-null.la \ src/libnm-std-aux/libnm-std-aux.la \ src/c-siphash/libc-siphash.la \ src/libnm-client-impl/libnm.la \ diff --git a/src/core/ppp/meson.build b/src/core/ppp/meson.build index ffeb0eba9a..9ee46113f5 100644 --- a/src/core/ppp/meson.build +++ b/src/core/ppp/meson.build @@ -14,6 +14,7 @@ nm_pppd_plugin = shared_module( ], link_with: [ libnm_core_impl, + libnm_log_null, libnm_glib_aux, libnm_std_aux, libc_siphash, diff --git a/src/libnm-core-impl/tests/meson.build b/src/libnm-core-impl/tests/meson.build index ca1d1c1aa6..988c60db86 100644 --- a/src/libnm-core-impl/tests/meson.build +++ b/src/libnm-core-impl/tests/meson.build @@ -32,8 +32,8 @@ foreach test_unit: test_units libnm_core_impl, libnm_crypto, libnm_base, - libnm_log_null, libnm_systemd_shared, + libnm_log_null, libnm_glib_aux, libnm_std_aux, libc_siphash, diff --git a/src/libnmc-setting/tests/meson.build b/src/libnmc-setting/tests/meson.build index 6c71f286b3..0ce39a9422 100644 --- a/src/libnmc-setting/tests/meson.build +++ b/src/libnmc-setting/tests/meson.build @@ -13,6 +13,7 @@ exe = executable( libnm_core_aux_extern, libnm_core_aux_intern, libnm_base, + libnm_log_null, libnm_glib_aux, libnm_std_aux, libc_siphash, diff --git a/src/nm-cloud-setup/tests/meson.build b/src/nm-cloud-setup/tests/meson.build index 77e25c81fc..c6881a57be 100644 --- a/src/nm-cloud-setup/tests/meson.build +++ b/src/nm-cloud-setup/tests/meson.build @@ -11,6 +11,7 @@ exe = executable( link_with: [ libnm_cloud_setup_core, libnmc_base, + libnm_log_null, libnm_glib_aux, libnm_std_aux, libc_siphash, diff --git a/src/nm-dispatcher/meson.build b/src/nm-dispatcher/meson.build index 37ae9e8222..eb3ea77700 100644 --- a/src/nm-dispatcher/meson.build +++ b/src/nm-dispatcher/meson.build @@ -39,6 +39,7 @@ executable( link_with: [ libnm_core_aux_extern, libnm_dispatcher_core, + libnm_log_null, libnm_glib_aux, libnm_std_aux, libc_siphash, diff --git a/src/nm-dispatcher/tests/meson.build b/src/nm-dispatcher/tests/meson.build index ac96ef92cd..deaa8819ad 100644 --- a/src/nm-dispatcher/tests/meson.build +++ b/src/nm-dispatcher/tests/meson.build @@ -13,6 +13,7 @@ exe = executable( c_args: introspection_extra_cflags, link_with: [ libnm_dispatcher_core, + libnm_log_null, libnm_glib_aux, libnm_std_aux, libc_siphash, diff --git a/src/nm-initrd-generator/meson.build b/src/nm-initrd-generator/meson.build index 896ed3ec27..080fe7e42f 100644 --- a/src/nm-initrd-generator/meson.build +++ b/src/nm-initrd-generator/meson.build @@ -34,8 +34,8 @@ executable( libnm_platform, libnm_base, libnm_systemd_shared, - libnm_log_core, libnm_udev_aux, + libnm_log_core, libnm_glib_aux, libnm_std_aux, libc_siphash, diff --git a/src/nm-online/meson.build b/src/nm-online/meson.build index 415bb29fdf..272de42c34 100644 --- a/src/nm-online/meson.build +++ b/src/nm-online/meson.build @@ -9,6 +9,7 @@ executable( ], link_with: [ libnm_client_aux_extern, + libnm_log_null, libnm_glib_aux, libnm_std_aux, libc_siphash, diff --git a/src/nmcli/meson.build b/src/nmcli/meson.build index e2d73534b8..fab7329ea0 100644 --- a/src/nmcli/meson.build +++ b/src/nmcli/meson.build @@ -35,6 +35,7 @@ executable( libnm_core_aux_extern, libnm_core_aux_intern, libnm_base, + libnm_log_null, libnm_glib_aux, libnm_std_aux, libc_siphash, @@ -61,6 +62,7 @@ generate_docs_nm_settings_nmcli = executable( libnm_core_aux_extern, libnm_core_aux_intern, libnm_base, + libnm_log_null, libnm_glib_aux, libnm_std_aux, libc_siphash, From bec4a40437e4120cf115771b03f4aa5f0fa5fef4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 30 Jun 2021 23:12:26 +0200 Subject: [PATCH 2/7] glib-aux: add nm_utils_thread_local_register_destroy() helper _nm_thread_local is very neat, but when we allocate resources we need to make sure that they are destroyed when the thread exits. We can use pthread_setspecific() for that, but using it is cumbersome. Add a helper function to make that simpler. Also, the number of possible pthread_key_t keys is limited. With this way, we only need one key in total. --- src/libnm-glib-aux/nm-shared-utils.c | 79 ++++++++++++++++++++++++++++ src/libnm-glib-aux/nm-shared-utils.h | 4 ++ 2 files changed, 83 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.c b/src/libnm-glib-aux/nm-shared-utils.c index 68598728cb..1b4c3cbc4f 100644 --- a/src/libnm-glib-aux/nm-shared-utils.c +++ b/src/libnm-glib-aux/nm-shared-utils.c @@ -14,7 +14,9 @@ #include #include #include +#include +#include "c-list/src/c-list.h" #include "nm-errno.h" #include "nm-str-buf.h" @@ -6385,3 +6387,80 @@ nm_utils_validate_hostname(const char *hostname) return (p - hostname <= HOST_NAME_MAX); } + +/*****************************************************************************/ + +typedef struct { + CList lst; + gpointer tls_data; + GDestroyNotify destroy_notify; +} TlsRegData; + +static pthread_key_t _tls_reg_key; + +static void +_tls_reg_destroy(gpointer data) +{ + CList * lst_head = data; + TlsRegData *entry; + + if (!lst_head) + return; + + /* For no strong reason are we destroying the elements in reverse + * order than they were added. It seems a bit more sensible (but shouldn't + * matter nor should you rely on that). */ + while ((entry = c_list_last_entry(lst_head, TlsRegData, lst))) { + c_list_unlink_stale(&entry->lst); + entry->destroy_notify(entry->tls_data); + nm_g_slice_free(entry); + } + + nm_g_slice_free(lst_head); +} + +static void +_tls_reg_make_key(void) +{ + if (pthread_key_create(&_tls_reg_key, _tls_reg_destroy) != 0) + g_return_if_reached(); +} + +/** + * nm_utils_thread_local_register_destroy: + * @tls_data: the thread local storage data that should be destroyed when the thread + * exits. This pointer will be "owned" by the current thread. There is no way + * to un-register the destruction. + * @destroy_notify: the free function that will be called when the thread exits. + * + * If _nm_tread_local storage is heap allocated it requires freeing the pointer + * when the thread exits. Use this function to register the pointer to be + * released. + * + * This function does not change errno. + */ +void +nm_utils_thread_local_register_destroy(gpointer tls_data, GDestroyNotify destroy_notify) +{ + NM_AUTO_PROTECT_ERRNO(errsv); + static pthread_once_t key_once = PTHREAD_ONCE_INIT; + CList * lst_head; + TlsRegData * entry; + + nm_assert(destroy_notify); + + if (pthread_once(&key_once, _tls_reg_make_key) != 0) + g_return_if_reached(); + + if ((lst_head = pthread_getspecific(_tls_reg_key)) == NULL) { + lst_head = g_slice_new(CList); + c_list_init(lst_head); + if (pthread_setspecific(_tls_reg_key, lst_head) != 0) + g_return_if_reached(); + } + + entry = g_slice_new(TlsRegData); + entry->tls_data = tls_data; + entry->destroy_notify = destroy_notify; + c_list_link_tail(lst_head, &entry->lst); +} diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 803a730c6a..06b7e164fa 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -2994,4 +2994,8 @@ char *nm_utils_get_process_exit_status_desc(int status); gboolean nm_utils_validate_hostname(const char *hostname); +/*****************************************************************************/ + +void nm_utils_thread_local_register_destroy(gpointer tls_data, GDestroyNotify destroy_notify); + #endif /* __NM_SHARED_UTILS_H__ */ From 5bc39d97834c5ba9d81e6f38e07f19eb4e9fd398 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 30 Jun 2021 23:18:22 +0200 Subject: [PATCH 3/7] glib-aux: fix releasing thread-local storage from nm_strerror_native() The previous implementation was just wrong. Fixes: e1ca3bf7ed40 ('shared: add nm_strerror_native() to replace strerror() and g_strerror()') --- src/libnm-glib-aux/nm-errno.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/src/libnm-glib-aux/nm-errno.c b/src/libnm-glib-aux/nm-errno.c index 283173e328..0426a21d40 100644 --- a/src/libnm-glib-aux/nm-errno.c +++ b/src/libnm-glib-aux/nm-errno.c @@ -7,8 +7,6 @@ #include "nm-errno.h" -#include - /*****************************************************************************/ static NM_UTILS_LOOKUP_STR_DEFINE( @@ -162,19 +160,9 @@ nm_strerror_native(int errsv) buf = buf_static; if (G_UNLIKELY(!buf)) { - int errno_saved = errno; - pthread_key_t key; - buf = g_malloc(NM_STRERROR_BUFSIZE); buf_static = buf; - - if (pthread_key_create(&key, g_free) != 0 || pthread_setspecific(key, buf) != 0) { - /* Failure. We will leak the buffer when the thread exits. - * - * Nothing we can do about it really. For Debug builds we fail with an assertion. */ - nm_assert_not_reached(); - } - errno = errno_saved; + nm_utils_thread_local_register_destroy(buf, g_free); } return nm_strerror_native_r(errsv, buf, NM_STRERROR_BUFSIZE); From b433c21ae47949329c973c0a58d6df16068bf629 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 30 Jun 2021 23:19:44 +0200 Subject: [PATCH 4/7] platform: fix releasing thead-local stack of NMPNetns instances Fixes: 12df49f8ab45 ('platform: make NMPNetns thread-safe') --- src/libnm-platform/nmp-netns.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/libnm-platform/nmp-netns.c b/src/libnm-platform/nmp-netns.c index aea5b3b6dc..2b28a4cd43 100644 --- a/src/libnm-platform/nmp-netns.c +++ b/src/libnm-platform/nmp-netns.c @@ -11,7 +11,6 @@ #include #include #include -#include #include "libnm-log-core/nm-logging.h" @@ -151,20 +150,13 @@ _netns_stack_get_impl(void) { gs_unref_object NMPNetns *netns = NULL; gs_free_error GError *error = NULL; - pthread_key_t key; GArray * s; s = g_array_new(FALSE, FALSE, sizeof(NetnsInfo)); g_array_set_clear_func(s, _netns_stack_clear_cb); _netns_stack = s; - /* register a destructor function to cleanup the array. If we fail - * to do so, we will leak NMPNetns instances (and their file descriptor) when the - * thread exits. */ - if (pthread_key_create(&key, (void (*)(void *)) g_array_unref) != 0) - _LOGE(NULL, "failure to initialize thread-local storage"); - else if (pthread_setspecific(key, s) != 0) - _LOGE(NULL, "failure to set thread-local storage"); + nm_utils_thread_local_register_destroy(s, (GDestroyNotify) g_array_unref); /* at the bottom of the stack we must try to create a netns instance * that we never pop. It's the base to which we need to return. */ From c127e1beccfd468b2b2bfcca2e7cb1d952068e17 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 30 Jun 2021 23:19:44 +0200 Subject: [PATCH 5/7] glib-aux: fix releasing thead-local GRand instance from nm_utils_random_bytes() Fixes: b01a453ca298 ('core: add nm_utils_random_bytes() and use getrandom()') --- src/libnm-glib-aux/nm-random-utils.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-random-utils.c b/src/libnm-glib-aux/nm-random-utils.c index 56b99d5e3c..b6f76b05a9 100644 --- a/src/libnm-glib-aux/nm-random-utils.c +++ b/src/libnm-glib-aux/nm-random-utils.c @@ -122,8 +122,10 @@ fd_open: */ has_high_quality = FALSE; - if (G_UNLIKELY(!rand)) + if (G_UNLIKELY(!rand)) { rand = g_rand_new(); + nm_utils_thread_local_register_destroy(rand, (GDestroyNotify) g_rand_free); + } nm_assert(n > 0); i = 0; From 3649efe2b574866f0a9fb5e38f368408b1547697 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 30 Jun 2021 22:26:20 +0200 Subject: [PATCH 6/7] glib-aux: put more effort into seeding GRand fallback for nm_utils_random_bytes() g_rand_new() reads /dev/urandom and falls back to timestamp and pid. At this point, we already unsuccessfully tried getrandom()/urandom, so that doesn't seem promising to try. Try harder to get good random seeds for our GRand instance. Have one global instance, that gets seeded with various things that come to mind. The random sequence of that instance is then used to initialize the thread-local GRand instances. Maybe this is all snake oil. If we fail to get good randomness by using kernel API, what can we do? But really, callers also don't know how they should handle a failure to get random data (short of abort() or logging), so there is value in nm_utils_random_bytes() trying really the best it can, and callers pretending that it doesn't fail. This aims to improve the fallback case. --- src/libnm-glib-aux/nm-random-utils.c | 94 +++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-random-utils.c b/src/libnm-glib-aux/nm-random-utils.c index b6f76b05a9..8c364cb543 100644 --- a/src/libnm-glib-aux/nm-random-utils.c +++ b/src/libnm-glib-aux/nm-random-utils.c @@ -8,6 +8,7 @@ #include "nm-random-utils.h" #include +#include #if USE_SYS_RANDOM_H #include @@ -16,9 +17,100 @@ #endif #include "nm-shared-utils.h" +#include "nm-time-utils.h" /*****************************************************************************/ +#define SEED_ARRAY_SIZE (16 + 2 + 4 + 2 + 3) + +static guint32 +_pid_hash(pid_t id) +{ + if (sizeof(pid_t) > sizeof(guint32)) + return (((guint64) id) >> 32) ^ ((guint64) id); + return id; +} + +static void +_rand_init_seed(guint32 seed_array[static SEED_ARRAY_SIZE], GRand *rand) +{ + int seed_idx; + const guint8 *p_at_random; + guint64 now_nsec; + + /* Get some seed material from the provided GRand. */ + for (seed_idx = 0; seed_idx < 16; seed_idx++) + seed_array[seed_idx] = g_rand_int(rand); + + /* Add an address from the heap. */ + seed_array[seed_idx++] = ((guint64) ((uintptr_t) ((gpointer) rand))) >> 32; + seed_array[seed_idx++] = ((guint64) ((uintptr_t) ((gpointer) rand))); + + /* Add the per-process, random number. */ + p_at_random = ((gpointer) getauxval(AT_RANDOM)); + if (p_at_random) { + memcpy(&seed_array[seed_idx], p_at_random, 16); + } else + memset(&seed_array[seed_idx], 0, 16); + G_STATIC_ASSERT_EXPR(sizeof(guint32) == 4); + seed_idx += 4; + + /* Add the current timestamp, the pid and ppid. */ + now_nsec = nm_utils_clock_gettime_nsec(CLOCK_BOOTTIME); + seed_array[seed_idx++] = ((guint64) now_nsec) >> 32; + seed_array[seed_idx++] = ((guint64) now_nsec); + seed_array[seed_idx++] = _pid_hash(getpid()); + seed_array[seed_idx++] = _pid_hash(getppid()); + seed_array[seed_idx++] = _pid_hash(gettid()); + + nm_assert(seed_idx == SEED_ARRAY_SIZE); +} + +static GRand * +_rand_create_thread_local(void) +{ + G_LOCK_DEFINE_STATIC(global_rand); + static GRand *global_rand = NULL; + guint32 seed_array[SEED_ARRAY_SIZE]; + + /* We use thread-local instances of GRand to create a series of + * "random" numbers. We use thread-local instances, so that we don't + * require additional locking except the first time. + * + * We trust that once seeded, a GRand gives us a good enough stream of + * random numbers. If that wouldn't be the case, then maybe GRand should + * be fixed. + * Also, we tell our callers that the numbers from GRand are not good. + * But that isn't gonna help, because callers have no other way to get + * better random numbers, so usually the just ignore the failure and make + * the best of it. + * + * That means, the remaining problem is to seed the instance well. + * Note that we are already in a situation where getrandom() failed + * to give us good random numbers. So we can not do much to get reliably + * good entropy for the seed. */ + + G_LOCK(global_rand); + + if (G_UNLIKELY(!global_rand)) { + GRand *rand1; + + /* g_rand_new() reads /dev/urandom, but we already noticed that + * /dev/urandom fails to give us good randomness (which is why + * we hit this code path). So this may not be as good as we wish, + * but let's add it to the mix. */ + rand1 = g_rand_new(); + _rand_init_seed(seed_array, rand1); + global_rand = g_rand_new_with_seed_array(seed_array, SEED_ARRAY_SIZE); + g_rand_free(rand1); + } + + _rand_init_seed(seed_array, global_rand); + G_UNLOCK(global_rand); + + return g_rand_new_with_seed_array(seed_array, SEED_ARRAY_SIZE); +} + /** * nm_utils_random_bytes: * @p: the buffer to fill @@ -123,7 +215,7 @@ fd_open: has_high_quality = FALSE; if (G_UNLIKELY(!rand)) { - rand = g_rand_new(); + rand = _rand_create_thread_local(); nm_utils_thread_local_register_destroy(rand, (GDestroyNotify) g_rand_free); } From 94121a1b48b67ede0ebe8dec336da18e05d403eb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 30 Jun 2021 23:56:26 +0200 Subject: [PATCH 7/7] glib-aux: avoid accessing thread-local variable in a loop Dunno whether the compiler can optimize this out. Assign to an auto variable. --- src/libnm-glib-aux/nm-random-utils.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libnm-glib-aux/nm-random-utils.c b/src/libnm-glib-aux/nm-random-utils.c index 8c364cb543..f622b28411 100644 --- a/src/libnm-glib-aux/nm-random-utils.c +++ b/src/libnm-glib-aux/nm-random-utils.c @@ -203,7 +203,8 @@ fd_open: } if (!urandom_success) { - static _nm_thread_local GRand *rand = NULL; + static _nm_thread_local GRand *rand_tls = NULL; + GRand * rand; gsize i; int j; @@ -214,8 +215,10 @@ fd_open: */ has_high_quality = FALSE; + rand = rand_tls; if (G_UNLIKELY(!rand)) { - rand = _rand_create_thread_local(); + rand = _rand_create_thread_local(); + rand_tls = rand; nm_utils_thread_local_register_destroy(rand, (GDestroyNotify) g_rand_free); }