[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().
---
 src/dns-manager/nm-dns-manager.c | 126 ++++++++++++++++++---------------------
 1 file changed, 58 insertions(+), 68 deletions(-)

diff --git a/src/dns-manager/nm-dns-manager.c b/src/dns-manager/nm-dns-manager.c
index 01e8bf1..48f44ea 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,12 +423,9 @@ write_resolv_conf (FILE *f,
                g_free (tmp_str);
        }
 
-       str = g_string_new ("");
-
        if (nameservers) {
-               int num = g_strv_length (nameservers);
-
-               for (i = 0; i < num; i++) {
+               str = g_string_new ("");
+               for (i = 0; i < g_strv_length (nameservers); i++) {
                        if (i == 3) {
                                g_string_append (str, "# ");
                                g_string_append (str, _("NOTE: the libc resolver may not support more than 3 
nameservers."));
@@ -443,28 +438,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,10 +463,11 @@ 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;
+       GError *local = NULL;
 
        if (!g_file_test (RESOLVCONF_PATH, G_FILE_TEST_IS_EXECUTABLE)) {
                g_set_error_literal (error,
@@ -487,39 +477,45 @@ 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, &local);
+       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);
+               return SR_ERROR;
+       } else if (err > 0) {
+               _LOGW ("resolvconf failed with status %d", err);
+               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;
+       g_propagate_error (error, local);
+       return success ? SR_SUCCESS : SR_ERROR;
 }
 
 #define MY_RESOLV_CONF NMRUNDIR "/resolv.conf"
@@ -536,7 +532,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 +540,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;
                }
        }
 
@@ -566,10 +559,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 +573,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) {
@@ -603,11 +595,9 @@ update_resolv_conf (NMDnsManager *self,
                /* 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)
                                        return SR_SUCCESS;
                        } else {
                                if (errno != ENOENT)
-- 
2.4.3


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]