>From 2370f508df8400b424fec032f8314cea97fdbaef Mon Sep 17 00:00:00 2001 From: Dan Williams <d...@redhat.com> Date: Wed, 20 Jan 2016 13:52:59 -0600 Subject: [PATCH] dns: clean up error paths in dns-manager
Specifically for resolvconf, if the write succeeded, but the pclose() failed error would be left NULL and SR_ERROR would be returned, which caused a crash in nm_dns_manager_end_updates(). --- v2: fixed thaller's comments except for the '\n' which I'll do in another patch src/dns-manager/nm-dns-manager.c | 152 +++++++++++++++++++-------------------- 1 file changed, 76 insertions(+), 76 deletions(-) diff --git a/src/dns-manager/nm-dns-manager.c b/src/dns-manager/nm-dns-manager.c index 20af6d2..66cd500 100644 --- a/src/dns-manager/nm-dns-manager.c +++ b/src/dns-manager/nm-dns-manager.c @@ -367,7 +367,6 @@ dispatch_netconfig (NMDnsManager *self, if (searches) { str = g_strjoinv (" ", searches); - write_to_netconfig (self, fd, "DNSSEARCH", str); g_free (str); } @@ -415,10 +414,9 @@ write_resolv_conf (FILE *f, char **options, GError **error) { - char *searches_str = NULL; - char *nameservers_str = NULL; - char *options_str = NULL; - gboolean retval = FALSE; + 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; @@ -435,11 +433,10 @@ write_resolv_conf (FILE *f, g_free (tmp_str); } - str = g_string_new (""); - if (nameservers) { int num = g_strv_length (nameservers); + str = g_string_new (""); for (i = 0; i < num; i++) { if (i == 3) { g_string_append (str, "# "); @@ -453,28 +450,22 @@ write_resolv_conf (FILE *f, g_string_append (str, nameservers[i]); g_string_append_c (str, '\n'); } + nameservers_str = g_string_free (str, FALSE); } - nameservers_str = g_string_free (str, FALSE); - if (fprintf (f, "# Generated by NetworkManager\n%s%s%s", searches_str ? searches_str : "", - nameservers_str, - options_str ? options_str : "") > 0) - retval = TRUE; - else { + nameservers_str ? nameservers_str : "", + options_str ? options_str : "") < 0) { g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_FAILED, "Could not write " _PATH_RESCONF ": %s\n", g_strerror (errno)); + return FALSE; } - g_free (searches_str); - g_free (nameservers_str); - g_free (options_str); - - return retval; + return TRUE; } static SpawnResult @@ -484,9 +475,9 @@ dispatch_resolvconf (NMDnsManager *self, char **options, GError **error) { - char *cmd; + gs_free char *cmd = NULL; FILE *f; - gboolean retval = FALSE; + gboolean success = FALSE; int errnosv, err; if (!g_file_test (RESOLVCONF_PATH, G_FILE_TEST_IS_EXECUTABLE)) { @@ -497,39 +488,46 @@ dispatch_resolvconf (NMDnsManager *self, return SR_NOTFOUND; } - if (searches || nameservers) { - cmd = g_strconcat (RESOLVCONF_PATH, " -a ", "NetworkManager", NULL); - _LOGI ("Writing DNS information to %s", RESOLVCONF_PATH); - if ((f = popen (cmd, "w")) == NULL) - g_set_error (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_FAILED, - "Could not write to %s: %s\n", - RESOLVCONF_PATH, - g_strerror (errno)); - else { - retval = write_resolv_conf (f, searches, nameservers, options, error); - err = pclose (f); - if (err < 0) { - errnosv = errno; - g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errnosv), - "Failed to close pipe to resolvconf: %d", errnosv); - retval = FALSE; - } else if (err > 0) { - _LOGW ("resolvconf failed with status %d", err); - retval = FALSE; - } - } - } else { - cmd = g_strconcat (RESOLVCONF_PATH, " -d ", "NetworkManager", NULL); + if (!searches && !nameservers) { _LOGI ("Removing DNS information from %s", RESOLVCONF_PATH); - if (nm_spawn_process (cmd, error) == 0) - retval = TRUE; + + cmd = g_strconcat (RESOLVCONF_PATH, " -d ", "NetworkManager", NULL); + if (nm_spawn_process (cmd, error) != 0) + return SR_ERROR; + + return SR_SUCCESS; + } + + _LOGI ("Writing DNS information to %s", RESOLVCONF_PATH); + + cmd = g_strconcat (RESOLVCONF_PATH, " -a ", "NetworkManager", NULL); + if ((f = popen (cmd, "w")) == NULL) { + g_set_error (error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_FAILED, + "Could not write to %s: %s\n", + RESOLVCONF_PATH, + g_strerror (errno)); + return SR_ERROR; } - g_free (cmd); + success = write_resolv_conf (f, searches, nameservers, options, error); + err = pclose (f); + if (err < 0) { + errnosv = errno; + g_clear_error (error); + g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errnosv), + "Failed to close pipe to resolvconf: %d", errnosv); + return SR_ERROR; + } else if (err > 0) { + _LOGW ("resolvconf failed with status %d", err); + g_clear_error (error); + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "resolvconf failed with status %d", err); + return SR_ERROR; + } - return retval ? SR_SUCCESS : SR_ERROR; + return success ? SR_SUCCESS : SR_ERROR; } #define MY_RESOLV_CONF NMRUNDIR "/resolv.conf" @@ -546,7 +544,7 @@ update_resolv_conf (NMDnsManager *self, { FILE *f; struct stat st; - gboolean ret; + gboolean success; /* If we are not managing /etc/resolv.conf and it points to * MY_RESOLV_CONF, don't write the private DNS configuration to @@ -554,15 +552,12 @@ update_resolv_conf (NMDnsManager *self, * some external application. */ if (!install_etc) { - char *path = g_file_read_link (_PATH_RESCONF, NULL); - gboolean ours = !g_strcmp0 (path, MY_RESOLV_CONF); + gs_free char *path = g_file_read_link (_PATH_RESCONF, NULL); - g_free (path); - - if (ours) { + if (g_strcmp0 (path, MY_RESOLV_CONF) == 0) { _LOGD ("not updating " MY_RESOLV_CONF " since it points to " _PATH_RESCONF); - return SR_ERROR; + return SR_SUCCESS; } } @@ -576,10 +571,10 @@ update_resolv_conf (NMDnsManager *self, return SR_ERROR; } - ret = write_resolv_conf (f, searches, nameservers, options, error); + success = write_resolv_conf (f, searches, nameservers, options, error); if (fclose (f) < 0) { - if (ret) { + if (success) { /* only set an error here if write_resolv_conf() was successful, * since its error is more important. */ @@ -590,9 +585,8 @@ update_resolv_conf (NMDnsManager *self, MY_RESOLV_CONF_TMP, g_strerror (errno)); } - } - - if (!ret) + return SR_ERROR; + } else if (!success) return SR_ERROR; if (rename (MY_RESOLV_CONF_TMP, MY_RESOLV_CONF) < 0) { @@ -608,30 +602,32 @@ update_resolv_conf (NMDnsManager *self, if (!install_etc) return SR_SUCCESS; - /* Don't overwrite a symbolic link unless it points to MY_RESOLV_CONF. */ + /* A symlink pointing to NM's own resolv.conf (MY_RESOLV_CONF) is always + * overwritten to ensure that changes are indicated with inotify. Symlinks + * pointing to any other file are never overwritten. + */ if (lstat (_PATH_RESCONF, &st) != -1) { - /* Don't overwrite a symbolic link. */ if (S_ISLNK (st.st_mode)) { if (stat (_PATH_RESCONF, &st) != -1) { - char *path = g_file_read_link (_PATH_RESCONF, NULL); - gboolean not_ours = g_strcmp0 (path, MY_RESOLV_CONF) != 0; + gs_free char *path = g_file_read_link (_PATH_RESCONF, NULL); - g_free (path); - if (not_ours) + if (g_strcmp0 (path, MY_RESOLV_CONF) != 0) { + /* It's not NM's symlink; do nothing */ return SR_SUCCESS; + } + + /* resolv.conf is a symlink owned by NM and the target is accessible + */ } else { - if (errno != ENOENT) - return SR_SUCCESS; - g_set_error (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_FAILED, - "Could not stat %s: %s\n", - _PATH_RESCONF, - g_strerror (errno)); - return SR_ERROR; + /* resolv.conf is a symlink but the target is not accessible; + * some other program is probably managing resolv.conf and + * NM should not touch it. + */ + return SR_SUCCESS; } } } else if (errno != ENOENT) { + /* NM cannot read /etc/resolv.conf */ g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_FAILED, @@ -641,6 +637,10 @@ update_resolv_conf (NMDnsManager *self, return SR_ERROR; } + /* By this point, either /etc/resolv.conf does not exist, is a regular + * file, or is a symlink already owned by NM. In all cases /etc/resolv.conf + * is replaced with a symlink pointing to NM's resolv.conf in /var/run/. + */ if (unlink (RESOLV_CONF_TMP) == -1 && errno != ENOENT) { g_set_error (error, NM_MANAGER_ERROR, -- 2.4.3 _______________________________________________ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list