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
