GNOME Bugzilla – Bug 786217
Crash when calculating relative path
Last modified: 2017-08-24 09:31:49 UTC
I was getting a crash in my client when comparing relative paths of two davs:// URIs. Digging into it, I found some seemingly nonsense code? Patch attached to fix it, I think. I was getting the remainder==NULL case, where we try to g_strdup(1), but the other side of the if condition also seems like it needed to be changed. Am I crazy?
Created attachment 357501 [details] [review] Initial patch
Review of attachment 357501 [details] [review]: Thanks for that patch! This is definitely wrong, but as far as I can tell I am convinced that the surrounding code is also wrong...
Created attachment 357613 [details] [review] gdaemonfile: Fix relative path handling g_daemon_file_get_relative_path() fails for files with different mount_prefix and always return NULL. This can happen when comparing two files from two different origins, e.g. g_mount_get_root() and g_file_new_for_uri(): Mountspec for g_mount_get_root(): dav:host=example.com,user=test,prefix=%2Fdir Mountspec for g_file_new_for_uri("dav://test@example.com/dir/path"): dav:host=example.com,user=test The result from g_file_new_for_uri() is obviously descendant, but g_file_get_relative_path() returns NULL instead of path, because the code concatenates mount_prefix with path, which results in comparison of /dir/dir and /dir/path. Let's ignore mount_prefix when comparing.
Can you please test that it works correctly for you with the patches? I wonder that this buggy code is here for several years, it seems that the relative path functionality isn't heavily used...
Yes, that was the situation I ran into (g_mount_get_root vs g_file_new) and your patch fixed the issue for me. Thank you! After your patch, that function can probably be simplified further. The two code paths are nearly identical.
(By fixed the issue for me, I mean not only the crash, but get_relative_path returns sane results now too.)
Created attachment 357871 [details] [review] gdaemonfile: Fix relative path handling So I updated the patch, can you please check it again before pushing?
Yup! That still works for me.
Thanks for testing, but I've realized that the patches break shadow mount handling, which was the reason, why the current buggy implementation was introduced [1]. More fixes are needed for this reason... [1] https://bugzilla.gnome.org/show_bug.cgi?id=696279
Created attachment 358070 [details] [review] gdaemonfile: Fix relative path handling Just added comment.
Created attachment 358071 [details] [review] gdaemonfile: Fix g_file_equal for different mount_prefix g_daemon_file_equal() always return FALSE for files with different mount_prefix even though the files can be equal. It happen when comparing two files from different origins, e.g. g_mount_get_root() and g_file_new_for_uri(). Let's do the same which is done in _prefix_matches and _get_relative_path in order to fix this issue.
Created attachment 358072 [details] [review] proxy: Fix shadow mount handling for equal paths Automatic shadow mount handling in some cases works currently only thanks to the bug in g_daemon_file_prefix_matches implementation, which I am going to fix. The problematic case is when activation_root is equal to mount_root (e.g. Nextcloud integration). The bug in _prefix_matches causes that it succeeds also if the paths are equal, but this is wrong as per the docs. Let's use also g_file_equal when looking for shadow mounts in order to fix this issue.
Thanks for your contributions and testing. Let's push the patches, hopefully, it doesn't break anything else. Attachment 357501 [details] pushed as 33f6ff0 - Initial patch - reviewed Attachment 358070 [details] pushed as fbce9f8 - gdaemonfile: Fix relative path handling Attachment 358071 [details] pushed as 8cc160e - gdaemonfile: Fix g_file_equal for different mount_prefix Attachment 358072 [details] pushed as dd2b1ff - proxy: Fix shadow mount handling for equal paths
Nice! Thanks for the further fixes, looks good.
Pushed to gnome-3-24 also.