From a15756d99070abc3247cffa68a00578c40bb242a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Nov 2018 11:46:47 +0100 Subject: [PATCH 1/5] shared: add NM_MAKE_STRV() macro --- shared/nm-utils/nm-macros-internal.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/shared/nm-utils/nm-macros-internal.h b/shared/nm-utils/nm-macros-internal.h index 6a419f788f..40e2dd0bc9 100644 --- a/shared/nm-utils/nm-macros-internal.h +++ b/shared/nm-utils/nm-macros-internal.h @@ -639,6 +639,9 @@ NM_G_ERROR_MSG (GError *error) #define NM_PROPAGATE_CONST(test_expr, ptr) (ptr) #endif +#define NM_MAKE_STRV(...) \ + ((const char *const[]) { __VA_ARGS__, NULL }) + /*****************************************************************************/ #define _NM_IN_SET_EVAL_1( op, _x, y) (_x == (y)) From 1c338861c486569113a6c2b2ca301db1f6808d3c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Nov 2018 11:01:30 +0100 Subject: [PATCH 2/5] dns: make strv arguments of create_resolv_conf() const --- src/dns/nm-dns-manager.c | 59 ++++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 18 deletions(-) diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c index 5ef9a6b83f..5d07f08954 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -569,9 +569,9 @@ again: } static char * -create_resolv_conf (char **searches, - char **nameservers, - char **options) +create_resolv_conf (const char *const*searches, + const char *const*nameservers, + const char *const*options) { GString *str; gsize i; @@ -636,9 +636,9 @@ write_resolv_conf_contents (FILE *f, static gboolean write_resolv_conf (FILE *f, - char **searches, - char **nameservers, - char **options, + const char *const*searches, + const char *const*nameservers, + const char *const*options, GError **error) { gs_free char *content = NULL; @@ -700,7 +700,11 @@ dispatch_resolvconf (NMDnsManager *self, return SR_ERROR; } - success = write_resolv_conf (f, searches, nameservers, options, error); + success = write_resolv_conf (f, + NM_CAST_STRV_CC (searches), + NM_CAST_STRV_CC (nameservers), + NM_CAST_STRV_CC (options), + error); err = pclose (f); if (err < 0) { errnosv = errno; @@ -741,9 +745,9 @@ _read_link_cached (const char *path, gboolean *is_cached, char **cached) static void update_resolv_conf_no_stub (NMDnsManager *self, - char **searches, - char **nameservers, - char **options) + const char *const*searches, + const char *const*nameservers, + const char *const*options) { gs_free char *content = NULL; GError *local = NULL; @@ -766,9 +770,9 @@ update_resolv_conf_no_stub (NMDnsManager *self, static SpawnResult update_resolv_conf (NMDnsManager *self, - char **searches, - char **nameservers, - char **options, + const char *const*searches, + const char *const*nameservers, + const char *const*options, GError **error, NMDnsManagerResolvConfManager rc_manager) { @@ -1431,7 +1435,10 @@ update_dns (NMDnsManager *self, * guarantee they stay alive. */ clear_domain_lists (self); - update_resolv_conf_no_stub (self, searches, nameservers, options); + update_resolv_conf_no_stub (self, + NM_CAST_STRV_CC (searches), + NM_CAST_STRV_CC (nameservers), + NM_CAST_STRV_CC (options)); /* If caching was successful, we only send 127.0.0.1 to /etc/resolv.conf * to ensure that the glibc resolver doesn't try to round-robin nameservers, @@ -1454,7 +1461,12 @@ update_dns (NMDnsManager *self, switch (priv->rc_manager) { case NM_DNS_MANAGER_RESOLV_CONF_MAN_SYMLINK: case NM_DNS_MANAGER_RESOLV_CONF_MAN_FILE: - result = update_resolv_conf (self, searches, nameservers, options, error, priv->rc_manager); + result = update_resolv_conf (self, + NM_CAST_STRV_CC (searches), + NM_CAST_STRV_CC (nameservers), + NM_CAST_STRV_CC (options), + error, + priv->rc_manager); resolv_conf_updated = TRUE; /* If we have ended with no nameservers avoid updating again resolv.conf * on stop, as some external changes may be applied to it in the meanwhile */ @@ -1479,15 +1491,26 @@ update_dns (NMDnsManager *self, if (result == SR_NOTFOUND) { _LOGD ("update-dns: program not available, writing to resolv.conf"); g_clear_error (error); - result = update_resolv_conf (self, searches, nameservers, options, error, NM_DNS_MANAGER_RESOLV_CONF_MAN_SYMLINK); + result = update_resolv_conf (self, + NM_CAST_STRV_CC (searches), + NM_CAST_STRV_CC (nameservers), + NM_CAST_STRV_CC (options), + error, + NM_DNS_MANAGER_RESOLV_CONF_MAN_SYMLINK); resolv_conf_updated = TRUE; } } /* Unless we've already done it, update private resolv.conf in NMRUNDIR ignoring any errors */ - if (!resolv_conf_updated) - update_resolv_conf (self, searches, nameservers, options, NULL, NM_DNS_MANAGER_RESOLV_CONF_MAN_UNMANAGED); + if (!resolv_conf_updated) { + update_resolv_conf (self, + NM_CAST_STRV_CC (searches), + NM_CAST_STRV_CC (nameservers), + NM_CAST_STRV_CC (options), + NULL, + NM_DNS_MANAGER_RESOLV_CONF_MAN_UNMANAGED); + } /* signal that resolv.conf was changed */ if (update && result == SR_SUCCESS) From 60cd93612f15d1e5626121e73b08b2e434cac225 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Nov 2018 10:44:19 +0100 Subject: [PATCH 3/5] dns/tests: add test for writing resolv.conf --- src/dns/nm-dns-manager.c | 8 +++++++ src/dns/nm-dns-manager.h | 6 +++++ src/tests/test-general.c | 48 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+) diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c index 5d07f08954..e1c295666b 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -613,6 +613,14 @@ create_resolv_conf (const char *const*searches, return g_string_free (str, FALSE); } +char * +nmtst_dns_create_resolv_conf (const char *const*searches, + const char *const*nameservers, + const char *const*options) +{ + return create_resolv_conf (searches, nameservers, options); +} + static gboolean write_resolv_conf_contents (FILE *f, const char *content, diff --git a/src/dns/nm-dns-manager.h b/src/dns/nm-dns-manager.h index ed1974a566..a3e9472e5f 100644 --- a/src/dns/nm-dns-manager.h +++ b/src/dns/nm-dns-manager.h @@ -129,4 +129,10 @@ typedef enum { void nm_dns_manager_stop (NMDnsManager *self); +/*****************************************************************************/ + +char *nmtst_dns_create_resolv_conf (const char *const*searches, + const char *const*nameservers, + const char *const*options); + #endif /* __NETWORKMANAGER_DNS_MANAGER_H__ */ diff --git a/src/tests/test-general.c b/src/tests/test-general.c index 51f9fedbd3..87ff7f8480 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -29,6 +29,8 @@ #include "NetworkManagerUtils.h" #include "nm-core-internal.h" +#include "dns/nm-dns-manager.h" + #include "nm-test-utils-core.h" /* Reference implementation for nm_utils_ip6_address_clear_host_address. @@ -1864,6 +1866,50 @@ test_utils_file_is_in_path (void) /*****************************************************************************/ +#define _TEST_RC(searches, nameservers, options, expected) \ + G_STMT_START { \ + const char *const*const _searches = (searches); \ + const char *const*const _nameservers = (nameservers); \ + const char *const*const _options = (options); \ + gs_free char *_content = NULL; \ + \ + _content = nmtst_dns_create_resolv_conf (_searches, _nameservers, _options); \ + g_assert_cmpstr (_content, ==, expected); \ + } G_STMT_END + +static void +test_dns_create_resolv_conf (void) +{ + _TEST_RC (NM_MAKE_STRV ("a"), + NULL, + NULL, + "# Generated by NetworkManager\n" + "search a\n" + ""); + + _TEST_RC (NM_MAKE_STRV ("a", "b.com"), + NM_MAKE_STRV ("192.168.55.1", "192.168.56.1"), + NM_MAKE_STRV ("opt1", "opt2"), + "# Generated by NetworkManager\n" + "search a b.com\n" + "nameserver 192.168.55.1\n" + "nameserver 192.168.56.1\n" + "options opt1 opt2\n" + ""); + + _TEST_RC (NM_MAKE_STRV ("a2x456789.b2x456789.c2x456789.d2x456789.e2x456789.f2x456789.g2x456789.h2x456789.i2x456789.j2x4567890", + "a2y456789.b2y456789.c2y456789.d2y456789.e2y456789.f2y456789.g2y456789.h2y456789.i2y456789.j2y4567890", + "a2z456789.b2z456789.c2z456789.d2z456789.e2z456789.f2z456789.g2z456789.h2z456789.i2z456789.j2z4567890"), + NULL, + NULL, + "# Generated by NetworkManager\n" + "search a2x456789.b2x456789.c2x456789.d2x456789.e2x456789.f2x456789.g2x456789.h2x456789.i2x456789.j2x4567890 a2y456789.b2y456789.c2y456789.d2y456789.e2y456789.f2y456789.g2y456789.h2y456789.i2y456789.j2y4567890 a2z456789.b2z456789.c2z456789.d2z456789.e2z456789.f2z456789.g2z456789.h2z456789.i2z456789.j2z4567890\n" + ""); + +} + +/*****************************************************************************/ + NMTST_DEFINE (); int @@ -1912,6 +1958,8 @@ main (int argc, char **argv) g_test_add_func ("/general/test_utils_file_is_in_path", test_utils_file_is_in_path); + g_test_add_func ("/general/test_dns_create_resolv_conf", test_dns_create_resolv_conf); + return g_test_run (); } From 49c11a44e4e901752a81e3942efca64448fa7c53 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Nov 2018 10:34:54 +0100 Subject: [PATCH 4/5] dns: avoid truncation of searches list due to 256 char limit in glibc Before glibc 2.26, glibc's resolver would only honor 6 search entries and a character limit of 256. This was lifted recently ([1], [2], [3]). We also lift this limitation in NetworkManager ([4], [5]). However, older glibc versions would just truncate the string at 255 characters. In particular, it would not only tuncate the list to 6 entries, but the entry which crosses the 256th character boundary would be mangled. Avoid that, by adding spaces. [1] https://sourceware.org/ml/libc-alpha/2017-08/msg00010.html [2] https://sourceware.org/bugzilla/show_bug.cgi?id=19569 [3] https://sourceware.org/bugzilla/show_bug.cgi?id=21475 [4] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/47 [5] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/80 --- src/dns/nm-dns-manager.c | 31 ++++++++++++++++++++++++++++++- src/tests/test-general.c | 2 +- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c index e1c295666b..7542228f54 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -581,10 +581,39 @@ create_resolv_conf (const char *const*searches, g_string_append (str, "# Generated by NetworkManager\n"); if (searches && searches[0]) { + gsize search_base_idx; + g_string_append (str, "search"); + search_base_idx = str->len; + for (i = 0; searches[i]; i++) { + const char *s = searches[i]; + gsize l = strlen (s); + + if ( l == 0 + || NM_STRCHAR_ANY (s, ch, NM_IN_SET (ch, ' ', '\t', '\n'))) { + /* there should be no such characters in the search entry. Also, + * because glibc parser would treat them as line/word separator. + * + * Skip the value silently. */ + continue; + } + + if (search_base_idx > 0) { + if (str->len - search_base_idx + 1 + l > 254) { + /* this entry crosses the 256 character boundery. Older glibc versions + * would truncate the entry at this point. + * + * Fill the line with spaces to cross the 256 char boundary and continue + * afterwards. This way, the truncation happens between two search entries. */ + while (str->len - search_base_idx < 257) + g_string_append_c (str, ' '); + search_base_idx = 0; + } + } + g_string_append_c (str, ' '); - g_string_append (str, searches[i]); + g_string_append_len (str, s, l); } g_string_append_c (str, '\n'); } diff --git a/src/tests/test-general.c b/src/tests/test-general.c index 87ff7f8480..2ab5f8f735 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -1903,7 +1903,7 @@ test_dns_create_resolv_conf (void) NULL, NULL, "# Generated by NetworkManager\n" - "search a2x456789.b2x456789.c2x456789.d2x456789.e2x456789.f2x456789.g2x456789.h2x456789.i2x456789.j2x4567890 a2y456789.b2y456789.c2y456789.d2y456789.e2y456789.f2y456789.g2y456789.h2y456789.i2y456789.j2y4567890 a2z456789.b2z456789.c2z456789.d2z456789.e2z456789.f2z456789.g2z456789.h2z456789.i2z456789.j2z4567890\n" + "search a2x456789.b2x456789.c2x456789.d2x456789.e2x456789.f2x456789.g2x456789.h2x456789.i2x456789.j2x4567890 a2y456789.b2y456789.c2y456789.d2y456789.e2y456789.f2y456789.g2y456789.h2y456789.i2y456789.j2y4567890 a2z456789.b2z456789.c2z456789.d2z456789.e2z456789.f2z456789.g2z456789.h2z456789.i2z456789.j2z4567890\n" ""); } From 3f2cc579e7627f45a77f835ba2f402e1b5c600b5 Mon Sep 17 00:00:00 2001 From: Kyle Walker Date: Thu, 8 Nov 2018 13:11:18 -0500 Subject: [PATCH 5/5] dns: remove the resolv.conf 6 entry limit The resolv.conf used to have a limit of 6 entries for the search option. With later versions of glibc, this limit has been removed. As a result, remove the limit here so that all search entries set will be applied to the resolv.conf. If there is a limit imposed by older versions of glibc, it should be imposed there as opposed to within NetworkManager. https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/80 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/47 --- src/dns/nm-dns-manager.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c index 7542228f54..8575237742 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -1100,7 +1100,6 @@ _collect_resolv_conf_data (NMDnsManager *self, const char **out_nis_domain) { NMDnsManagerPrivate *priv; - guint i, num, len; NMResolvConfData rc = { .nameservers = g_ptr_array_new (), .searches = g_ptr_array_new (), @@ -1170,17 +1169,6 @@ _collect_resolv_conf_data (NMDnsManager *self, } } - /* Per 'man resolv.conf', the search list is limited to 6 domains - * totalling 256 characters. - */ - num = MIN (rc.searches->len, 6u); - for (i = 0, len = 0; i < num; i++) { - len += strlen (rc.searches->pdata[i]) + 1; /* +1 for spaces */ - if (len > 256) - break; - } - g_ptr_array_set_size (rc.searches, i); - *out_searches = _ptrarray_to_strv (rc.searches); *out_options = _ptrarray_to_strv (rc.options); *out_nameservers = _ptrarray_to_strv (rc.nameservers);