Re: [Nautilus-list] bug 1462

[Darin Adler <darin eazel com>]

on 8/24/00 4:54 AM, Kenneth Kocienda at kocienda kocienda com wrote:

> +/* Transform a path so it safe to pass to the system shell */
> +char *                  nautilus_get_shell_escaped_path     (const char
> *path);

This function shell-escapes an arbitrary string, not specifically a path. If
should be named:

        nautilus_shell_escape_string

to match the gnome_vfs_escape_string function. Note that the
gnome_vfs_escape_path_string is a variant that has special handling
specifically for paths, but this function works for any string.

> +/* 
> + * Transform a path so it safe to pass to the system shell
> + * This function returns a newly-allocated string
> + * that transforms the input string by enclosing it in single quotes,
> + * and handling all embedded single quotes in the input string by
> + * escaping them with this four-character sequence: '\''
> + */

Our comment first line style is:

    /* comment

not

    /*
     * comment

We inherited this from Gtk.

> +char *          
> +nautilus_get_shell_escaped_path (const char *path) {

Brace goes in the line after, not the end of the line.

You need a check for NULL. The normal approach is to make NULL illegal like
this:

        g_return_val_if_fail (path != NULL, NULL);

Perhaps this function could return just a strdup of the path if there are no
characters at all that are special to the shell. Something like this:

        #define SHELL_METACHARACTERS "|&;()<> \t\n'\""

        /* If there are no metacharacters, then no need to escape. */
        if (strpbrk (path, SHELL_METACHARACTERS) == NULL) {
            return g_strdup (path);
        }

Tests are vital. You should write the tests at the same time as the
function.

> Index: libnautilus-extensions/nautilus-program-choosing.c
> /**
> * nautilus_launch_application_from_command:
> * 
> @@ -300,9 +299,20 @@
> nautilus_launch_application_from_command (const char *command_string, const
> char *uri)
> {
> char *full_command;
> +    char *uri_or_path;

This variable should be named "path", not "uri_or_path", given how the
function currently works. It never holds a URI. However, if we change this
function to simply pass this second string as a parameter, then the function
parameter's name could be changed to "uri_or_path" or even to "argument" or
"parameter", since this function doesn't need to know if it's a URI, a path,
or anything else about it (see .

> +    char *shell_escaped_path;

This function is missing the obligatory parameter check (not your fault of
course since you didn't write it):

        g_return_val_if_fail (command_string != NULL, NULL);

> if (uri != NULL) {
> -        full_command = g_strconcat (command_string, " ", uri, " &", NULL);
> +        uri_or_path = nautilus_get_local_path_from_uri (uri);
> +        if (uri_or_path != NULL) {
> +                shell_escaped_path = nautilus_get_shell_escaped_path
>                          (uri_or_path);
> +                full_command = g_strconcat (command_string, " ",
>                                              shell_escaped_path, " &",
>                                              NULL);
> +                g_free (uri_or_path);
> +                g_free (shell_escaped_path);
> +        }
> +        else {
> +                full_command = g_strconcat (command_string, " ", uri, " &",
>                                              NULL);
> +        }

This function as written will pass either an unescaped URI or an escaped
path to the application. There are 3 small problems with this:

    1) Some applications can not handle a URI; so it's not OK to just pass a
URI to an unknown application. It's wrong to have this low-level function
choose whether to pass a path or URI. The decision of whether to pass a URI
or path must be made at a higher level that knows which applications can
accept URIs. This information is kept in the MIME info database. This does
not represent a problem with the patch, but rather a separate issue. It's up
to a higher level of the code to look up the application's capability and
determine how to pass the file location. It can be fixed separately from
this change.

    2) Converting the URI to a path might result in the pathological case
where the path looks like a URI (for example, if the file's name is
"http://www.yahoo.com";) and there's no reason to do it if the application
can handle URIs. Thus, if the application can handle a URI, we should pass
it a URI. If it can't handle a URI, it's an error to call this function with
a URI (see part 1 above). This does represent a problem with the patch; it's
unacceptable to do nautilus_get_local_path_from_uri and then just pass the
URI if that call fails; we need a function that does either a path or a URI,
not one that does both.

    3) Shell escaping is needed for some URIs too, not just for paths,
unless we have a guarantee that all the shell metacharacters from the URI
are already escaped using URI %-sequence escaping. Since the escaping is to
get the argument through the shell intact to the application, not because
the argument is a path, it's smarter to just always do the escaping and not
make assumptions about the parameter. This does represent a problem with the
patch I think; it's unacceptable to attempt to pass the URI without
shell-escaping it unless we are certain it's already using URI-escaping for
all shell metacharacters, and I'm not sure this function requires that
(certainly there's no comment to that effect, nor assert or other runtime
check).

    -- Darin