From e862f34dd8292f6ffcd30e20c22cd04d47531c3a Mon Sep 17 00:00:00 2001 From: Alexander Slobodeniuk Date: Tue, 14 Nov 2023 20:20:11 +0100 Subject: [PATCH] gmessages: introduce g_log_writer_default_set_debug_domains() The problem is that resetting the environment of a proccess is not safe, this is mentioned in the man pages of the setenv, and also by the concept: when we get a constant string returned by the getenv, we need to be guaranteed that this string will exist during all that time we are using it. While setting the same env var to another value destroys this string inside of libc. Now looking at the should_drop_message(), we can see that it just gets the environment on every call. Getting the environment is safe, but however there's no safe way to switch the logging domains selection: setenv can't be used because of the reasons listed above. That is why g_log_writer_default_set_debug_domains() is needed: it's just a safe replacement for the setenv in this case. Now should_drop_message() still reads the environment, but does so only on the first call, after that the domains are stored internally, and can only be changed by g_log_writer_default_set_debug_domains(). This also means that now resetting G_MESSAGES_DEBUG env var in runtime has no effect. But in any case this is not what the user should do, because resetting the environment in runtime is not correct. --- glib/gmessages.c | 79 ++++++++++--- glib/gmessages.h | 3 + glib/tests/logging.c | 269 ++++++++++++++++++++++++++++++++++--------- tools/glib.supp | 44 +++++++ 4 files changed, 326 insertions(+), 69 deletions(-) diff --git a/glib/gmessages.c b/glib/gmessages.c index ebd3a5433..cad2faadf 100644 --- a/glib/gmessages.c +++ b/glib/gmessages.c @@ -310,7 +310,8 @@ * * Such messages are suppressed by the g_log_default_handler() and * g_log_writer_default() unless the `G_MESSAGES_DEBUG` environment variable is - * set appropriately. + * set appropriately. If you need to set the allowed domains at runtime, use + * g_log_writer_default_set_debug_domains(). * * If structured logging is enabled, this will use g_log_structured(); * otherwise it will use g_log(). See @@ -333,7 +334,8 @@ * * Such messages are suppressed by the g_log_default_handler() and * g_log_writer_default() unless the `G_MESSAGES_DEBUG` environment variable is - * set appropriately. + * set appropriately. If you need to set the allowed domains at runtime, use + * g_log_writer_default_set_debug_domains(). * * If structured logging is enabled, this will use g_log_structured(); * otherwise it will use g_log(). See @@ -2485,6 +2487,36 @@ domain_found (const gchar *domains, return FALSE; } +static struct { + GRWLock lock; + gchar *domains; + gboolean domains_set; +} g_log_global; + +/** + * g_log_writer_default_set_debug_domains: + * @domains: (nullable) (transfer none): `NULL`-terminated array with domains to be printed. + * `NULL` or an array with no values means none. Array with a single value `"all"` means all. + + * Reset the list of domains to be logged, that might be initially set by the + * `G_MESSAGES_DEBUG` environment variable. This function is thread-safe. + * + * Since: 2.80 + */ +void +g_log_writer_default_set_debug_domains (const gchar * const *domains) +{ + g_rw_lock_writer_lock (&g_log_global.lock); + + g_free (g_log_global.domains); + g_log_global.domains = domains ? + g_strjoinv (" ", (gchar **)domains) : NULL; + + g_log_global.domains_set = TRUE; + + g_rw_lock_writer_unlock (&g_log_global.lock); +} + /* * Internal version of g_log_writer_default_would_drop(), which can * read from either a log_domain or an array of fields. This avoids @@ -2502,14 +2534,22 @@ should_drop_message (GLogLevelFlags log_level, !(log_level >> G_LOG_LEVEL_USER_SHIFT) && !g_log_get_debug_enabled ()) { - const gchar *domains; gsize i; - domains = g_getenv ("G_MESSAGES_DEBUG"); + g_rw_lock_reader_lock (&g_log_global.lock); + + if (G_UNLIKELY (!g_log_global.domains_set)) + { + g_log_global.domains = g_strdup (g_getenv ("G_MESSAGES_DEBUG")); + g_log_global.domains_set = TRUE; + } if ((log_level & INFO_LEVELS) == 0 || - domains == NULL) - return TRUE; + g_log_global.domains == NULL) + { + g_rw_lock_reader_unlock (&g_log_global.lock); + return TRUE; + } if (log_domain == NULL) { @@ -2523,9 +2563,14 @@ should_drop_message (GLogLevelFlags log_level, } } - if (strcmp (domains, "all") != 0 && - (log_domain == NULL || !domain_found (domains, log_domain))) - return TRUE; + if (strcmp (g_log_global.domains, "all") != 0 && + (log_domain == NULL || !domain_found (g_log_global.domains, log_domain))) + { + g_rw_lock_reader_unlock (&g_log_global.lock); + return TRUE; + } + + g_rw_lock_reader_unlock (&g_log_global.lock); } return FALSE; @@ -2542,7 +2587,7 @@ should_drop_message (GLogLevelFlags log_level, * * As with g_log_default_handler(), this function drops debug and informational * messages unless their log domain (or `all`) is listed in the space-separated - * `G_MESSAGES_DEBUG` environment variable. + * `G_MESSAGES_DEBUG` environment variable, or by g_log_writer_default_set_debug_domains(). * * This can be used when implementing log writers with the same filtering * behaviour as the default, but a different destination or output format: @@ -2599,7 +2644,7 @@ g_log_writer_default_would_drop (GLogLevelFlags log_level, * * As with g_log_default_handler(), this function drops debug and informational * messages unless their log domain (or `all`) is listed in the space-separated - * `G_MESSAGES_DEBUG` environment variable. + * `G_MESSAGES_DEBUG` environment variable, or set at runtime by g_log_writer_default_set_debug_domains(). * * g_log_writer_default() uses the mask set by g_log_set_always_fatal() to * determine which messages are fatal. When using a custom writer func instead it is @@ -2740,8 +2785,8 @@ _g_log_writer_fallback (GLogLevelFlags log_level, * other logging functions; it should only be used from %GLogWriterFunc * implementations. * - * Note also that the value of this does not depend on `G_MESSAGES_DEBUG`; see - * the docs for g_log_set_debug_enabled(). + * Note also that the value of this does not depend on `G_MESSAGES_DEBUG`, nor + * g_log_writer_default_set_debug_domains(); see the docs for g_log_set_debug_enabled(). * * Returns: %TRUE if debug output is enabled, %FALSE otherwise * @@ -2758,8 +2803,9 @@ g_log_get_debug_enabled (void) * @enabled: %TRUE to enable debug output, %FALSE otherwise * * Enable or disable debug output from the GLib logging system for all domains. - * This value interacts disjunctively with `G_MESSAGES_DEBUG` — if either of - * them would allow a debug message to be outputted, it will be. + * This value interacts disjunctively with `G_MESSAGES_DEBUG` and + * g_log_writer_default_set_debug_domains() — if any of them would allow + * a debug message to be outputted, it will be. * * Note that this should not be used from within library code to enable debug * output — it is intended for external use. @@ -3086,7 +3132,8 @@ escape_string (GString *string) * * - `G_MESSAGES_DEBUG`: A space-separated list of log domains for * which debug and informational messages are printed. By default - * these messages are not printed. + * these messages are not printed. If you need to set the allowed + * domains at runtime, use g_log_writer_default_set_debug_domains(). * * stderr is used for levels %G_LOG_LEVEL_ERROR, %G_LOG_LEVEL_CRITICAL, * %G_LOG_LEVEL_WARNING and %G_LOG_LEVEL_MESSAGE. stdout is used for diff --git a/glib/gmessages.h b/glib/gmessages.h index eab6d0678..47ba06771 100644 --- a/glib/gmessages.h +++ b/glib/gmessages.h @@ -250,6 +250,9 @@ void g_log_writer_default_set_use_stderr (gboolean use_stderr); GLIB_AVAILABLE_IN_2_68 gboolean g_log_writer_default_would_drop (GLogLevelFlags log_level, const char *log_domain); +GLIB_AVAILABLE_IN_2_80 +void g_log_writer_default_set_debug_domains (const gchar * const *domains); + /* G_MESSAGES_DEBUG enablement */ GLIB_AVAILABLE_IN_2_72 diff --git a/glib/tests/logging.c b/glib/tests/logging.c index f4c47e16c..858cad8ce 100644 --- a/glib/tests/logging.c +++ b/glib/tests/logging.c @@ -201,6 +201,71 @@ test_default_handler_debug_stderr (void) exit (0); } +static void +test_default_handler_would_drop_env5 (void) +{ + g_setenv ("G_MESSAGES_DEBUG", "foobar", TRUE); + + g_assert_true (g_log_writer_default_would_drop (G_LOG_LEVEL_DEBUG, "foo")); + g_assert_true (g_log_writer_default_would_drop (G_LOG_LEVEL_DEBUG, "bar")); +} + +static void +test_default_handler_would_drop_env4 (void) +{ + g_setenv ("G_MESSAGES_DEBUG", "all", TRUE); + + g_assert_false (g_log_writer_default_would_drop (G_LOG_LEVEL_ERROR, "foo")); + g_assert_false (g_log_writer_default_would_drop (G_LOG_LEVEL_CRITICAL, "foo")); + g_assert_false (g_log_writer_default_would_drop (G_LOG_LEVEL_WARNING, "foo")); + g_assert_false (g_log_writer_default_would_drop (G_LOG_LEVEL_MESSAGE, "foo")); + g_assert_false (g_log_writer_default_would_drop (G_LOG_LEVEL_INFO, "foo")); + g_assert_false (g_log_writer_default_would_drop (G_LOG_LEVEL_DEBUG, "foo")); + g_assert_false (g_log_writer_default_would_drop (1< 0) + g_log_writer_default_would_drop (G_LOG_LEVEL_DEBUG, test_would_drop_robustness_random_domain ()); + + g_atomic_int_set (&test_would_drop_robustness_stopping, TRUE); + for (i = 0; i < G_N_ELEMENTS (threads); i++) + g_thread_join (threads[i]); +} + static void test_default_handler_0x400 (void) { @@ -378,6 +517,24 @@ test_default_handler (void) g_test_trap_subprocess ("/logging/default-handler/subprocess/would-drop", 0, G_TEST_SUBPROCESS_DEFAULT); g_test_trap_assert_passed (); + g_test_trap_subprocess ("/logging/default-handler/subprocess/would-drop-env1", 0, + G_TEST_SUBPROCESS_DEFAULT); + g_test_trap_assert_passed (); + g_test_trap_subprocess ("/logging/default-handler/subprocess/would-drop-env2", 0, + G_TEST_SUBPROCESS_DEFAULT); + g_test_trap_assert_passed (); + g_test_trap_subprocess ("/logging/default-handler/subprocess/would-drop-env3", 0, + G_TEST_SUBPROCESS_DEFAULT); + g_test_trap_assert_passed (); + g_test_trap_subprocess ("/logging/default-handler/subprocess/would-drop-env4", 0, + G_TEST_SUBPROCESS_DEFAULT); + g_test_trap_assert_passed (); + g_test_trap_subprocess ("/logging/default-handler/subprocess/would-drop-env5", 0, + G_TEST_SUBPROCESS_DEFAULT); + g_test_trap_assert_passed (); + g_test_trap_subprocess ("/logging/default-handler/subprocess/would-drop-robustness", 0, + G_TEST_SUBPROCESS_DEFAULT); + g_test_trap_assert_passed (); } static void @@ -911,6 +1068,12 @@ main (int argc, char *argv[]) g_test_add_func ("/logging/default-handler/subprocess/debug-stderr", test_default_handler_debug_stderr); g_test_add_func ("/logging/default-handler/subprocess/0x400", test_default_handler_0x400); g_test_add_func ("/logging/default-handler/subprocess/would-drop", test_default_handler_would_drop); + g_test_add_func ("/logging/default-handler/subprocess/would-drop-env1", test_default_handler_would_drop_env1); + g_test_add_func ("/logging/default-handler/subprocess/would-drop-env2", test_default_handler_would_drop_env2); + g_test_add_func ("/logging/default-handler/subprocess/would-drop-env3", test_default_handler_would_drop_env3); + g_test_add_func ("/logging/default-handler/subprocess/would-drop-env4", test_default_handler_would_drop_env4); + g_test_add_func ("/logging/default-handler/subprocess/would-drop-env5", test_default_handler_would_drop_env5); + g_test_add_func ("/logging/default-handler/subprocess/would-drop-robustness", test_default_handler_would_drop_robustness); g_test_add_func ("/logging/warnings", test_warnings); g_test_add_func ("/logging/fatal-log-mask", test_fatal_log_mask); g_test_add_func ("/logging/set-handler", test_set_handler); diff --git a/tools/glib.supp b/tools/glib.supp index cb379e2f5..bddfe603e 100644 --- a/tools/glib.supp +++ b/tools/glib.supp @@ -1278,3 +1278,47 @@ ... fun:xdg_mime_init } + +# One-time allocations for default log writer lock and domains +{ + should_drop_message_rw_lock + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + fun:g_rw_lock_impl_new + fun:g_rw_lock_get_impl + fun:g_rw_lock_reader_lock + fun:should_drop_message +} + +{ + should_drop_message_strdup + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + fun:g_malloc + fun:g_strdup + fun:g_strdup_inline + fun:should_drop_message +} + +{ + g_log_writer_default_set_debug_strdup + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + fun:g_malloc + fun:g_strdup_inline + fun:g_log_writer_default_set_debug_domains +} + +{ + g_log_writer_default_set_debug_rw_lock + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + fun:g_rw_lock_impl_new + fun:g_rw_lock_get_impl + fun:g_rw_lock_writer_lock + fun:g_log_writer_default_set_debug_domains +}