From 396a4dfc2b0d142d186f1a79249123a4e197179d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 13:02:03 +0200 Subject: [PATCH 1/8] 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. (cherry picked from commit 2e4771be5efb2b0e6820243929e55f4bf42c7cad) (cherry picked from commit 5a44792e4127d380a483809959e729d193322221) --- 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 68ebd78192..82124c1be8 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -543,16 +543,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 23bc8cadb62dc0cafce8d3855641f3a1e80de962 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 13:14:18 +0200 Subject: [PATCH 2/8] ifcfg-rh/tests: add unit test for reading DEVTIMEOUT (connection.wait-device-timeout) (cherry picked from commit 9cbf4c2825a7ce68c26080386fe72f5393706b87) (cherry picked from commit c81d12bc696f256e6e48ca018e8e54785bb3561c) --- .../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 dc47126ceb..e683db3c7b 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 45e90b919c..62c08a6c33 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -1732,6 +1732,7 @@ static void test_read_wired_autoip (void) { gs_unref_object NMConnection *connection = NULL; + NMSettingConnection *s_con; NMSettingIPConfig *s_ip4; char *unmanaged = NULL; @@ -1745,6 +1746,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 1a54909bb4161341272c761759ac8c35fa87a46c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:30:12 +0200 Subject: [PATCH 3/8] ifupdown: use _nm_utils_ascii_str_to_int64() for converting netmask to string (cherry picked from commit 3930ef194ec37d51eb74aed6e7e2ac403539bbfd) (cherry picked from commit 1a80179c60ea02af9da1ce454afbdc3cae023426) --- 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 41b20850ad..6f901fd85a 100644 --- a/src/settings/plugins/ifupdown/nms-ifupdown-parser.c +++ b/src/settings/plugins/ifupdown/nms-ifupdown-parser.c @@ -580,9 +580,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, @@ -590,12 +589,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; @@ -608,7 +607,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_valid (AF_INET6, gateway_v)) { @@ -629,7 +627,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 386ea3ff26fe68105c4f72d1706b06a3a2dc4341 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:47:55 +0200 Subject: [PATCH 4/8] 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. (cherry picked from commit 659ac9cc1299399b34b4677a492c9943458e1b52) (cherry picked from commit 62469c1401dcbd155da5e03bf8cfd37986d6541f) --- src/devices/bluetooth/nm-bluez-device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/devices/bluetooth/nm-bluez-device.c b/src/devices/bluetooth/nm-bluez-device.c index fb0a72c40f..b6619bfdd4 100644 --- a/src/devices/bluetooth/nm-bluez-device.c +++ b/src/devices/bluetooth/nm-bluez-device.c @@ -682,7 +682,7 @@ convert_uuids_to_capabilities (const char **strings) parts = g_strsplit (*iter, "-", -1); if (parts && parts[0]) { - switch (g_ascii_strtoull (parts[0], NULL, 16)) { + switch (_nm_utils_ascii_str_to_int64 (parts[0], 16, 0, G_MAXINT, -1)) { case 0x1103: capabilities |= NM_BT_CAPABILITY_DUN; break; From 49c523cf1e4da6a14fb09d0877db44646df1c688 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:26:20 +0200 Subject: [PATCH 5/8] shared: add nm_g_ascii_strtoll() to workaround bug (cherry picked from commit f4446e34c689d6bd81deb8bbab01387716db801f) (cherry picked from commit 6836679878889f882acd243b0c914865971edd4e) --- 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 9bfd5ae2b8..d35db13513 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -713,6 +713,72 @@ nm_utils_parse_inaddr_prefix (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 d9c430d495..5eb51d8f69 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -562,6 +562,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 0de1c3a53a6196694cbe4ecb2dade727a7469e65 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:26:20 +0200 Subject: [PATCH 6/8] shared: add nm_g_ascii_strtod() to workaround bug (cherry picked from commit 35a9f632a81df40209e3d71f2328ccdfcf175aee) (cherry picked from commit f8cae1ed18ad5d29c246e9d9036f670badd28126) --- 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 d35db13513..6e6a05c1c5 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -779,6 +779,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 5eb51d8f69..b362a1a168 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -566,6 +566,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 d629db4a0e80752e031ca0f5c410ad4b59e9090a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 13:50:16 +0200 Subject: [PATCH 7/8] shared: add nm_g_ascii_strtoull() to workaround bug (cherry picked from commit 3b58c5fef490cfdae632f5007c77267e47971734) (cherry picked from commit 95565bef77ddfb36dd61b764a3d8c0b6cec21333) --- 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 6e6a05c1c5..858f8959cb 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -779,6 +779,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 b362a1a168..e0b6adafdd 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -566,6 +566,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 500f0b96ae93cc0dbcb63124c46956cf532031c3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Apr 2020 12:30:20 +0200 Subject: [PATCH 8/8] 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 (cherry picked from commit 7e49f4a199beb9b8012ec554c4a9ad1c851f7ff2) (cherry picked from commit eec2740d718db4c73bf4c31b97852ee83bb3dfa7) --- libnm-core/nm-utils.c | 4 ++-- shared/nm-glib-aux/nm-shared-utils.c | 8 ++++---- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 8f2b40184a..2848e83c5c 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -2314,7 +2314,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; @@ -2323,7 +2323,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 858f8959cb..1c672d2c70 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -892,7 +892,7 @@ _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) return fallback; @@ -928,7 +928,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; @@ -947,8 +947,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 82124c1be8..7971fba56d 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -553,7 +553,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'