[Spice-devel] [spice-gtk] Unescape SpiceSession::uri component by component

[Hans de Goede]

Hi,

On 09/20/2012 11:53 AM, Christophe Fergeau wrote:
> Unescaping the whole URI and then parsing it is dangerous as
> the unescaping may (for example) add some extra '/' in the URI
> which are not part of a path. It's better to do the unescaping later
> once the URI has been split in separate components.
> This commit unescapes the path, host and query values. Handling escaped
> query values is important for usernames/passwords which might contain
> chars which are invalid in URIs.
> If the host is enclosed in [], it's intentionally not escaped as this
> contains an ipv6 URI, and may contain a %zone_id (see RFC4007). This is
> consistent with libvirt/libxml2 behaviour, not with what gvfs does.

You declare, set, and free an unescaped_path variable, but you don't
seem to use it anywhere ?

Regards,

Hans

> ---
>   gtk/spice-session.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
> index 5fbc1d2..68d1594 100644
> --- a/gtk/spice-session.c
> +++ b/gtk/spice-session.c
> @@ -247,18 +247,15 @@ static int spice_uri_create(SpiceSession *session, char *dest, int len)
>   static int spice_uri_parse(SpiceSession *session, const char *original_uri)
>   {
>       SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
> -    gchar key[32], value[128];
>       gchar *host = NULL, *port = NULL, *tls_port = NULL, *uri = NULL, *password = NULL;
> -    gchar **target_key;
>       gchar *path = NULL;
> +    gchar *unescaped_path = NULL;
>       gchar *authority = NULL;
>       gchar *query = NULL;
>
>       g_return_val_if_fail(original_uri != NULL, -1);
>
> -    uri = g_uri_unescape_string(original_uri, NULL);
> -    g_return_val_if_fail(uri != NULL, -1);
> -
> +    uri = g_strdup(original_uri);
>
>       /* Break up the URI into its various parts, scheme, authority,
>        * path (ignored) and query
> @@ -308,7 +305,7 @@ static int spice_uri_parse(SpiceSession *session, const char *original_uri)
>               tmp++;
>               port = g_strdup(tmp);
>           }
> -        host = g_strdup(authority);
> +        host = g_uri_unescape_string(authority, NULL);
>       }
>
>       if (path && !(g_str_equal(path, "") ||
> @@ -316,8 +313,12 @@ static int spice_uri_parse(SpiceSession *session, const char *original_uri)
>           g_warning("Unexpected path data '%s' for URI '%s'", path, uri);
>           /* don't fail, just ignore */
>       }
> +    unescaped_path = g_uri_unescape_string(path, NULL);
>
>       while (query && query[0] != '\0') {
> +        gchar key[32], value[128];
> +        gchar **target_key;
> +
>           int len;
>           if (sscanf(query, "%31[-a-zA-Z0-9]=%127[^;&]%n", key, value, &len) != 2) {
>               g_warning("Failed to parse URI query '%s'", query);
> @@ -344,7 +345,7 @@ static int spice_uri_parse(SpiceSession *session, const char *original_uri)
>                   g_warning("Double set of '%s' in URI '%s'", key, uri);
>                   goto fail;
>               }
> -            *target_key = g_strdup(value);
> +            *target_key = g_uri_unescape_string(value, NULL);
>           }
>       }
>
> @@ -355,6 +356,7 @@ static int spice_uri_parse(SpiceSession *session, const char *original_uri)
>
>       /* parsed ok -> apply */
>       g_free(uri);
> +    g_free(unescaped_path);
>       g_free(s->host);
>       g_free(s->port);
>       g_free(s->tls_port);
> @@ -367,6 +369,7 @@ static int spice_uri_parse(SpiceSession *session, const char *original_uri)
>
>   fail:
>       g_free(uri);
> +    g_free(unescaped_path);
>       g_free(host);
>       g_free(port);
>       g_free(tls_port);
>