From 23d9a5218ab0da09fce5993e3299ad9043bab14e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 12 Sep 2018 16:04:51 +0200 Subject: [PATCH 1/7] dns: refactor create_resolv_conf() to use GString for constructing content (cherry picked from commit 95b006c244978fecec9463690477e8b64f743202) --- src/dns/nm-dns-manager.c | 55 ++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c index 6a59b41c7b..33a414831f 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -586,49 +586,42 @@ create_resolv_conf (char **searches, char **nameservers, char **options) { - gs_free char *searches_str = NULL; - gs_free char *nameservers_str = NULL; - gs_free char *options_str = NULL; - char *tmp_str; GString *str; - int i; + gsize i; - if (searches) { - tmp_str = g_strjoinv (" ", searches); - searches_str = g_strconcat ("search ", tmp_str, "\n", NULL); - g_free (tmp_str); + str = g_string_new_len ("# Generated by NetworkManager\n", 245); + + if (searches && searches[0]) { + g_string_append (str, "search"); + for (i = 0; searches[i]; i++) { + g_string_append_c (str, ' '); + g_string_append (str, searches[i]); + } + g_string_append_c (str, '\n'); } - if (options) { - tmp_str = g_strjoinv (" ", options); - options_str = g_strconcat ("options ", tmp_str, "\n", NULL); - g_free (tmp_str); - } - - if (nameservers) { - int num = g_strv_length (nameservers); - - str = g_string_new (""); - for (i = 0; i < num; i++) { + if (nameservers && nameservers[0]) { + for (i = 0; nameservers[i]; i++) { if (i == 3) { - g_string_append (str, "# "); - g_string_append (str, "NOTE: the libc resolver may not support more than 3 nameservers."); - g_string_append (str, "\n# "); - g_string_append (str, "The nameservers listed below may not be recognized."); - g_string_append_c (str, '\n'); + g_string_append (str, "# NOTE: the libc resolver may not support more than 3 nameservers.\n"); + g_string_append (str, "# The nameservers listed below may not be recognized.\n"); } - g_string_append (str, "nameserver "); g_string_append (str, nameservers[i]); g_string_append_c (str, '\n'); } - nameservers_str = g_string_free (str, FALSE); } - return g_strdup_printf ("# Generated by NetworkManager\n%s%s%s", - searches_str ?: "", - nameservers_str ?: "", - options_str ?: ""); + if (options && options[0]) { + g_string_append (str, "options"); + for (i = 0; options[i]; i++) { + g_string_append_c (str, ' '); + g_string_append (str, options[i]); + } + g_string_append_c (str, '\n'); + } + + return g_string_free (str, FALSE); } static gboolean From ce88f7c404bace6a9d9d01c2eb57c0f7ea178a23 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Sep 2018 13:09:02 +0200 Subject: [PATCH 2/7] dns: fix creating resolv.conf content g_string_new_len() allocates the buffer with length bytes. Maybe it should be obvious (wasn't to me), but if a init argument is given, that is taken as containing length bytes. So, str = g_string_new_len (init, len); is more like str = g_string_new_len (NULL, len); g_string_append_len (str, init, len); and not (how I wrongly thought) str = g_string_new_len (NULL, len); g_string_append (str, init); Fixes: 95b006c244978fecec9463690477e8b64f743202 (cherry picked from commit 511709c54df5ecdc96453cb8af9796a54d901aa8) --- src/dns/nm-dns-manager.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c index 33a414831f..45b5556709 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -589,7 +589,9 @@ create_resolv_conf (char **searches, GString *str; gsize i; - str = g_string_new_len ("# Generated by NetworkManager\n", 245); + str = g_string_new_len (NULL, 245); + + g_string_append (str, "# Generated by NetworkManager\n"); if (searches && searches[0]) { g_string_append (str, "search"); From 5fe1728d1fa1e4a8515ee57de8e3a3709aaf40a4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Nov 2018 11:46:47 +0100 Subject: [PATCH 3/7] shared: add NM_MAKE_STRV() macro (cherry picked from commit a15756d99070abc3247cffa68a00578c40bb242a) --- 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 084219b4bb..764144e9f5 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 4515d36fa17f9996e43056e83688f8d7c6edf7e5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Nov 2018 11:01:30 +0100 Subject: [PATCH 4/7] dns: make strv arguments of create_resolv_conf() const (cherry picked from commit 1c338861c486569113a6c2b2ca301db1f6808d3c) --- src/dns/nm-dns-manager.c | 48 ++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c index 45b5556709..945a4dd4d7 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -582,9 +582,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; @@ -649,9 +649,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; @@ -713,7 +713,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; @@ -752,9 +756,9 @@ _read_link_cached (const char *path, gboolean *is_cached, char **cached) 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) { @@ -1444,7 +1448,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 */ @@ -1469,15 +1478,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 b78a0ebcb18057fb8f0a43d291ebc7cfab7326b0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Nov 2018 10:44:19 +0100 Subject: [PATCH 5/7] dns/tests: add test for writing resolv.conf (cherry picked from commit 60cd93612f15d1e5626121e73b08b2e434cac225) --- 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 945a4dd4d7..c4402d5dc5 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -626,6 +626,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 1e7329d13b..34b183a65d 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. @@ -1845,6 +1847,50 @@ test_nm_utils_exp10 (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 @@ -1891,6 +1937,8 @@ main (int argc, char **argv) g_test_add_func ("/general/stable-id/parse", test_stable_id_parse); g_test_add_func ("/general/stable-id/generated-complete", test_stable_id_generated_complete); + g_test_add_func ("/general/test_dns_create_resolv_conf", test_dns_create_resolv_conf); + return g_test_run (); } From dfce87b2b775772a60922d2bdd064f3fa7a22fad Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Nov 2018 10:34:54 +0100 Subject: [PATCH 6/7] 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 (cherry picked from commit 49c11a44e4e901752a81e3942efca64448fa7c53) --- 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 c4402d5dc5..0266c0708f 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -594,10 +594,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 34b183a65d..01d964322c 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -1884,7 +1884,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 3ce19034faf5340a5ef5486c276516ebcca4b147 Mon Sep 17 00:00:00 2001 From: Kyle Walker Date: Thu, 8 Nov 2018 13:11:18 -0500 Subject: [PATCH 7/7] 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 (cherry picked from commit 3f2cc579e7627f45a77f835ba2f402e1b5c600b5) --- 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 0266c0708f..5bffddb43d 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -1102,7 +1102,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 (), @@ -1172,17 +1171,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);