[PATCH v2] dns: clean up error paths in dns-manager
- From: Dan Williams <dcbw redhat com>
- To: Thomas Haller <thaller redhat com>, "networkmanager-list gnome org" <networkmanager-list gnome org>
- Subject: [PATCH v2] dns: clean up error paths in dns-manager
- Date: Mon, 25 Jan 2016 12:59:22 -0600
From 2370f508df8400b424fec032f8314cea97fdbaef Mon Sep 17 00:00:00 2001
From: Dan Williams <dcbw 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
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
