From 138c187376cff1c14c66a3fdc502d260178081bb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 14 May 2019 13:37:35 +0200 Subject: [PATCH 1/5] logging: fix stack overflow in logging for iov_data array This overflow could only happen when we would try to log a message with "NM_DEVICE=", "NM_CONNECTION=", and more than 8 logging domains (_NUM_MAX_FIELDS_SYSLOG_FACILITY - 2). The latter is never the case. While we sometimes log messages with more than one logging domain, there are no logging statements that use most as 8 different logging domains. So, this overflow is not actually reachable from current code (I think). Fixes: ed552c732c76 ('logging: log device and connection along with the message'): --- src/nm-logging.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nm-logging.c b/src/nm-logging.c index 79e3658ab4..724fd004f2 100644 --- a/src/nm-logging.c +++ b/src/nm-logging.c @@ -747,7 +747,7 @@ _nm_log_impl (const char *file, { gint64 now, boottime; #define _NUM_MAX_FIELDS_SYSLOG_FACILITY 10 - struct iovec iov_data[12 + _NUM_MAX_FIELDS_SYSLOG_FACILITY]; + struct iovec iov_data[14 + _NUM_MAX_FIELDS_SYSLOG_FACILITY]; struct iovec *iov = iov_data; gpointer iov_free_data[5]; gpointer *iov_free = iov_free_data; From 467ac96dd1e71d3c3868671001cca4fb5954dc17 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 14 May 2019 13:40:04 +0200 Subject: [PATCH 2/5] logging: use char pointer for iov_free in _nm_log_impl() --- src/nm-logging.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nm-logging.c b/src/nm-logging.c index 724fd004f2..7b6b2f7a12 100644 --- a/src/nm-logging.c +++ b/src/nm-logging.c @@ -629,7 +629,7 @@ _iovec_set_string (struct iovec *iov, const char *str) _nm_printf (3, 4) static void -_iovec_set_format (struct iovec *iov, gpointer *iov_free, const char *format, ...) +_iovec_set_format (struct iovec *iov, char **iov_free, const char *format, ...) { va_list ap; char *str; @@ -749,8 +749,8 @@ _nm_log_impl (const char *file, #define _NUM_MAX_FIELDS_SYSLOG_FACILITY 10 struct iovec iov_data[14 + _NUM_MAX_FIELDS_SYSLOG_FACILITY]; struct iovec *iov = iov_data; - gpointer iov_free_data[5]; - gpointer *iov_free = iov_free_data; + char *iov_free_data[5]; + char **iov_free = iov_free_data; nm_auto_free_gstring GString *s_domain_all = NULL; now = nm_utils_get_monotonic_timestamp_ns (); From cc2553e871ca7cff7e4e3f5d92321a6789731b4d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 14 May 2019 12:28:22 +0200 Subject: [PATCH 3/5] logging: don't misuse SYSLOG_FACILITY field in journal Syslog's "facility" is a well defined thing and must be one of a few well-known numbers. Don't re-use it for our own purposes. Fixes: 1b808d3b255c ('logging: add native systemd-journald support to nm-logging') https://bugzilla.redhat.com/show_bug.cgi?id=1709741 --- src/nm-logging.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/nm-logging.c b/src/nm-logging.c index 7b6b2f7a12..4fea7595f7 100644 --- a/src/nm-logging.c +++ b/src/nm-logging.c @@ -627,6 +627,8 @@ _iovec_set_string (struct iovec *iov, const char *str) _iovec_set (iov, str, strlen (str)); } +#define _iovec_set_string_literal(iov, str) _iovec_set ((iov), ""str"", NM_STRLEN (str)) + _nm_printf (3, 4) static void _iovec_set_format (struct iovec *iov, char **iov_free, const char *format, ...) @@ -746,8 +748,7 @@ _nm_log_impl (const char *file, case LOG_BACKEND_JOURNAL: { gint64 now, boottime; -#define _NUM_MAX_FIELDS_SYSLOG_FACILITY 10 - struct iovec iov_data[14 + _NUM_MAX_FIELDS_SYSLOG_FACILITY]; + struct iovec iov_data[15]; struct iovec *iov = iov_data; char *iov_free_data[5]; char **iov_free = iov_free_data; @@ -762,12 +763,10 @@ _nm_log_impl (const char *file, _iovec_set_format_a (iov++, 30, "SYSLOG_PID=%ld", (long) getpid ()); { const LogDesc *diter; - int i_domain = _NUM_MAX_FIELDS_SYSLOG_FACILITY; const char *s_domain_1 = NULL; NMLogDomain dom_all = domain; - NMLogDomain dom = dom_all & cur_log_state[level]; - for (diter = &domain_desc[0]; diter->name; diter++) { + for (diter = &domain_desc[0]; dom_all != 0 && diter->name; diter++) { if (!NM_FLAGS_ANY (dom_all, diter->num)) continue; @@ -785,23 +784,14 @@ _nm_log_impl (const char *file, g_string_append_c (s_domain_all, ','); g_string_append (s_domain_all, diter->name); } - - if (NM_FLAGS_ANY (dom, diter->num)) { - if (i_domain > 0) { - /* SYSLOG_FACILITY is specified multiple times for each domain that is actually enabled. */ - _iovec_set_format_str_a (iov++, 30, "SYSLOG_FACILITY=%s", diter->name); - i_domain--; - } - dom &= ~diter->num; - } - if (!dom && !dom_all) - break; } if (s_domain_all) _iovec_set (iov++, s_domain_all->str, s_domain_all->len); else _iovec_set_format_str_a (iov++, 30, "NM_LOG_DOMAINS=%s", s_domain_1); } + G_STATIC_ASSERT_EXPR (LOG_FAC (LOG_DAEMON) == 3); + _iovec_set_string_literal (iov++, "SYSLOG_FACILITY=3"); _iovec_set_format_str_a (iov++, 15, "NM_LOG_LEVEL=%s", level_desc[level].name); if (func) _iovec_set_format (iov++, iov_free++, "CODE_FUNC=%s", func); @@ -916,7 +906,7 @@ nm_log_handler (const char *log_domain, "MESSAGE=%s%s", gl.imm.prefix, message ?: "", syslog_identifier_full (gl.imm.syslog_identifier), "SYSLOG_PID=%ld", (long) getpid (), - "SYSLOG_FACILITY=GLIB", + "SYSLOG_FACILITY=3", "GLIB_DOMAIN=%s", log_domain ?: "", "GLIB_LEVEL=%d", (int) (level & G_LOG_LEVEL_MASK), "TIMESTAMP_MONOTONIC=%lld.%06lld", (long long) (now / NM_UTILS_NS_PER_SECOND), (long long) ((now % NM_UTILS_NS_PER_SECOND) / 1000), From 9cddb9f8bd77325935296e75cbc03499cfa0a1b7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 14 May 2019 12:56:44 +0200 Subject: [PATCH 4/5] logging: use static buffer in nm_logging_all_domains_to_string() Don't create a heap allocated GString to hold the static result of nm_logging_all_domains_to_string(). Instead, use a static buffer of the exactly required size. The main reason to do this, is to get the exact size of "_all_logging_domains_to_str" buffer. This is the upper boundary for the size of a string buffer to hold all domain names. We will need that boundary in the next commit. The attractive thing here is that we will have a unit-test failure if this boundery no longer matches (--with-more-asserts). That means, this boundary is guarded by unit tests and we don't accidentally get it wrong when the domains change. Also, take care to initialize the buffer in a thread-safe manner. It's easy enough to get right, so there is no excuse for having non-thread-safe code in logging. --- src/nm-logging.c | 41 +++++++++++++++++++++++++++++++--------- src/tests/test-general.c | 13 +++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/nm-logging.c b/src/nm-logging.c index 4fea7595f7..eeffeb657f 100644 --- a/src/nm-logging.c +++ b/src/nm-logging.c @@ -549,27 +549,50 @@ _domains_to_string (gboolean include_level_override, return g_string_free (str, FALSE); } +static char _all_logging_domains_to_str[273]; + const char * nm_logging_all_domains_to_string (void) { - static GString *str; + static const char *volatile str = NULL; + const char *s; - if (G_UNLIKELY (!str)) { +again: + s = g_atomic_pointer_get (&str); + if (G_UNLIKELY (!s)) { + static gsize once = 0; const LogDesc *diter; + gsize buf_l; + char *buf_p; - str = g_string_new (LOGD_DEFAULT_STRING); + if (!g_once_init_enter (&once)) + goto again; + + buf_p = _all_logging_domains_to_str; + buf_l = sizeof (_all_logging_domains_to_str); + + nm_utils_strbuf_append_str (&buf_p, &buf_l, LOGD_DEFAULT_STRING); for (diter = &domain_desc[0]; diter->name; diter++) { - g_string_append_c (str, ','); - g_string_append (str, diter->name); + nm_utils_strbuf_append_c (&buf_p, &buf_l, ','); + nm_utils_strbuf_append_str (&buf_p, &buf_l, diter->name); if (diter->num == LOGD_DHCP6) - g_string_append (str, "," LOGD_DHCP_STRING); + nm_utils_strbuf_append_str (&buf_p, &buf_l, ","LOGD_DHCP_STRING); else if (diter->num == LOGD_IP6) - g_string_append (str, "," LOGD_IP_STRING); + nm_utils_strbuf_append_str (&buf_p, &buf_l, ","LOGD_IP_STRING); } - g_string_append (str, "," LOGD_ALL_STRING); + nm_utils_strbuf_append_str (&buf_p, &buf_l, LOGD_ALL_STRING); + + /* Did you modify the logging domains (or their names)? Adjust the size of + * _all_logging_domains_to_str buffer above to have the exact size. */ + nm_assert (strlen (_all_logging_domains_to_str) == sizeof (_all_logging_domains_to_str) - 1); + nm_assert (buf_l == 1); + + s = _all_logging_domains_to_str; + g_atomic_pointer_set (&str, s); + g_once_init_leave (&once, 1); } - return str->str; + return s; } /** diff --git a/src/tests/test-general.c b/src/tests/test-general.c index 0dee566eaa..cedf5b8a9a 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -108,6 +108,17 @@ test_nm_utils_ip6_address_clear_host_address (void) /*****************************************************************************/ +static void +test_logging_domains (void) +{ + const char *s; + + s = nm_logging_all_domains_to_string (); + g_assert (s && s[0]); +} + +/*****************************************************************************/ + static void _test_same_prefix (const char *a1, const char *a2, guint8 plen) { @@ -2180,6 +2191,8 @@ main (int argc, char **argv) { nmtst_init_with_logging (&argc, &argv, NULL, "ALL"); + g_test_add_func ("/general/test_logging_domains", test_logging_domains); + g_test_add_func ("/general/nm_utils_strbuf_append", test_nm_utils_strbuf_append); g_test_add_func ("/general/nm_utils_ip6_address_clear_host_address", test_nm_utils_ip6_address_clear_host_address); From f8cbf06a77cbe5ef6d24a6f6b35f7b5607a2f530 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 14 May 2019 12:39:27 +0200 Subject: [PATCH 5/5] logging: use stack allocated string buffer to constuct NM_LOG_DOMAINS field NM_LOG_DOMAINS is a comma-separated list of the selected logging domains. As the number of all logging domains is fixed at compile-time, the maximum length of the buffer is known. $ git grep $'^\t{ LOGD_' | sed 's/.*"\(.*\)" .*/\1/' | xargs | sed 's/ */,/g' | sed 's/^/NM_LOG_DOMAINS=/' NM_LOG_DOMAINS=PLATFORM,RFKILL,ETHER,WIFI,BT,MB,DHCP4,DHCP6,PPP,WIFI_SCAN,IP4,IP6,AUTOIP4,DNS,VPN,SHARING,SUPPLICANT,AGENTS,SETTINGS,SUSPEND,CORE,DEVICE,OLPC,INFINIBAND,FIREWALL,ADSL,BOND,VLAN,BRIDGE,DBUS_PROPS,TEAM,CONCHECK,DCB,DISPATCH,AUDIT,SYSTEMD,VPN_PLUGIN,PROXY Note that the static buffer "_all_logging_domains_to_str" is known to be large enough to contain these domain names (it's even longer, as it also contains "ALL", "IP", and "DHCP" alises). We can use that to define the array of suitable size. --- src/nm-logging.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/nm-logging.c b/src/nm-logging.c index eeffeb657f..dfc6a19dbc 100644 --- a/src/nm-logging.c +++ b/src/nm-logging.c @@ -775,7 +775,6 @@ _nm_log_impl (const char *file, struct iovec *iov = iov_data; char *iov_free_data[5]; char **iov_free = iov_free_data; - nm_auto_free_gstring GString *s_domain_all = NULL; now = nm_utils_get_monotonic_timestamp_ns (); boottime = nm_utils_monotonic_timestamp_as_boottime (now, 1); @@ -786,32 +785,22 @@ _nm_log_impl (const char *file, _iovec_set_format_a (iov++, 30, "SYSLOG_PID=%ld", (long) getpid ()); { const LogDesc *diter; - const char *s_domain_1 = NULL; NMLogDomain dom_all = domain; + char s_log_domains_buf[NM_STRLEN ("NM_LOG_DOMAINS=") + sizeof (_all_logging_domains_to_str)]; + char *s_log_domains = s_log_domains_buf; + gsize l_log_domains = sizeof (s_log_domains_buf); + nm_utils_strbuf_append_str (&s_log_domains, &l_log_domains, "NM_LOG_DOMAINS="); for (diter = &domain_desc[0]; dom_all != 0 && diter->name; diter++) { if (!NM_FLAGS_ANY (dom_all, diter->num)) continue; - - /* construct a list of all domains (not only the enabled ones). - * Note that in by far most cases, there is only one domain present. - * Hence, save the construction of the GString. */ + if (dom_all != domain) + nm_utils_strbuf_append_c (&s_log_domains, &l_log_domains, ','); + nm_utils_strbuf_append_str (&s_log_domains, &l_log_domains, diter->name); dom_all &= ~diter->num; - if (!s_domain_1) - s_domain_1 = diter->name; - else { - if (!s_domain_all) { - s_domain_all = g_string_new ("NM_LOG_DOMAINS="); - g_string_append (s_domain_all, s_domain_1); - } - g_string_append_c (s_domain_all, ','); - g_string_append (s_domain_all, diter->name); - } } - if (s_domain_all) - _iovec_set (iov++, s_domain_all->str, s_domain_all->len); - else - _iovec_set_format_str_a (iov++, 30, "NM_LOG_DOMAINS=%s", s_domain_1); + nm_assert (l_log_domains > 0); + _iovec_set (iov++, s_log_domains_buf, s_log_domains - s_log_domains_buf); } G_STATIC_ASSERT_EXPR (LOG_FAC (LOG_DAEMON) == 3); _iovec_set_string_literal (iov++, "SYSLOG_FACILITY=3");