Merge branch 'user-dirs-duplicates' into 'main'

utils: Fix a leak when user-dirs.dirs declares a variable twice

See merge request GNOME/glib!4819

glib/gutils.c

@@ -2279,21 +2279,20 @@ load_user_special_dirs_unlocked (void)
   const gchar *config_dir = NULL;
   const gchar *home_dir;
   gchar *config_file;
-  gchar *data;
+  char *data = NULL;
 
   config_dir = g_get_user_config_dir_unlocked ();
   config_file = g_build_filename (config_dir,
                                   "user-dirs.dirs",
                                   NULL);
-
-  if (!g_file_get_contents (config_file, &data, NULL, NULL))
-    {
-      g_free (config_file);
-      return;
-    }
-
   home_dir = g_get_home_dir_unlocked ();
-  load_user_special_dirs_from_string (data, home_dir, g_user_special_dirs);
+
+  if (g_file_get_contents (config_file, &data, NULL, NULL))
+    load_user_special_dirs_from_string (data, home_dir, g_user_special_dirs);
+
+  /* Special-case desktop for historical compatibility */
+  if (g_user_special_dirs[G_USER_DIRECTORY_DESKTOP] == NULL)
+    g_user_special_dirs[G_USER_DIRECTORY_DESKTOP] = g_build_filename (home_dir, "Desktop", NULL);
 
   g_free (data);
   g_free (config_file);
@@ -2337,18 +2336,18 @@ g_reload_user_special_dirs_cache (void)
       for (i = 0; i < G_USER_N_DIRECTORIES; i++)
         {
           old_val = old_g_user_special_dirs[i];
-          if (g_user_special_dirs[i] == NULL)
+
+          if (g_user_special_dirs[i] == NULL ||
+              g_strcmp0 (old_val, g_user_special_dirs[i]) != 0)
             {
-              g_user_special_dirs[i] = old_val;
+              g_ignore_leak (old_val);
             }
-          else if (g_strcmp0 (old_val, g_user_special_dirs[i]) == 0)
+          else
             {
               /* don't leak */
               g_free (g_user_special_dirs[i]);
               g_user_special_dirs[i] = old_val;
             }
-          else
-            g_free (old_val);
         }
 
       /* free the old array */
@@ -2392,16 +2391,7 @@ g_get_user_special_dir (GUserDirectory directory)
   if (G_UNLIKELY (g_user_special_dirs == NULL))
     {
       g_user_special_dirs = g_new0 (gchar *, G_USER_N_DIRECTORIES);
-
       load_user_special_dirs_unlocked ();
-
-      /* Special-case desktop for historical compatibility */
-      if (g_user_special_dirs[G_USER_DIRECTORY_DESKTOP] == NULL)
-        {
-          gchar *home_dir = g_build_home_dir ();
-          g_user_special_dirs[G_USER_DIRECTORY_DESKTOP] = g_build_filename (home_dir, "Desktop", NULL);
-          g_free (home_dir);
-        }
     }
   user_special_dir = g_user_special_dirs[directory];
 

glib/gutilsprivate.c

@@ -126,10 +126,15 @@ load_user_special_dirs_from_string (const gchar *string, const gchar *home_dir,
       for (len = strlen (d); len > 1 && d[len - 1] == '/'; len--)
         d[len - 1] = 0;
 
+      /* Duplicates override the previous value. This is not explicit in the
+       * spec, but given that the spec[1] is designed to allow user-dirs.dirs to
+       * be sourced in a shell, overriding is the behaviour that would imply.
+       *
+       * [1]: https://www.freedesktop.org/wiki/Software/xdg-user-dirs/ */
+      g_clear_pointer (&special_dirs[directory], g_free);
+
       if (is_relative)
-        {
+        special_dirs[directory] = g_build_filename (home_dir, d, NULL);
-          special_dirs[directory] = g_build_filename (home_dir, d, NULL);
-        }
       else
         special_dirs[directory] = g_strdup (d);
     }

glib/tests/meson.build

@@ -185,6 +185,7 @@ glib_tests = {
     'c_standards': c_standards.keys(),
   },
   'utils-isolated' : {},
+  'utils-unisolated' : {},
   'unicode' : {},
   'unicode-encoding' : {},
   'unicode-normalize': {},

glib/tests/utils-isolated.c

@@ -186,63 +186,6 @@ test_cleanup_doesnt_follow_symlinks (void)
 #endif
 }
 
-static void
-test_load_user_special_dirs_unlocked (void)
-{
-#if defined(HAVE_COCOA) || defined(G_OS_WIN32)
-  g_test_skip ("The user-dirs.dirs parser is not used on this platform.");
-#else
-  g_test_summary ("Tests error and corner cases of user-dirs.dirs content.");
-  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/merge_requests/4800");
-
-  if (g_test_subprocess ())
-    {
-      gchar *user_dirs_file;
-      const gchar *config_dir;
-      const gchar *dir;
-      gchar *expected;
-      gboolean result;
-
-      config_dir = g_get_user_config_dir ();
-      user_dirs_file = g_build_filename (config_dir, "user-dirs.dirs", NULL);
-      g_mkdir_with_parents (g_path_get_dirname (user_dirs_file), 0700);
-      result = g_file_set_contents (user_dirs_file,
-                                    "XDG_DESKTOP_DIR = \"/root\"\nXDG_DESKTOP_DIR = \"$HOMER/Desktop\"\n"
-                                    "XDG_DOCUMENTS_DIR = \"$HOME\"\n"
-                                    "XDG_DOWNLOAD_DIR = \"$HOME/Downloads\"\n"
-                                    "XDG_MUSIC_DIR = \"///\"\n"
-                                    "XDG_PICTURES_DIR = \"/\"\nXDG_DOWNLOAD_DIR = \"/dev/null\n",
-                                    -1, NULL);
-      g_assert_true (result);
-      g_free (user_dirs_file);
-
-      g_reload_user_special_dirs_cache ();
-
-      dir = g_get_user_special_dir (G_USER_DIRECTORY_DESKTOP);
-      g_assert_cmpstr (dir, ==, "/root");
-
-      dir = g_get_user_special_dir (G_USER_DIRECTORY_DOCUMENTS);
-      g_assert_cmpstr (dir, ==, g_get_home_dir ());
-
-      expected = g_build_filename (g_get_home_dir (), "Downloads", NULL);
-      dir = g_get_user_special_dir (G_USER_DIRECTORY_DOWNLOAD);
-      g_assert_cmpstr (dir, ==, expected);
-      g_free (expected);
-
-      dir = g_get_user_special_dir (G_USER_DIRECTORY_MUSIC);
-      g_assert_cmpstr (dir, ==, "/");
-
-      dir = g_get_user_special_dir (G_USER_DIRECTORY_PICTURES);
-      g_assert_cmpstr (dir, ==, "/");
-    }
-  else
-    {
-      g_test_trap_subprocess (NULL, 0, G_TEST_SUBPROCESS_DEFAULT);
-      g_test_trap_assert_passed ();
-    }
-#endif
-}
-
 int
 main (int   argc,
       char *argv[])
@@ -255,7 +198,6 @@ main (int   argc,
 
   g_test_add_func ("/utils-isolated/tmp-dir", test_tmp_dir);
   g_test_add_func ("/utils-isolated/home-dir", test_home_dir);
-  g_test_add_func ("/utils-isolated/load-user-special-dirs-unlocked", test_load_user_special_dirs_unlocked);
   g_test_add_func ("/utils-isolated/user-cache-dir", test_user_cache_dir);
   g_test_add_func ("/utils-isolated/system-config-dirs", test_system_config_dirs);
   g_test_add_func ("/utils-isolated/user-config-dir", test_user_config_dir);

glib/tests/utils-unisolated.c

@@ -0,0 +1,110 @@
+/* Unit tests for utilities
+ * Copyright (C) 2010 Red Hat, Inc.
+ *
+ * SPDX-License-Identifier: LicenseRef-old-glib-tests
+ *
+ * This work is provided "as is"; redistribution and modification
+ * in whole or in part, in any medium, physical or electronic is
+ * permitted without restriction.
+ *
+ * This work is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ * In no event shall the authors or contributors be liable for any
+ * direct, indirect, incidental, special, exemplary, or consequential
+ * damages (including, but not limited to, procurement of substitute
+ * goods or services; loss of use, data, or profits; or business
+ * interruption) however caused and on any theory of liability, whether
+ * in contract, strict liability, or tort (including negligence or
+ * otherwise) arising in any way out of the use of this software, even
+ * if advised of the possibility of such damage.
+ *
+ * Author: Matthias Clasen
+ */
+
+#include "glib.h"
+
+static void
+test_xdg_dirs (void)
+{
+#ifdef G_OS_UNIX
+  char *xdg = NULL;
+  const char *dir;
+  const char * const *dirs;
+  char *s = NULL;
+
+  xdg = g_strdup (g_getenv ("XDG_CONFIG_HOME"));
+  if (!xdg)
+    xdg = g_build_filename (g_get_home_dir (), ".config", NULL);
+
+  dir = g_get_user_config_dir ();
+
+  g_assert_cmpstr (dir, ==, xdg);
+  g_free (xdg);
+
+  xdg = g_strdup (g_getenv ("XDG_DATA_HOME"));
+  if (!xdg)
+    xdg = g_build_filename (g_get_home_dir (), ".local", "share", NULL);
+
+  dir = g_get_user_data_dir ();
+
+  g_assert_cmpstr (dir, ==, xdg);
+  g_free (xdg);
+
+  xdg = g_strdup (g_getenv ("XDG_CACHE_HOME"));
+  if (!xdg)
+    xdg = g_build_filename (g_get_home_dir (), ".cache", NULL);
+
+  dir = g_get_user_cache_dir ();
+
+  g_assert_cmpstr (dir, ==, xdg);
+  g_free (xdg);
+
+  xdg = g_strdup (g_getenv ("XDG_STATE_HOME"));
+  if (!xdg)
+    xdg = g_build_filename (g_get_home_dir (), ".local/state", NULL);
+
+  dir = g_get_user_state_dir ();
+
+  g_assert_cmpstr (dir, ==, xdg);
+  g_free (xdg);
+
+  xdg = g_strdup (g_getenv ("XDG_RUNTIME_DIR"));
+  if (!xdg)
+    xdg = g_strdup (g_get_user_cache_dir ());
+
+  dir = g_get_user_runtime_dir ();
+
+  g_assert_cmpstr (dir, ==, xdg);
+  g_free (xdg);
+
+  xdg = (gchar *)g_getenv ("XDG_CONFIG_DIRS");
+  if (!xdg)
+    xdg = "/etc/xdg";
+
+  dirs = g_get_system_config_dirs ();
+
+  s = g_strjoinv (":", (gchar **)dirs);
+
+  g_assert_cmpstr (s, ==, xdg);
+
+  g_free (s);
+#else
+  g_test_skip ("User special dirs are not defined using environment variables on non-Unix systems");
+#endif
+}
+
+int
+main (int   argc,
+      char *argv[])
+{
+  /* Note: We do *not* use G_TEST_OPTION_ISOLATE_DIRS here, as we want to test
+   * the calculation of the XDG dirs from a real environment. */
+  g_test_init (&argc, &argv, NULL);
+
+  /* You almost certainly want to add a test to glib/tests/utils.c instead of here. */
+  g_test_add_func ("/utils/xdgdirs", test_xdg_dirs);
+
+  return g_test_run ();
+}

glib/tests/utils.c

@@ -23,6 +23,8 @@
  * Author: Matthias Clasen
  */
 
+#include "config.h"
+
 #ifndef GLIB_DISABLE_DEPRECATION_WARNINGS
 #define GLIB_DISABLE_DEPRECATION_WARNINGS
 #endif
@@ -817,88 +819,8 @@ test_hostname (void)
   g_assert_true (g_utf8_validate (name, -1, NULL));
 }
 
-#ifdef G_OS_UNIX
 static void
-test_xdg_dirs (void)
+test_user_special_dirs_desktop (void)
-{
-  gchar *xdg;
-  const gchar *dir;
-  const gchar * const *dirs;
-  gchar *s;
-
-  xdg = g_strdup (g_getenv ("XDG_CONFIG_HOME"));
-  if (!xdg)
-    xdg = g_build_filename (g_get_home_dir (), ".config", NULL);
-
-  dir = g_get_user_config_dir ();
-
-  g_assert_cmpstr (dir, ==, xdg);
-  g_free (xdg);
-
-  xdg = g_strdup (g_getenv ("XDG_DATA_HOME"));
-  if (!xdg)
-    xdg = g_build_filename (g_get_home_dir (), ".local", "share", NULL);
-
-  dir = g_get_user_data_dir ();
-
-  g_assert_cmpstr (dir, ==, xdg);
-  g_free (xdg);
-
-  xdg = g_strdup (g_getenv ("XDG_CACHE_HOME"));
-  if (!xdg)
-    xdg = g_build_filename (g_get_home_dir (), ".cache", NULL);
-
-  dir = g_get_user_cache_dir ();
-
-  g_assert_cmpstr (dir, ==, xdg);
-  g_free (xdg);
-
-  xdg = g_strdup (g_getenv ("XDG_STATE_HOME"));
-  if (!xdg)
-    xdg = g_build_filename (g_get_home_dir (), ".local/state", NULL);
-
-  dir = g_get_user_state_dir ();
-
-  g_assert_cmpstr (dir, ==, xdg);
-  g_free (xdg);
-
-  xdg = g_strdup (g_getenv ("XDG_RUNTIME_DIR"));
-  if (!xdg)
-    xdg = g_strdup (g_get_user_cache_dir ());
-
-  dir = g_get_user_runtime_dir ();
-
-  g_assert_cmpstr (dir, ==, xdg);
-  g_free (xdg);
-
-  xdg = (gchar *)g_getenv ("XDG_CONFIG_DIRS");
-  if (!xdg)
-    xdg = "/etc/xdg";
-
-  dirs = g_get_system_config_dirs ();
-
-  s = g_strjoinv (":", (gchar **)dirs);
-
-  g_assert_cmpstr (s, ==, xdg);
-
-  g_free (s);
-}
-#endif
-
-static void
-test_special_dir (void)
-{
-  const gchar *dir, *dir2;
-
-  dir = g_get_user_special_dir (G_USER_DIRECTORY_DESKTOP);
-  g_reload_user_special_dirs_cache ();
-  dir2 = g_get_user_special_dir (G_USER_DIRECTORY_DESKTOP);
-
-  g_assert_cmpstr (dir, ==, dir2);
-}
-
-static void
-test_desktop_special_dir (void)
 {
   const gchar *dir, *dir2;
 
@@ -910,6 +832,161 @@ test_desktop_special_dir (void)
   g_assert_nonnull (dir2);
 }
 
+#if !defined(HAVE_COCOA) && !defined(G_OS_WIN32)
+#define USES_USER_DIRS_DIRS 1
+#endif
+
+/* Write a mock user-dirs.dirs file with the given @content. This must be used
+ * with `G_TEST_OPTION_ISOLATE_DIRS`. See
+ * [`xdg-user-dirs-update(1)`](man:xdg-user-dirs-update(1)) for the specification. */
+#ifdef USES_USER_DIRS_DIRS
+static void
+set_mock_user_dirs_dirs_file (const char *content)
+{
+  char *user_dirs_file = NULL;
+  char *user_dirs_parent = NULL;
+  const char *config_dir;
+  GError *local_error = NULL;
+
+  config_dir = g_get_user_config_dir ();
+
+  /* Double check we’re running under G_TEST_OPTION_ISOLATE_DIRS and not about
+   * to overwrite actual user data. */
+  g_assert (g_str_has_prefix (config_dir, g_get_tmp_dir ()));
+
+  user_dirs_file = g_build_filename (config_dir, "user-dirs.dirs", NULL);
+  user_dirs_parent = g_path_get_dirname (user_dirs_file);
+  g_mkdir_with_parents (user_dirs_parent, 0700);
+  g_file_set_contents (user_dirs_file, content, -1, &local_error);
+  g_assert_no_error (local_error);
+  g_free (user_dirs_file);
+  g_free (user_dirs_parent);
+}
+#endif  /* USES_USER_DIRS_DIRS */
+
+static void
+test_user_special_dirs_load_unlocked (void)
+{
+#ifndef USES_USER_DIRS_DIRS
+  g_test_skip ("The user-dirs.dirs parser is not used on this platform.");
+#else
+  g_test_summary ("Tests error and corner cases of user-dirs.dirs content.");
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/merge_requests/4800");
+
+  if (g_test_subprocess ())
+    {
+      const gchar *dir;
+      gchar *expected;
+
+      set_mock_user_dirs_dirs_file ("XDG_DESKTOP_DIR = \"/root\"\nXDG_DESKTOP_DIR = \"$HOMER/Desktop\"\n"
+                                    "XDG_DOCUMENTS_DIR = \"$HOME\"\n"
+                                    "XDG_DOWNLOAD_DIR = \"$HOME/Downloads\"\n"
+                                    "XDG_MUSIC_DIR = \"///\"\n"
+                                    "XDG_PICTURES_DIR = \"$HOME/Pictures\"\n"
+                                    "XDG_PICTURES_DIR = \"/\"\nXDG_DOWNLOAD_DIR = \"/dev/null\n");
+
+      g_reload_user_special_dirs_cache ();
+
+      dir = g_get_user_special_dir (G_USER_DIRECTORY_DESKTOP);
+      g_assert_cmpstr (dir, ==, "/root");
+
+      dir = g_get_user_special_dir (G_USER_DIRECTORY_DOCUMENTS);
+      g_assert_cmpstr (dir, ==, g_get_home_dir ());
+
+      expected = g_build_filename (g_get_home_dir (), "Downloads", NULL);
+      dir = g_get_user_special_dir (G_USER_DIRECTORY_DOWNLOAD);
+      g_assert_cmpstr (dir, ==, expected);
+      g_free (expected);
+
+      dir = g_get_user_special_dir (G_USER_DIRECTORY_MUSIC);
+      g_assert_cmpstr (dir, ==, "/");
+
+      dir = g_get_user_special_dir (G_USER_DIRECTORY_PICTURES);
+      g_assert_cmpstr (dir, ==, "/");
+    }
+  else
+    {
+      g_test_trap_subprocess (NULL, 0, G_TEST_SUBPROCESS_DEFAULT);
+      g_test_trap_assert_passed ();
+    }
+#endif
+}
+
+static void
+test_user_special_dirs_reload_leaks (void)
+{
+#ifndef USES_USER_DIRS_DIRS
+  g_test_skip ("The user-dirs.dirs parser is not used on this platform.");
+#else
+  g_test_summary ("Tests that old user special dirs values are deliberately leaked on reload.");
+
+  if (g_test_subprocess ())
+    {
+      const char *original_special_dirs[G_USER_N_DIRECTORIES] = { NULL, };
+      const char *new_special_dirs[G_USER_N_DIRECTORIES] = { NULL, };
+      size_t i;
+
+      /* Set some original values for the variables and store them all. */
+      set_mock_user_dirs_dirs_file ("XDG_DESKTOP_DIR = \"/original/desktop/dir\"\n"
+                                    "XDG_DOCUMENTS_DIR = \"/original/documents/dir\"\n"
+                                    "XDG_DOWNLOAD_DIR = \"/original/download/dir\"\n");
+      g_reload_user_special_dirs_cache ();
+
+      for (i = 0; i < G_USER_N_DIRECTORIES; i++)
+        original_special_dirs[(GUserDirectory) i] = g_get_user_special_dir ((GUserDirectory) i);
+
+      g_assert_cmpstr (original_special_dirs[G_USER_DIRECTORY_DESKTOP], ==, "/original/desktop/dir");
+      g_assert_cmpstr (original_special_dirs[G_USER_DIRECTORY_DOCUMENTS], ==, "/original/documents/dir");
+      g_assert_cmpstr (original_special_dirs[G_USER_DIRECTORY_DOWNLOAD], ==, "/original/download/dir");
+
+      /* Update the values and reload them. Change some, keep others the same, drop some. */
+      set_mock_user_dirs_dirs_file ("XDG_DESKTOP_DIR = \"/new/desktop/dir\"\n"
+                                    "XDG_DOCUMENTS_DIR = \"/original/documents/dir\"\n"
+                                    "XDG_MUSIC_DIR = \"/new/music/dir\"\n");
+      g_reload_user_special_dirs_cache ();
+
+      for (i = 0; i < G_USER_N_DIRECTORIES; i++)
+        new_special_dirs[(GUserDirectory) i] = g_get_user_special_dir ((GUserDirectory) i);
+
+      /* We expect all the original strings to still be accessible. Those which
+       * have the same string values as the new ones will not have changed. The
+       * ones which have changed string value should have their original values
+       * deliberately leaked so that const pointers in the program don’t break.
+       * See the documentation for g_reload_user_special_dirs_cache(). */
+      for (i = 0; i < G_USER_N_DIRECTORIES; i++)
+        g_test_message ("Special dir %" G_GSIZE_FORMAT ", original value %s, new value %s",
+                        i, original_special_dirs[(GUserDirectory) i],
+                        new_special_dirs[(GUserDirectory) i]);
+
+      g_assert_cmpstr (original_special_dirs[G_USER_DIRECTORY_DESKTOP], ==, "/original/desktop/dir");
+      g_assert_cmpstr (original_special_dirs[G_USER_DIRECTORY_DOCUMENTS], ==, "/original/documents/dir");
+      g_assert_cmpstr (original_special_dirs[G_USER_DIRECTORY_DOWNLOAD], ==, "/original/download/dir");
+      g_assert_cmpstr (original_special_dirs[G_USER_DIRECTORY_MUSIC], ==, NULL);
+      g_assert_cmpstr (new_special_dirs[G_USER_DIRECTORY_DESKTOP], ==, "/new/desktop/dir");
+      g_assert_cmpstr (new_special_dirs[G_USER_DIRECTORY_DOCUMENTS], ==, "/original/documents/dir");
+      g_assert_cmpstr (new_special_dirs[G_USER_DIRECTORY_DOWNLOAD], ==, NULL);
+      g_assert_cmpstr (new_special_dirs[G_USER_DIRECTORY_MUSIC], ==, "/new/music/dir");
+
+      /* We expect exactly these two strings to leak. Rather than mark them as
+       * leaked (which we can do for asan, but not for valgrind at runtime), go
+       * ahead and free them. This means we can catch unexpected additional
+       * leaks, and also unexpected double-frees.
+       *
+       * This is definitely *not* something that production code should be doing.
+       *
+       * We can (relatively) safely do it here because this test is running in
+       * a subprocess which is about to terminate. */
+      g_free ((char *) original_special_dirs[G_USER_DIRECTORY_DESKTOP]);
+      g_free ((char *) original_special_dirs[G_USER_DIRECTORY_DOWNLOAD]);
+    }
+  else
+    {
+      g_test_trap_subprocess (NULL, 0, G_TEST_SUBPROCESS_DEFAULT);
+      g_test_trap_assert_passed ();
+    }
+#endif
+}
+
 static void
 test_os_info (void)
 {
@@ -1353,7 +1430,7 @@ main (int   argc,
    */
   g_set_prgname (argv[0]);
 
-  g_test_init (&argc, &argv, NULL);
+  g_test_init (&argc, &argv, G_TEST_OPTION_ISOLATE_DIRS, NULL);
 
   g_test_add_func ("/utils/language-names", test_language_names);
   g_test_add_func ("/utils/locale-variants", test_locale_variants);
@@ -1374,11 +1451,9 @@ main (int   argc,
   g_test_add_func ("/utils/username", test_username);
   g_test_add_func ("/utils/realname", test_realname);
   g_test_add_func ("/utils/hostname", test_hostname);
-#ifdef G_OS_UNIX
+  g_test_add_func ("/utils/user-special-dirs/desktop", test_user_special_dirs_desktop);
-  g_test_add_func ("/utils/xdgdirs", test_xdg_dirs);
+  g_test_add_func ("/utils/user-special-dirs/load-unlocked", test_user_special_dirs_load_unlocked);
-#endif
+  g_test_add_func ("/utils/user-special-dirs/reload-leaks", test_user_special_dirs_reload_leaks);
-  g_test_add_func ("/utils/specialdir", test_special_dir);
-  g_test_add_func ("/utils/specialdir/desktop", test_desktop_special_dir);
   g_test_add_func ("/utils/os-info", test_os_info);
   g_test_add_func ("/utils/clear-pointer", test_clear_pointer);
   g_test_add_func ("/utils/clear-pointer-cast", test_clear_pointer_cast);