GNOME Bugzilla – Bug 676626
g_file_new_for_commandline_arg() doesn't work well when directory has some colon
Last modified: 2018-05-24 14:15:00 UTC
Try the following: mkdir foo:bar gedit foo:bar/test gedit will fail to open the file with a "Could not open the file foo:///bar/test. gedit cannot handle foo: locations." error. I'm actually unsure if that's something that is fixable, as g_file_new_for_commandline_arg() works like this by design. But for URI schemes that we don't support, this feels wrong. What we could do is test if foo:bar is an existing directory, but I doubt we want to do some IO in g_file_new_for_commandline_arg(). I've no clue if this is just WONTFIX or if there is a proper solution. (And yes, I'm aware that this works fine if I use a proper URI for the file, or if I use the full path)
I encounter this problem a lot, because I usually name papers "Author: Title.pdf" and then try to open them using evince from a terminal. Right now, you use has_valid_scheme() to check whether the command line argument matches ^[A-Za-z0-9+.-]: and use new_for_uri or new_for_path depending on the outcome. Here are two alternatives: * You could additionally check whether the scheme matches one from the list returned by g_vfs_get_supported_uri_schemes(), and only use g_file_new_for_uri() in that case. The only harm this solutions would do is that the error message for non-existing resources would become "No such file" instead of "Location not supported". * You could check for ^[A-Za-z0-9+.-]:// instead. Technically, this might not be the best solution, but this is exactly what I'd expect from a today's end-user's perspective. In any case, I think that the documentation for g_file_new_for_commandline_arg() should be updated with a note on how ambiguous names are handled.
Created attachment 266428 [details] [review] A patch against the latest git version implementing my first suggestion
Created attachment 266429 [details] [review] A patch against the latest git version implementing my first suggestion I forgot to remove a curly brace. Here's the correct patch.
*** Bug 732914 has been marked as a duplicate of this bug. ***
Created attachment 290925 [details] [review] Open files with colon in their names Hi, I've also looked at this problem because of https://bugzilla.redhat.com/show_bug.cgi?id=1161374 and I think that the function "new_for_cmdline_arg()" could test whether the given file exists before using the name as an URI. Regards Marek
This is explicitly _not_ what new_for_cmdline_arg should do, because: 1) GFile is by design made so you can construct a GFile representing a file that does not necessarily exist (for example, if you'd like to use g_file_create...). Making this different for new_for_cmdline_arg would make GFile more inconsistent 2) There is a genuine use case, for example in gedit, because we allow 'gedit non-existing-file-name', which opens a new file with that name without a backing file. The file is created on first save in that case.
I thought about Marek's solution too, and have some objections about it. It has security implications if a program is run from a directory to which a third user has write access. The simplest example is a script that runs /usr/bin/some_app ssh://host/foo/bar as root from, say, /tmp, and relies on it's output. A malicious user could create a file in "/tmp/ssh:/host/foo/bar", and would thus have control over the input to the some_app program.
(In reply to comment #6) > This is explicitly _not_ what new_for_cmdline_arg should do, because: > > 1) GFile is by design made so you can construct a GFile representing a file > that does not necessarily exist (for example, if you'd like to use > g_file_create...). Making this different for new_for_cmdline_arg would make > GFile more inconsistent > 2) There is a genuine use case, for example in gedit, because we allow 'gedit > non-existing-file-name', which opens a new file with that name without a > backing file. The file is created on first save in that case. Looking at the modified "new_for_cmdline_arg()" it seems to me that the non-existing file wouldn't be a problem unless you want to name it like "file:for-me.txt". I've just tested it and it works for me to run "gedit aaa:bbb.txt".
jessevdk, I think you misunderstood what either of us suggests. This bug is about what gio should do with command line arguments that are both valid relative path names and valid URIs. Marek suggests to interpret the argument as a local file if a file with that name already exists and use the URI interpretation otherwise. My suggestion is to use the URI interpretation whenever and only if the associated scheme is supported by a gvfs backend.
I see, yes I went to quickly over the patch, sorry about that. I still think that not doing IO in new_for_cmdline_arg is better as it is one of the design choices of GFile to avoid that. This can always be done by the app in any case if someone wants to do it like this. If you do use IO, I think this API would need an additional _async variant as per the rest of the GFile API design.
The real problem is that GFile constructors really shouldn't be doing I/O at all, since any I/O it does is begging for races, especially ones testing for file existence. Amending this call to do I/O is breaking GIO's design, period. Testing whether the text before the colon is a registered scheme is at least more realistic as far as solutions go.
> Testing whether the text before the colon is a registered scheme is at least > more realistic as far as solutions go. Adding additional gvfs modules could render files that could previously be used unaccessible, but that's still an improvement over the current situation. Other than that, I don't see any downsides to my solution. Yet your comment doesn't sound overly approving - do you have any objections?
(In reply to comment #12) > > Testing whether the text before the colon is a registered scheme is at least > > more realistic as far as solutions go. > > Adding additional gvfs modules could render files that could previously be used > unaccessible, but that's still an improvement over the current situation. And that's the exact caveat I was thinking of when I made the comment. Racing VFS module registration at least seems more winnable than racing file creation tests. But injecting synchronous I/O into a call that previously did zero I/O is an absolute non-starter. I don't really have a better solution in mind, other than the obvious "if that's causing pain, don't do it." I'm not sure why colons in file paths are a desirable thing, given that on some platforms it'd be considered illegal, and in other cases it's at least ambiguous (e.g. "C://Hello.txt" - C isn't usually going to be a scheme, but I don't think there's anything actually stopping an application from registering it.)
> I'm not sure why colons in file paths are a desirable thing, given that on > some platforms it'd be considered illegal, and in other cases it's at least > ambiguous (e.g. "C://Hello.txt" - C isn't usually going to be a scheme, but I > don't think there's anything actually stopping an application from registering > it.) This ambiguity is what this bug is about. Currently, the only reason *not* to use colons in file names on common Linux file systems is that those files can't be passed as command line arguments to applications which use gio, because gio will assume that "E.A.Poe: The raven.pdf" is a request for the URI using the scheme "E.A.Poe". In OSes where colons are reserved the situation is even worse. In Windows *all* absolute path names are valid URIs and therefore cannot be used as command line arguments. The same goes for MacOS, if one uses a colon as the directory separator.
(In reply to comment #10) > I see, yes I went to quickly over the patch, sorry about that. I still think > that not doing IO in new_for_cmdline_arg is better as it is one of the design > choices of GFile to avoid that. This can always be done by the app in any case > if someone wants to do it like this. If you do use IO, I think this API would > need an additional _async variant as per the rest of the GFile API design. I thought that the g_stat() in g_get_current_dir() already does some IO so I didn't hesitate to add the g_file_test().
Created attachment 291190 [details] [review] GFile: require ':/' to appear in commandline URIs Don't treat a commandline argument as a URI unless it contains a valid URI scheme name followed by ":/". Previously we were only checking for the colon, which means that files like "Author: Title.pdf" would be considered as URIs. This still allows for doing the wrong thing if there is a directory that looks like a valid URI scheme, ending with a colon. We could make this more robust by requiring two slashes but that would break things that currently work, like 'gvfs-ls file:/'. One more very simple proposal...
IMO, requiring two slashes is the better variant. "foo:/bar/baz" is a valid relative path in Linux, and I'd prefer if ambiguities were resolved in favour of local files. (Also see comment #1, where I also suggested this as an alternative to my patch.) I think that it is important that the solution you decide upon still allows to somehow pass all possible URIs and files to the program. The schemes that ship with GVFS all support arbitrarily many slashes at the beginning of the hierarchical part; recent:, trash:, computer:, and network: are examples. So while trash:foo.jpg wouldn't work anymore with your patch, trash://foo.jpg still would. Is this a requirement within gfvs, or might there be third-party modules whose URIs can't be "fixed" by adding slashes?
This is not exactly true. It seems that 0 and 2 slashes are equivalent, as are 1 and 3. So: file: == file:// and trash: == trash:// file:/ == file:/// and trash:/ == trash:/// In particular, note that file: and file:// don't work, while file:/ and file:/// do. This is caused by the behaviour of g_filename_from_uri() in gconvert.c. It rejects anything that doesn't start with file:/ and then strips away two slashes if it finds them. It also appears to support URIs like file://my.hostname/path and it checks hostname for valid format, but doesn't actually use it for anything else. When launching apps via GDesktopAppInfo we've always given URIs as containing at least two slashes, so we're safe with respect to that (across every version of GLib). At this point, I'm mostly concerned about existing users who might be broken if doing something from a script that uses only one '/'. The fact that 'trash:' and the others also works comes as a bit of a surprise to me, though...
My bad! You are absolutely right, trash://foo.jpg does *not* work, and trash:///foo.jpg does.
(In reply to comment #16) > Created an attachment (id=291190) [details] [review] > GFile: require ':/' to appear in commandline URIs This seems reasonable to me. I was worried when I saw the patch initially that there might be a URI scheme that this would break, but all of them that I could think of are reasonably different from something that should ever be handled by GFile that they shouldn't pose a real-world problem. Does make me wish we'd go ahead and grow a GUri though.
(In reply to comment #20) > Does make me wish we'd go ahead and grow a GUri though. That's bug 489862, which I just added a comment too. Feel free to take my code and finish it. :)
*** Bug 791635 has been marked as a duplicate of this bug. ***
I’d be in favour of checking against g_vfs_get_supported_uri_schemes().
*** Bug 769649 has been marked as a duplicate of this bug. ***
*** Bug 592792 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/549.