GNU bug report logs - #75688
Replace wrapper scripts with search path value files in search-paths.d

Previous Next

Full log

Message #67 received at 75688 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: iyzsong <at> envs.net
Cc: 75688 <at> debbugs.gnu.org, 宋文武 <iyzsong <at> member.fsf.org>,
 Vivien Kraus <vivien <at> planete-kraus.eu>,
 Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Subject: Re: bug#75688: GUIX_ify harmful environment variables and replace
 wrapper scripts with search path value files
Date: Mon, 27 Jan 2025 11:59:05 +0900

Hello,

iyzsong <at> envs.net writes:

> From: 宋文武 <iyzsong <at> member.fsf.org>
>
> Add a new function "g_build_guix_search_path_dirs" to GLIB, which in addition
> to environment variable, read search path values from etc/search-paths.d

Some typo/grammar nitpicks:

environment variableS, readS search path values from THE
etc/search-paths.d ...

> directory of the current executable.  This can be used to replace wrapper
> scripts.
>
> Use it for GUIX_GSETTINGS_SCHEMA_DIR, GUIX_GIO_EXTRA_MODULES,
> GUIX_XDG_DATA_DIRS and GUIX_XDG_CONFIG_DIRS.
>
> * gnu/packages/patches/glib-guix-search-paths.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Register it.
> * gnu/packages/glib.scm (glib)[source]: Add patch.
> [native-search-paths]: Add GUIX_GSETTINGS_SCHEMA_DIR, Replace

nitpick: I'd use a '.  ' instead of ', ' above (two sentences).

[...]

> +++ b/gnu/packages/patches/glib-guix-search-paths.patch
> @@ -0,0 +1,158 @@

I think it'd be nice to forward a subset of this patch that implements
just the loading environment variable from a file, as that mechanism
seems like it could be generally useful (and upstreaming it would lower
the maintenance burden for us).

> +diff --git a/gio/giomodule.c b/gio/giomodule.c
> +index 76c2028..49b02bb 100644
> +--- a/gio/giomodule.c
> ++++ b/gio/giomodule.c
> +@@ -1330,6 +1330,13 @@ _g_io_modules_ensure_loaded (void)
> +       g_io_modules_scan_all_in_directory_with_scope (module_dir, scope);
> +       g_free (module_dir);
> +
> ++      /* GUIX: Load gio modules from GUIX_GIO_EXTRA_MODULES */

Let's not add credence to the Reddit's GUIX misspelling ;-).  I'd drop
'GUIX: ' from the beginning of your comment, as it's already clear from
the variable name.  Please add a period to make it a complete sentence.

[...]

> +diff --git a/glib/gutils.c b/glib/gutils.c
> +index 8628a56..bc21efc 100644

[...]

> ++gchar **
> ++g_build_guix_search_path_dirs (const gchar *variable)

> ++{
> ++  gchar **dirs = NULL;
> ++  char *value = NULL;
> ++  GStrvBuilder *builder = g_strv_builder_new ();
> ++
> ++#if defined(__linux__) || defined(__gnu_hurd__)
> ++  /* First add paths from the etc/search-paths.d, which can be used to replace wrapper script. */

I'd ensure all lines wrapped around the 80 characters mark (here and
everywhere else).

> ++  gchar *exe_path = g_file_read_link ("/proc/self/exe", NULL);
> ++  gchar *out_path = NULL;
> ++  gchar *search_paths_d = NULL;
> ++
> ++  /* We install executables under "bin" or "libexec", can also be a subdirectory of "libexec". */
> ++  if (exe_path && (g_str_match_string("/bin/", exe_path, FALSE) ||
> ++                   g_str_match_string("/libexec/", exe_path, FALSE))) {

Perhaps these 'bin' and 'libexec' hard-coded names should come from the
build system of gdk-pixbuf, in case a distro uses different names across
its package collection (to make it more general).

> ++    /* Find output directory, which is the parent directory of "bin" or "libexec". */
> ++    out_path = g_path_get_dirname (exe_path);
> ++    while (g_str_match_string("/bin/", out_path, FALSE) ||
> ++           g_str_match_string("/libexec/", out_path, FALSE)) {
> ++      gchar *dir_path = out_path;

Is the intent above to *copy* out_path into dir_path?  Currently that's
not done; we just point another pointer to it.

> ++      out_path = g_path_get_dirname (dir_path);

If g_path_get_dirname mutates dir_path, than dir_path should be a string
copy.  Otherwise if it doesn't get mutated by the call, we should be
able to use just:

out_path = g_path_get_dirname (out_path);

> ++      g_free (dir_path);
> ++    }
> ++
> ++    /* Now add paths from etc/search-paths.d/VARIABLE file. */
> ++    search_paths_d = g_build_filename (out_path, "etc", "search-paths.d", NULL);
> ++    if (g_file_test (search_paths_d, G_FILE_TEST_EXISTS)) {
> ++      gchar *var_path = g_build_filename (search_paths_d, variable, NULL);
> ++      if (g_file_get_contents (var_path, &value, NULL, NULL)) {
> ++        dirs = g_strsplit (value, G_SEARCHPATH_SEPARATOR_S, 0);
> ++        g_strv_builder_addv (builder, (const gchar **) dirs);
> ++        g_strfreev (dirs);
> ++        g_free (value);
> ++      }
> ++      g_free (var_path);
> ++    }
> ++  }
> ++
> ++  g_free (exe_path);
> ++  g_free (out_path);
> ++  g_free (search_paths_d);
> ++#endif
> ++
> ++  /* Then add paths from the environment variable. */
> ++  gboolean is_setuid = GLIB_PRIVATE_CALL (g_check_setuid) ();
> ++  if (is_setuid) /* we don't want to access arbitrary files when running as setuid. */
> ++    value = NULL;
> ++  else
> ++    value = g_strdup (g_getenv (variable));
> ++
> ++  if (value && value[0]) {
> ++      dirs = g_strsplit (value, G_SEARCHPATH_SEPARATOR_S, 0);
> ++      g_strv_builder_addv (builder, (const gchar **) dirs);
> ++      g_strfreev (dirs);
> ++  }
> ++  g_free (value);
> ++
> ++  dirs = g_strv_builder_end (builder);
> ++  g_strv_builder_unref (builder);
> ++  return dirs;
> ++}

Apart from my above comments, this looks good to me.  I think I'd stress
once more the value of upstreaming as much of this to ease maintenance
in the future.

Thanks for this novel idea/implementation!

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.