On Wed, 2016-01-20 at 13:52 -0600, Dan Williams wrote: > 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++) {
for (i = 0; nameservers[i]; i++) otherwise, the length has to be calculated needlessly and for each iteration. > 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; leaks @local. Possibly declare @local as gs_free_error, and clear pointer after g_propagate_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) While at it, there are several "\n" at GError messages: »···»···g_set_error (error, »···»··· NM_MANAGER_ERROR, »···»··· NM_MANAGER_ERROR_FAILED, »···»··· "Could not rename %s to %s: %s\n", ^^^ Let's add a code comment below (with better wording): »···if (!install_etc) »···»···return SR_SUCCESS; /* we always rewrite the symlink pointing to our own resolf.conf. * this is to accomodate applications that watch the file via inotify. */ TODO: rework nm_spawn_process() to accept a separate argv list instead of one command-string. Thomas
signature.asc
Description: This is a digitally signed message part
_______________________________________________ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list