From e573582293576289bec4e665b583852d5c6bbe05 Mon Sep 17 00:00:00 2001 From: Ondrej Holy Date: Thu, 13 Sep 2018 17:33:59 +0200 Subject: [PATCH 1/5] glocalfile: Fix access::can-trash if parent is symlink G_FILE_ATTRIBUTE_ACCESS_CAN_TRASH can be set to a wrong value if its parent dir is a symlink. This is because the find_mountpoint_for() function tries to find mountpoint for a filepath and expands symlinks only in parent dirs. But in this case the path is already parent dir and needs to be expanded first... Closes: https://gitlab.gnome.org/GNOME/glib/issues/1522 --- gio/glocalfile.c | 104 +++++++++++++++++++++++++++++------------------ 1 file changed, 65 insertions(+), 39 deletions(-) diff --git a/gio/glocalfile.c b/gio/glocalfile.c index 30fa2281c..33b5ba3da 100644 --- a/gio/glocalfile.c +++ b/gio/glocalfile.c @@ -109,7 +109,7 @@ G_DEFINE_TYPE_WITH_CODE (GLocalFile, g_local_file, G_TYPE_OBJECT, G_IMPLEMENT_INTERFACE (G_TYPE_FILE, g_local_file_file_iface_init)) -static char *find_mountpoint_for (const char *file, dev_t dev); +static char *find_mountpoint_for (const char *file, dev_t dev, gboolean resolve_basename_symlink); static void g_local_file_finalize (GObject *object) @@ -791,7 +791,7 @@ get_mount_info (GFileInfo *fs_info, { mount_info = 0; - mountpoint = find_mountpoint_for (path, buf.st_dev); + mountpoint = find_mountpoint_for (path, buf.st_dev, FALSE); if (mountpoint == NULL) mountpoint = g_strdup ("/"); @@ -886,7 +886,7 @@ get_volume_for_path (const char *path) } static char * -find_mountpoint_for (const char *file, dev_t dev) +find_mountpoint_for (const char *file, dev_t dev, gboolean resolve_basename_symlink) { wchar_t *wpath; char *utf8_path; @@ -1132,7 +1132,7 @@ g_local_file_find_enclosing_mount (GFile *file, if (g_lstat (local->filename, &buf) != 0) goto error; - mountpoint = find_mountpoint_for (local->filename, buf.st_dev); + mountpoint = find_mountpoint_for (local->filename, buf.st_dev, FALSE); if (mountpoint == NULL) goto error; @@ -1584,12 +1584,51 @@ expand_symlink (const char *link) } static char * -get_parent (const char *path, +expand_symlinks (const char *path, + dev_t *dev) +{ + char *tmp, *target; + GStatBuf target_stat; + int num_recursions; + + target = g_strdup (path); + + num_recursions = 0; + do + { + if (g_lstat (target, &target_stat) != 0) + { + g_free (target); + return NULL; + } + + if (S_ISLNK (target_stat.st_mode)) + { + tmp = target; + target = expand_symlink (target); + g_free (tmp); + } + + num_recursions++; + if (num_recursions > 12) + { + g_free (target); + return NULL; + } + } + while (S_ISLNK (target_stat.st_mode)); + + if (dev) + *dev = target_stat.st_dev; + + return target; +} + +static char * +get_parent (const char *path, dev_t *parent_dev) { - char *parent, *tmp; - GStatBuf parent_stat; - int num_recursions; + char *parent, *res; char *path_copy; path_copy = strip_trailing_slashes (path); @@ -1604,32 +1643,10 @@ get_parent (const char *path, } g_free (path_copy); - num_recursions = 0; - do { - if (g_lstat (parent, &parent_stat) != 0) - { - g_free (parent); - return NULL; - } - - if (S_ISLNK (parent_stat.st_mode)) - { - tmp = parent; - parent = expand_symlink (parent); - g_free (tmp); - } - - num_recursions++; - if (num_recursions > 12) - { - g_free (parent); - return NULL; - } - } while (S_ISLNK (parent_stat.st_mode)); + res = expand_symlinks (parent, parent_dev); + g_free (parent); - *parent_dev = parent_stat.st_dev; - - return parent; + return res; } static char * @@ -1656,13 +1673,22 @@ expand_all_symlinks (const char *path) } static char * -find_mountpoint_for (const char *file, - dev_t dev) +find_mountpoint_for (const char *file, + dev_t dev, + gboolean resolve_basename_symlink) { char *dir, *parent; dev_t dir_dev, parent_dev; - dir = g_strdup (file); + if (resolve_basename_symlink) + { + dir = expand_symlinks (file, NULL); + if (dir == NULL) + return g_strdup (file); + } + else + dir = g_strdup (file); + dir_dev = dev; while (1) @@ -1693,7 +1719,7 @@ _g_local_file_find_topdir_for (const char *file) if (dir == NULL) return NULL; - mountpoint = find_mountpoint_for (dir, dir_dev); + mountpoint = find_mountpoint_for (dir, dir_dev, TRUE); g_free (dir); return mountpoint; @@ -1789,7 +1815,7 @@ _g_local_file_has_trash_dir (const char *dirname, dev_t dir_dev) if (dir_dev == home_dev) return TRUE; - topdir = find_mountpoint_for (dirname, dir_dev); + topdir = find_mountpoint_for (dirname, dir_dev, TRUE); if (topdir == NULL) return FALSE; @@ -1856,7 +1882,7 @@ _g_local_file_is_lost_found_dir (const char *path, dev_t path_dev) if (!g_str_has_suffix (path, "/lost+found")) goto out; - mount_dir = find_mountpoint_for (path, path_dev); + mount_dir = find_mountpoint_for (path, path_dev, FALSE); if (mount_dir == NULL) goto out; From d565484a6ae4df20faba3872bf97b69e09184a4c Mon Sep 17 00:00:00 2001 From: Ondrej Holy Date: Fri, 14 Sep 2018 14:53:30 +0200 Subject: [PATCH 2/5] glocalfile: Fix bug uri in trash test Fix typo in g_test_bug_base and move g_test_bug in the concrete test. --- gio/tests/trash.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gio/tests/trash.c b/gio/tests/trash.c index 176c4b9c3..b586f16f9 100644 --- a/gio/tests/trash.c +++ b/gio/tests/trash.c @@ -38,6 +38,8 @@ test_trash_not_supported (void) gchar *parent_dirname; GStatBuf parent_stat, home_stat; + g_test_bug ("251"); + /* The test assumes that tmp file is located on system internal mount. */ file = g_file_new_tmp ("test-trashXXXXXX", &stream, &error); parent_dirname = g_path_get_dirname (g_file_peek_path (file)); @@ -99,8 +101,7 @@ main (int argc, char *argv[]) { g_test_init (&argc, &argv, NULL); - g_test_bug_base ("htps://gitlab.gnome.org/GNOME/glib/issues/"); - g_test_bug ("251"); + g_test_bug_base ("https://gitlab.gnome.org/GNOME/glib/issues/"); g_test_add_func ("/trash/not-supported", test_trash_not_supported); From 92f7592ec08b55d4cc85953f8301a368eeb3c368 Mon Sep 17 00:00:00 2001 From: Ondrej Holy Date: Mon, 1 Oct 2018 13:45:17 +0200 Subject: [PATCH 3/5] glocalfile: Add test case for symlink expansion Test symlink expansion in find_mountpoint_for() function over _g_local_file_find_topdir_for(). find_mount_for() is crucial for many of glocalfile.c functionality (e.g. to determine correct trash location) and symlink expansion has to work properly. https://gitlab.gnome.org/GNOME/glib/issues/1522 --- gio/tests/trash.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/gio/tests/trash.c b/gio/tests/trash.c index b586f16f9..1055585e5 100644 --- a/gio/tests/trash.c +++ b/gio/tests/trash.c @@ -96,6 +96,69 @@ test_trash_not_supported (void) g_object_unref (file); } +/* Test that symlinks are properly expaned when looking for topdir (e.g. for trash folder). */ +static void +test_trash_symlinks (void) +{ + GFile *symlink; + GUnixMountEntry *target_mount, *tmp_mount, *symlink_mount, *target_over_symlink_mount; + gchar *target, *tmp, *target_over_symlink; + GError *error = NULL; + + g_test_bug ("1522"); + + /* The test assumes that ~/.local always exists. */ + target = g_build_filename (g_get_home_dir (), ".local", NULL); + target_mount = g_unix_mount_for (target, NULL); + g_assert_nonnull (target_mount); + g_test_message ("Target: %s (mount: %s)", target, g_unix_mount_get_mount_path (target_mount)); + + tmp = g_dir_make_tmp ("test-trashXXXXXX", &error); + tmp_mount = g_unix_mount_for (tmp, NULL); + g_assert_nonnull (tmp_mount); + g_test_message ("Tmp: %s (mount: %s)", tmp, g_unix_mount_get_mount_path (tmp_mount)); + + if (g_unix_mount_compare (target_mount, tmp_mount) == 0) + { + g_test_skip ("The tmp has to be on another mount than the home to run this test"); + + g_unix_mount_free (tmp_mount); + g_free (tmp); + g_unix_mount_free (target_mount); + g_free (target); + + return; + } + + symlink = g_file_new_build_filename (tmp, "symlink", NULL); + g_file_make_symbolic_link (symlink, g_get_home_dir (), NULL, &error); + g_assert_no_error (error); + + symlink_mount = g_unix_mount_for (g_file_peek_path (symlink), NULL); + g_assert_nonnull (symlink_mount); + g_test_message ("Symlink: %s (mount: %s)", g_file_peek_path (symlink), g_unix_mount_get_mount_path (symlink_mount)); + + g_assert_cmpint (g_unix_mount_compare (symlink_mount, tmp_mount), ==, 0); + + target_over_symlink = g_build_filename (g_file_peek_path (symlink), + ".local", + NULL); + target_over_symlink_mount = g_unix_mount_for (target_over_symlink, NULL); + g_assert_nonnull (symlink_mount); + g_test_message ("Target over symlink: %s (mount: %s)", target_over_symlink, g_unix_mount_get_mount_path (target_over_symlink_mount)); + + g_assert_cmpint (g_unix_mount_compare (target_over_symlink_mount, target_mount), ==, 0); + + g_unix_mount_free (target_over_symlink_mount); + g_unix_mount_free (symlink_mount); + g_free (target_over_symlink); + g_object_unref (symlink); + g_unix_mount_free (tmp_mount); + g_free (tmp); + g_unix_mount_free (target_mount); + g_free (target); +} + int main (int argc, char *argv[]) { @@ -104,6 +167,7 @@ main (int argc, char *argv[]) g_test_bug_base ("https://gitlab.gnome.org/GNOME/glib/issues/"); g_test_add_func ("/trash/not-supported", test_trash_not_supported); + g_test_add_func ("/trash/symlinks", test_trash_symlinks); return g_test_run (); } From 012646ce6b06447dad094eb6738717951848b571 Mon Sep 17 00:00:00 2001 From: Ondrej Holy Date: Fri, 21 Sep 2018 17:02:05 +0200 Subject: [PATCH 4/5] glocalfile: Return NULL if symlink expansion fails find_mountpoint_for() uses current file in case of error, because get_parent() returns NULL for error, but also if parent doesn't exist. Return "." from get_parent() if parent doesn't exist in order to differentiate the error state. --- gio/glocalfile.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/gio/glocalfile.c b/gio/glocalfile.c index 33b5ba3da..a547bcda0 100644 --- a/gio/glocalfile.c +++ b/gio/glocalfile.c @@ -1634,8 +1634,7 @@ get_parent (const char *path, path_copy = strip_trailing_slashes (path); parent = g_path_get_dirname (path_copy); - if (strcmp (parent, ".") == 0 || - strcmp (parent, path_copy) == 0) + if (strcmp (parent, ".") == 0) { g_free (parent); g_free (path_copy); @@ -1657,10 +1656,12 @@ expand_all_symlinks (const char *path) dev_t parent_dev; parent = get_parent (path, &parent_dev); - if (parent) + if (parent == NULL) + return NULL; + + if (g_strcmp0 (parent, "/") != 0) { parent_expanded = expand_all_symlinks (parent); - g_free (parent); basename = g_path_get_basename (path); res = g_build_filename (parent_expanded, basename, NULL); g_free (basename); @@ -1668,7 +1669,9 @@ expand_all_symlinks (const char *path) } else res = g_strdup (path); - + + g_free (parent); + return res; } @@ -1684,19 +1687,22 @@ find_mountpoint_for (const char *file, { dir = expand_symlinks (file, NULL); if (dir == NULL) - return g_strdup (file); + return NULL; } else dir = g_strdup (file); dir_dev = dev; - while (1) + while (g_strcmp0 (dir, "/") != 0) { parent = get_parent (dir, &parent_dev); if (parent == NULL) - return dir; - + { + g_free (dir); + return NULL; + } + if (parent_dev != dir_dev) { g_free (parent); @@ -1706,6 +1712,8 @@ find_mountpoint_for (const char *file, g_free (dir); dir = parent; } + + return dir; } char * @@ -1773,7 +1781,7 @@ try_make_relative (const char *path, base2 = expand_all_symlinks (base); relative = NULL; - if (path_has_prefix (path2, base2)) + if (path2 != NULL && base2 != NULL && path_has_prefix (path2, base2)) { relative = path2 + strlen (base2); while (*relative == '/') From aa5b7206e0e23ca280a6bd016619399eb6d046cc Mon Sep 17 00:00:00 2001 From: Ondrej Holy Date: Fri, 21 Sep 2018 16:12:51 +0200 Subject: [PATCH 5/5] glocalfile: Use MAXSYMLINKS when following symlinks Currently, readlink() is used only 12 times when expanding symlinks. However, kernel uses 40 for this purpose and it is defined as MAXSYMLINKS. Use that constant if available, or 40. See: https://github.com/torvalds/linux/include/linux/namei.h. --- gio/glocalfile.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/gio/glocalfile.c b/gio/glocalfile.c index a547bcda0..064755981 100644 --- a/gio/glocalfile.c +++ b/gio/glocalfile.c @@ -1610,7 +1610,15 @@ expand_symlinks (const char *path, } num_recursions++; - if (num_recursions > 12) + +#ifdef MAXSYMLINKS + if (num_recursions > MAXSYMLINKS) +#else + /* 40 is used in kernel sources currently: + * https://github.com/torvalds/linux/include/linux/namei.h + */ + if (num_recursions > 40) +#endif { g_free (target); return NULL;