From d506823d4f21f67d2c388e79982ebac170bce3e3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:50:53 +0200 Subject: [PATCH 1/9] initrd: fix setting VLan ID in reader_parse_vlan() g_ascii_strtoull() returns a guint64, which is very wrong to directly pass to the variadic argument list of g_object_set(). We expect a guint there and need to cast. While at it, use _nm_utils_ascii_str_to_int64() to parse and validate the input. --- src/initrd/nmi-cmdline-reader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/initrd/nmi-cmdline-reader.c b/src/initrd/nmi-cmdline-reader.c index 41b063132..2eb6d7e5b 100644 --- a/src/initrd/nmi-cmdline-reader.c +++ b/src/initrd/nmi-cmdline-reader.c @@ -719,7 +719,7 @@ reader_parse_vlan (Reader *reader, char *argument) s_vlan = nm_connection_get_setting_vlan (connection); g_object_set (s_vlan, NM_SETTING_VLAN_PARENT, phy, - NM_SETTING_VLAN_ID, g_ascii_strtoull (vlanid, NULL, 10), + NM_SETTING_VLAN_ID, (guint) _nm_utils_ascii_str_to_int64 (vlanid, 10, 0, G_MAXUINT, G_MAXUINT), NULL); if (argument && *argument) From 2e4771be5efb2b0e6820243929e55f4bf42c7cad Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 13:02:03 +0200 Subject: [PATCH 2/9] ifcfg: strip whitespaces around "DEVTIMEOUT" Be more graceful and allow whitespaces around the floating point number for DEVTIMEOUT. Note that _nm_utils_ascii_str_to_int64() is already graceful against whitespace, so also be it with the g_ascii_strtod() code path. --- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 0dc889408..f337d0d4d 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -550,16 +550,18 @@ make_connection_setting (const char *file, g_object_set (s_con, NM_SETTING_CONNECTION_AUTH_RETRIES, (int) vint64, NULL); nm_clear_g_free (&value); - v = svGetValueStr (ifcfg, "DEVTIMEOUT", &value); + v = svGetValue (ifcfg, "DEVTIMEOUT", &value); if (v) { + v = nm_str_skip_leading_spaces (v); vint64 = _nm_utils_ascii_str_to_int64 (v, 10, 0, ((gint64) G_MAXINT32) / 1000, -1); if (vint64 != -1) vint64 *= 1000; - else { + else if (v[0] != '\0') { char *endptr; double d; d = g_ascii_strtod (v, &endptr); + endptr = nm_str_skip_leading_spaces (endptr); if ( errno == 0 && endptr[0] == '\0' && d >= 0.0) { From 9cbf4c2825a7ce68c26080386fe72f5393706b87 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 13:14:18 +0200 Subject: [PATCH 3/9] ifcfg-rh/tests: add unit test for reading DEVTIMEOUT (connection.wait-device-timeout) --- .../ifcfg-rh/tests/network-scripts/ifcfg-test-wired-autoip | 1 + src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-autoip b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-autoip index dc47126ce..e683db3c7 100644 --- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-autoip +++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-autoip @@ -3,3 +3,4 @@ DEVICE=eth0 BOOTPROTO=autoip IPV4_FAILURE_FATAL=yes PEERDNS=no +DEVTIMEOUT=2.6 diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index 6db5f64a6..efa9ea9b8 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -1738,6 +1738,7 @@ static void test_read_wired_autoip (void) { gs_unref_object NMConnection *connection = NULL; + NMSettingConnection *s_con; NMSettingIPConfig *s_ip4; char *unmanaged = NULL; @@ -1751,6 +1752,9 @@ test_read_wired_autoip (void) g_assert_cmpstr (nm_setting_ip_config_get_method (s_ip4), ==, NM_SETTING_IP4_CONFIG_METHOD_LINK_LOCAL); g_assert (!nm_setting_ip_config_get_may_fail (s_ip4)); g_assert (nm_setting_ip_config_get_ignore_auto_dns (s_ip4)); + + s_con = nm_connection_get_setting_connection (connection); + g_assert_cmpint (nm_setting_connection_get_wait_device_timeout (s_con), ==, 2600); } static void From 3930ef194ec37d51eb74aed6e7e2ac403539bbfd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:30:12 +0200 Subject: [PATCH 4/9] ifupdown: use _nm_utils_ascii_str_to_int64() for converting netmask to string --- src/settings/plugins/ifupdown/nms-ifupdown-parser.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/settings/plugins/ifupdown/nms-ifupdown-parser.c b/src/settings/plugins/ifupdown/nms-ifupdown-parser.c index 64cd431d5..ac3ed1ce0 100644 --- a/src/settings/plugins/ifupdown/nms-ifupdown-parser.c +++ b/src/settings/plugins/ifupdown/nms-ifupdown-parser.c @@ -565,9 +565,8 @@ update_ip6_setting_from_if_block (NMConnection *connection, const char *nameserver_v; const char *nameservers_v; const char *search_v; - int prefix_int = 128; + guint prefix_int; - /* Address */ address_v = ifparser_getkey (block, "address"); if (!address_v) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, @@ -575,12 +574,12 @@ update_ip6_setting_from_if_block (NMConnection *connection, return FALSE; } - /* Prefix */ prefix_v = ifparser_getkey (block, "netmask"); if (prefix_v) - prefix_int = g_ascii_strtoll (prefix_v, NULL, 10); + prefix_int = _nm_utils_ascii_str_to_int64 (prefix_v, 10, 0, 128, G_MAXINT); + else + prefix_int = 128; - /* Add the new address to the setting */ addr = nm_ip_address_new (AF_INET6, address_v, prefix_int, error); if (!addr) return FALSE; @@ -593,7 +592,6 @@ update_ip6_setting_from_if_block (NMConnection *connection, } nm_ip_address_unref (addr); - /* gateway */ gateway_v = ifparser_getkey (block, "gateway"); if (gateway_v) { if (!nm_utils_ipaddr_is_valid (AF_INET6, gateway_v)) { @@ -614,7 +612,6 @@ update_ip6_setting_from_if_block (NMConnection *connection, if (!nm_setting_ip_config_get_num_dns (s_ip6)) _LOGI ("No dns-nameserver configured in /etc/network/interfaces"); - /* DNS searches */ search_v = ifparser_getkey (block, "dns-search"); if (search_v) { gs_free const char **list = NULL; From 659ac9cc1299399b34b4677a492c9943458e1b52 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:47:55 +0200 Subject: [PATCH 5/9] device/bluetooth: avoid g_ascii_strtoull() to parse capabilities Avoid g_ascii_strtoull() calling directly. It has subtle issues, which is why we have a wrapper for it. --- src/devices/bluetooth/nm-bluez-manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/devices/bluetooth/nm-bluez-manager.c b/src/devices/bluetooth/nm-bluez-manager.c index c24f1306f..6ff96c323 100644 --- a/src/devices/bluetooth/nm-bluez-manager.c +++ b/src/devices/bluetooth/nm-bluez-manager.c @@ -221,7 +221,7 @@ convert_uuids_to_capabilities (const char *const*strv) continue; s_part1 = g_strndup (str, s - str); - switch (g_ascii_strtoull (s_part1, NULL, 16)) { + switch (_nm_utils_ascii_str_to_int64 (s_part1, 16, 0, G_MAXINT, -1)) { case 0x1103: capabilities |= NM_BT_CAPABILITY_DUN; break; From f4446e34c689d6bd81deb8bbab01387716db801f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:26:20 +0200 Subject: [PATCH 6/9] shared: add nm_g_ascii_strtoll() to workaround bug --- shared/nm-glib-aux/nm-shared-utils.c | 66 ++++++++++++++++++++++++++++ shared/nm-glib-aux/nm-shared-utils.h | 4 ++ 2 files changed, 70 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 74abfb933..96c966f16 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -955,6 +955,72 @@ nm_utils_ipaddr_is_normalized (int addr_family, /*****************************************************************************/ +/** + * nm_g_ascii_strtoll() + * @nptr: the string to parse + * @endptr: the pointer on the first invalid chars + * @base: the base. + * + * This wraps g_ascii_strtoll() and should in almost all cases behave identical + * to it. + * + * However, it seems there are situations where g_ascii_strtoll() might set + * errno to some unexpected value EAGAIN. Possibly this is related to creating + * the C locale during + * + * #ifdef USE_XLOCALE + * return strtoll_l (nptr, endptr, base, get_C_locale ()); + * + * This wrapper tries to workaround that condition. + */ +gint64 +nm_g_ascii_strtoll (const char *nptr, + char **endptr, + guint base) +{ + int try_count = 2; + gint64 v; + const int errsv_orig = errno; + int errsv; + + nm_assert (nptr); + nm_assert (base == 0u || (base >= 2u && base <= 36u)); + +again: + errno = 0; + v = g_ascii_strtoll (nptr, endptr, base); + errsv = errno; + + if (errsv == 0) { + if (errsv_orig != 0) + errno = errsv_orig; + return v; + } + + if ( errsv == ERANGE + && NM_IN_SET (v, G_MININT64, G_MAXINT64)) + return v; + + if ( errsv == EINVAL + && v == 0 + && nptr + && nptr[0] == '\0') + return v; + + if (try_count-- > 0) + goto again; + +#if NM_MORE_ASSERTS + g_critical ("g_ascii_strtoll() for \"%s\" failed with errno=%d (%s) and v=%"G_GINT64_FORMAT, + nptr, + errsv, + nm_strerror_native (errsv), + v); +#endif + + return v; +} + /* _nm_utils_ascii_str_to_int64: * * A wrapper for g_ascii_strtoll, that checks whether the whole string diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index bba60954b..7ee024550 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -657,6 +657,10 @@ gboolean nm_utils_parse_inaddr_prefix (int addr_family, char **out_addr, int *out_prefix); +gint64 nm_g_ascii_strtoll (const char *nptr, + char **endptr, + guint base); + gint64 _nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 max, gint64 fallback); guint64 _nm_utils_ascii_str_to_uint64 (const char *str, guint base, guint64 min, guint64 max, guint64 fallback); From 35a9f632a81df40209e3d71f2328ccdfcf175aee Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:26:20 +0200 Subject: [PATCH 7/9] shared: add nm_g_ascii_strtod() to workaround bug --- shared/nm-glib-aux/nm-shared-utils.c | 37 ++++++++++++++++++++++++++++ shared/nm-glib-aux/nm-shared-utils.h | 3 +++ 2 files changed, 40 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 96c966f16..783887214 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -1021,6 +1021,43 @@ again: return v; } +/* see nm_g_ascii_strtoll(). */ +double +nm_g_ascii_strtod (const char *nptr, + char **endptr) +{ + int try_count = 2; + double v; + int errsv; + + nm_assert (nptr); + +again: + v = g_ascii_strtod (nptr, endptr); + errsv = errno; + + if (errsv == 0) + return v; + + if (errsv == ERANGE) + return v; + + if (try_count-- > 0) + goto again; + +#if NM_MORE_ASSERTS + g_critical ("g_ascii_strtod() for \"%s\" failed with errno=%d (%s) and v=%f", + nptr, + errsv, + nm_strerror_native (errsv), + v); +#endif + + /* Not really much else to do. Return the parsed value and leave errno set + * to the unexpected value. */ + return v; +} + /* _nm_utils_ascii_str_to_int64: * * A wrapper for g_ascii_strtoll, that checks whether the whole string diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index 7ee024550..79ea31771 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -661,6 +661,9 @@ gint64 nm_g_ascii_strtoll (const char *nptr, char **endptr, guint base); +double nm_g_ascii_strtod (const char *nptr, + char **endptr); + gint64 _nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 max, gint64 fallback); guint64 _nm_utils_ascii_str_to_uint64 (const char *str, guint base, guint64 min, guint64 max, guint64 fallback); From 3b58c5fef490cfdae632f5007c77267e47971734 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 13:50:16 +0200 Subject: [PATCH 8/9] shared: add nm_g_ascii_strtoull() to workaround bug --- shared/nm-glib-aux/nm-shared-utils.c | 49 ++++++++++++++++++++++++++++ shared/nm-glib-aux/nm-shared-utils.h | 4 +++ 2 files changed, 53 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 783887214..52f4bff2f 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -1021,6 +1021,55 @@ again: return v; } +/* See nm_g_ascii_strtoll() */ +guint64 +nm_g_ascii_strtoull (const char *nptr, + char **endptr, + guint base) +{ + int try_count = 2; + guint64 v; + const int errsv_orig = errno; + int errsv; + + nm_assert (nptr); + nm_assert (base == 0u || (base >= 2u && base <= 36u)); + +again: + errno = 0; + v = g_ascii_strtoull (nptr, endptr, base); + errsv = errno; + + if (errsv == 0) { + if (errsv_orig != 0) + errno = errsv_orig; + return v; + } + + if ( errsv == ERANGE + && NM_IN_SET (v, G_MAXUINT64)) + return v; + + if ( errsv == EINVAL + && v == 0 + && nptr + && nptr[0] == '\0') + return v; + + if (try_count-- > 0) + goto again; + +#if NM_MORE_ASSERTS + g_critical ("g_ascii_strtoull() for \"%s\" failed with errno=%d (%s) and v=%"G_GUINT64_FORMAT, + nptr, + errsv, + nm_strerror_native (errsv), + v); +#endif + + return v; +} + /* see nm_g_ascii_strtoll(). */ double nm_g_ascii_strtod (const char *nptr, diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index 79ea31771..b68e658c3 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -661,6 +661,10 @@ gint64 nm_g_ascii_strtoll (const char *nptr, char **endptr, guint base); +guint64 nm_g_ascii_strtoull (const char *nptr, + char **endptr, + guint base); + double nm_g_ascii_strtod (const char *nptr, char **endptr); From 7e49f4a199beb9b8012ec554c4a9ad1c851f7ff2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:30:20 +0200 Subject: [PATCH 9/9] all: use wrappers for g_ascii_strtoll(), g_ascii_strtoull(), g_ascii_strtod() Sometimes these function may set errno to unexpected values like EAGAIN. This causes confusion. Avoid that by using our own wrappers that retry in that case. For example, in rhbz#1797915 we have failures like: errno = 0; v = g_ascii_strtoll ("10", 0, &end); if (errno != 0) g_assert_not_reached (); as g_ascii_strtoll() would return 10, but also set errno to EAGAIN. Work around that by using wrapper functions that retry. This certainly should be fixed in glib (or glibc), but the issues are severe enough to warrant a workaround. Note that our workarounds are very defensive. We only retry 2 times, if we get an unexpected errno value. This is in the hope to recover from a spurious EAGAIN. It won't recover from other errors. https://bugzilla.redhat.com/show_bug.cgi?id=1797915 --- libnm-core/nm-utils.c | 4 +-- shared/nm-glib-aux/nm-shared-utils.c | 26 ++++--------------- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 2 +- 3 files changed, 8 insertions(+), 24 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 2e4b0ceb4..24d9dfcb4 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -2299,7 +2299,7 @@ _nm_utils_parse_tc_handle (const char *str, GError **error) nm_assert (str); - maj = g_ascii_strtoll (str, (char **) &sep, 0x10); + maj = nm_g_ascii_strtoll (str, (char **) &sep, 0x10); if (sep == str) goto fail; @@ -2308,7 +2308,7 @@ _nm_utils_parse_tc_handle (const char *str, GError **error) if (sep[0] == ':') { const char *str2 = &sep[1]; - min = g_ascii_strtoll (str2, (char **) &sep, 0x10); + min = nm_g_ascii_strtoll (str2, (char **) &sep, 0x10); sep = nm_str_skip_leading_spaces (sep); if (sep[0] != '\0') goto fail; diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 52f4bff2f..f1371801c 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -1134,26 +1134,10 @@ _nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 ma } errno = 0; - v = g_ascii_strtoll (str, (char **) &s, base); + v = nm_g_ascii_strtoll (str, (char **) &s, base); - if (errno != 0) { -#if NM_MORE_ASSERTS - int errsv = errno; - - /* the caller must not pass an invalid @base. Hence, we expect the only failure that - * can happen here is ERANGE, because invalid @str is not signaled via an errno according - * to documentation. */ - if ( errsv != ERANGE - || !NM_IN_SET (v, G_MININT64, G_MAXINT64)) { - g_error ("g_ascii_strtoll() for \"%s\" failed with errno=%d (%s) and v=%"G_GINT64_FORMAT, - str, - errsv, - nm_strerror_native (errsv), - v); - } -#endif + if (errno != 0) return fallback; - } if (s[0] != '\0') { s = nm_str_skip_leading_spaces (s); @@ -1186,7 +1170,7 @@ _nm_utils_ascii_str_to_uint64 (const char *str, guint base, guint64 min, guint64 } errno = 0; - v = g_ascii_strtoull (str, (char **) &s, base); + v = nm_g_ascii_strtoull (str, (char **) &s, base); if (errno != 0) return fallback; @@ -1205,8 +1189,8 @@ _nm_utils_ascii_str_to_uint64 (const char *str, guint base, guint64 min, guint64 if ( v != 0 && str[0] == '-') { - /* I don't know why, but g_ascii_strtoull() accepts minus signs ("-2" gives 18446744073709551614). - * For "-0" that is OK, but otherwise not. */ + /* As documented, g_ascii_strtoull() accepts negative values, and returns their + * absolute value. We don't. */ errno = ERANGE; return fallback; } diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index f337d0d4d..60bc50471 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -560,7 +560,7 @@ make_connection_setting (const char *file, char *endptr; double d; - d = g_ascii_strtod (v, &endptr); + d = nm_g_ascii_strtod (v, &endptr); endptr = nm_str_skip_leading_spaces (endptr); if ( errno == 0 && endptr[0] == '\0'