[vlc-devel] [PATCH 1/4] vlc_url: parse '@[bindhost]:[bindport]' in '[serveraddr[:serverport]][@[bindaddr]:[bindport]]'

[Filip Roséen]

Hi Jean-Paul,

On 2016-10-27 15:42, Jean-Paul Saman wrote:

> ---
>  include/vlc_url.h |  2 ++
>  src/text/url.c    | 80 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 69 insertions(+), 13 deletions(-)
> 
> diff --git a/include/vlc_url.h b/include/vlc_url.h
> index 5a20c27..802227e 100644
> --- a/include/vlc_url.h
> +++ b/include/vlc_url.h
> @@ -149,6 +149,8 @@ struct vlc_url_t
>      char *psz_password;
>      char *psz_host;
>      unsigned i_port;
> +    char *psz_bind_host;
> +    unsigned i_bind_port;
>      char *psz_path;
>      char *psz_option;
>  
> diff --git a/src/text/url.c b/src/text/url.c
> index 6116893..566d954 100644
> --- a/src/text/url.c
> +++ b/src/text/url.c
> @@ -407,6 +407,8 @@ int vlc_UrlParse(vlc_url_t *restrict url, const char *str)
>      url->psz_password = NULL;
>      url->psz_host = NULL;
>      url->i_port = 0;
> +    url->psz_bind_host = NULL;
> +    url->i_bind_port = 0;
>      url->psz_path = NULL;
>      url->psz_option = NULL;
>      url->psz_buffer = NULL;
> @@ -473,27 +475,78 @@ int vlc_UrlParse(vlc_url_t *restrict url, const char *str)
>          /*else
>              url->psz_path = "/";*/
>  
> -        /* User name */
> -        next = strrchr(cur, '@');
> -        if (next != NULL)
> +        if (url->psz_protocol && (strcmp(url->psz_protocol, "udp") == 0))
>          {
> -            *(next++) = '\0';
> -            url->psz_username = cur;
> -            cur = next;
> -
> -            /* Password (obsolete) */
> -            next = strchr(url->psz_username, ':');
> +            /*
> +             * Parse udp syntax:
> +             * [serveraddr[:serverport]][@[bindaddr]:[bindport]]
> +             */
> +            next = strchr(cur, '@');
> +            if (next != NULL)
> +            {
> +                *(next++) = '\0';
> +                char *psz_bindhost = strdup(next);
> +                if (*psz_bindhost == '[' && (next = strchr(psz_bindhost, ']')) != NULL)
> +                {
> +                    /* Try IPv6 numeral within brackets */
> +                    *(next++) = '\0';
> +                    url->psz_bind_host = strdup(psz_bindhost + 1);
> +
> +                    if (*next == ':')
> +                        next++;
> +                    else
> +                        next = NULL;
> +                }
> +                else
> +                {
> +                    next = strchr(psz_bindhost, ':');
> +                    if (next != NULL)
> +                        *(next++) = '\0';
> +
> +                    url->psz_bind_host = vlc_idna_to_ascii(vlc_uri_decode(psz_bindhost));
> +                }
> +
> +                if (url->psz_bind_host == NULL)
> +                    ret = -1;
> +                else
> +                    if (!vlc_uri_host_validate(url->psz_bind_host))
> +                    {
> +                        free(url->psz_bind_host);
> +                        url->psz_bind_host = NULL;
> +                        errno = EINVAL;
> +                        ret = -1;
> +                    }
> +                /* Port number */
> +                if (next != NULL)
> +                    url->i_bind_port = atoi(next);

The above needs to validate that what is passed to `atoi` really is a
number (and that it properly exhaust the payload), because currently
URIs as the below will be treated as valid:

 - `udp://localhost:420@localhost:123!this:is:weird/`
 - `udp://localhost:42@localhost:`

I just noticed that this problem is actually also present in the
current implementation (`http://example.com:80:hello-world/resource`
opens fine), so a fix-up needs to happen in both cases.

Given that you are adding additional logic to the function, it might
make sense to refactor shared functionality (and at the same time fix
the mentioned problem), instead of introducing an equivalent branch.

> +
> +                free(psz_bindhost);
> +            }
> +        }
> +        else
> +        {
> +            /* User name */
> +            next = strrchr(cur, '@');
>              if (next != NULL)
>              {
>                  *(next++) = '\0';
> -                url->psz_password = next;
> -                vlc_uri_decode(url->psz_password);
> +                url->psz_username = cur;
> +                cur = next;
> +
> +                /* Password (obsolete) */
> +                next = strchr(url->psz_username, ':');
> +                if (next != NULL)
> +                {
> +                    *(next++) = '\0';
> +                    url->psz_password = next;
> +                    vlc_uri_decode(url->psz_password);
> +                }
> +                vlc_uri_decode(url->psz_username);
>              }
> -            vlc_uri_decode(url->psz_username);
>          }
>  
>          /* Host name */
> -        if (*cur == '[' && (next = strrchr(cur, ']')) != NULL)
> +        if (*cur == '[' && (next = strchr(cur, ']')) != NULL)

As noted earlier, it would make more sense to refactor the
functionality which would, after this patch, be duplicated (such as
the ipv6-bracket-check).

>          {   /* Try IPv6 numeral within brackets */
>              *(next++) = '\0';
>              url->psz_host = strdup(cur + 1);
> @@ -547,6 +600,7 @@ int vlc_UrlParse(vlc_url_t *restrict url, const char *str)
>  
>  void vlc_UrlClean (vlc_url_t *restrict url)
>  {
> +    free (url->psz_bind_host);
>      free (url->psz_host);
>      free (url->psz_buffer);
>  }
> -- 
> 2.7.4

Best Regards,\
Filip

> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161027/e4649ff6/attachment.html>