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().
Patch cherry-picked from upstream Git.

Closes: #813803

3 files changed, 297 insertions, 0 deletions

diff --git a/debian/changelog b/debian/changelog
index dee226cb..135641f1 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
+network-manager (1.1.90-5) UNRELEASED; urgency=medium
+
+  * 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().
+    Patch cherry-picked from upstream Git. (Closes: #813803)
+
+ -- Michael Biebl <biebl@debian.org>  Fri, 05 Feb 2016 20:09:10 +0100
+
 network-manager (1.1.90-4) unstable; urgency=medium
 
   * Upload to unstable.
diff --git a/debian/patches/dns-clean-up-error-paths-in-dns-manager.patch b/debian/patches/dns-clean-up-error-paths-in-dns-manager.patch
new file mode 100644
index 00000000..389ae0ed
--- /dev/null
+++ b/debian/patches/dns-clean-up-error-paths-in-dns-manager.patch
@@ -0,0 +1,286 @@
+From: Dan Williams <dcbw@redhat.com>
+Date: Wed, 20 Jan 2016 13:52:59 -0600
+Subject: 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().
+
+(cherry-picked from commit af189b3295847cf48bc7616ec3bfc4192458608c)
+---
+ 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 01e8bf1..33bef65 100644
+--- a/src/dns-manager/nm-dns-manager.c
++++ b/src/dns-manager/nm-dns-manager.c
+@@ -357,7 +357,6 @@ dispatch_netconfig (NMDnsManager *self,
+ 
+ 	if (searches) {
+ 		str = g_strjoinv (" ", searches);
+-
+ 		write_to_netconfig (self, fd, "DNSSEARCH", str);
+ 		g_free (str);
+ 	}
+@@ -405,10 +404,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;
+@@ -425,11 +423,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, "# ");
+@@ -443,28 +440,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
+@@ -474,9 +465,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)) {
+@@ -487,39 +478,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;
+ 	}
+ 
+-	g_free (cmd);
++	_LOGI ("Writing DNS information to %s", RESOLVCONF_PATH);
+ 
+-	return retval ? SR_SUCCESS : SR_ERROR;
++	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;
++	}
++
++	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 success ? SR_SUCCESS : SR_ERROR;
+ }
+ 
+ #define MY_RESOLV_CONF NMRUNDIR "/resolv.conf"
+@@ -536,7 +534,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
+@@ -544,15 +542,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);
+-
+-		g_free (path);
++		gs_free char *path = g_file_read_link (_PATH_RESCONF, NULL);
+ 
+-		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;
+ 		}
+ 	}
+ 
+@@ -566,10 +561,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.
+ 			 */
+@@ -580,9 +575,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) {
+@@ -598,30 +592,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,
+@@ -631,6 +627,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,
diff --git a/debian/patches/series b/debian/patches/series
index 50f4487c..f6d70ca7 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -11,3 +11,4 @@ libnm-vpn-restore-export-of-deprecated-NMVpnPluginOld-sym.patch
 tests-use-dbus-run-session-instead-of-dbus-launch.patch
 systemd-add-chroot-capability.patch
 Use-bash-explicitly-when-running-the-tests.patch
+dns-clean-up-error-paths-in-dns-manager.patch