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
Attachment:
signature.asc
Description: This is a digitally signed message part