[FFmpeg-devel] [PATCH 1/2] lavf/url: add ff_url_decompose().

[Marton Balint]

On Thu, 30 Jul 2020, Nicolas George wrote:

> Signed-off-by: Nicolas George <george at nsup.org>
> ---
> libavformat/tests/url.c | 34 +++++++++++++++++++
> libavformat/url.c       | 74 +++++++++++++++++++++++++++++++++++++++++
> libavformat/url.h       | 41 +++++++++++++++++++++++
> tests/ref/fate/url      | 45 +++++++++++++++++++++++++
> 4 files changed, 194 insertions(+)
>
>
> I chose to keep only pointers to the beginnings despite Marton's
> suggestion because I find it plays better with re-constructing the URL
> afterwards. The property that each char of the string belongs to one and
> only one part is a good invariant, and the delimiting characters are
> clearly documented and allow to check if the field is present.

Ok.

>
> Compared with the previous iteration, I added a few macros and the
> handling of NULL.
>
>
> diff --git a/libavformat/tests/url.c b/libavformat/tests/url.c
> index 1d961a1b43..e7d259ab7d 100644
> --- a/libavformat/tests/url.c
> +++ b/libavformat/tests/url.c
> @@ -21,6 +21,31 @@
> #include "libavformat/url.h"
> #include "libavformat/avformat.h"
> 
> +static void test_decompose(const char *url)
> +{
> +    URLComponents uc;
> +    int len, ret;
> +
> +    printf("%s =>\n", url);
> +    ret = ff_url_decompose(&uc, url, NULL);
> +    if (ret < 0) {
> +        printf("  error: %s\n", av_err2str(ret));
> +        return;
> +    }
> +#define PRINT_COMPONENT(comp) \
> +    len = uc.url_component_end_##comp - uc.comp; \
> +    if (len) printf("  "#comp": %.*s\n", len, uc.comp);
> +    PRINT_COMPONENT(scheme);
> +    PRINT_COMPONENT(authority);
> +    PRINT_COMPONENT(userinfo);
> +    PRINT_COMPONENT(host);
> +    PRINT_COMPONENT(port);
> +    PRINT_COMPONENT(path);
> +    PRINT_COMPONENT(query);
> +    PRINT_COMPONENT(fragment);
> +    printf("\n");
> +}
> +
> static void test(const char *base, const char *rel)
> {
>     char buf[200], buf2[200];
> @@ -51,6 +76,15 @@ static void test2(const char *url)
> 
> int main(void)
> {
> +    printf("Testing ff_url_decompose:\n\n");
> +    test_decompose("http://user:pass@ffmpeg:8080/dir/file?query#fragment");
> +    test_decompose("http://ffmpeg/dir/file");
> +    test_decompose("file:///dev/null");
> +    test_decompose("file:/dev/null");
> +    test_decompose("http://[::1]/dev/null");
> +    test_decompose("http://[::1]:8080/dev/null");
> +    test_decompose("//ffmpeg/dev/null");
> +
>     printf("Testing ff_make_absolute_url:\n");
>     test(NULL, "baz");
>     test("/foo/bar", "baz");
> diff --git a/libavformat/url.c b/libavformat/url.c
> index 20463a6674..26aaab4019 100644
> --- a/libavformat/url.c
> +++ b/libavformat/url.c
> @@ -78,6 +78,80 @@ int ff_url_join(char *str, int size, const char *proto,
>     return strlen(str);
> }
> 
> +static const char *find_delim(const char *delim, const char *cur, const char *end)
> +{
> +    while (cur < end && !strchr(delim, *cur))
> +        cur++;
> +    return cur;
> +}
> +
> +int ff_url_decompose(URLComponents *uc, const char *url, const char *end)
> +{
> +    const char *cur, *aend, *p;
> +
> +    if (!url) {
> +        URLComponents nul = { 0 };
> +        *uc = nul;

So you are returning NULL pointers here and success at the same time. This 
does not look like a good idea, e.g. checking fields later on involves 
arithmetic on NULL pointers, no? I don't really see it useful that we 
handle NULL url here, we are better off with an assert IMHO.

> +        return 0;
> +    }
> +    if (!end)
> +        end = url + strlen(url);
> +    cur = uc->url = url;
> +
> +    /* scheme */
> +    uc->scheme = cur;
> +    p = find_delim(":/", cur, end); /* lavf "schemes" can contain options */
> +    if (*p == ':')
> +        cur = p + 1;
> +
> +    /* authority */
> +    uc->authority = cur;
> +    if (end - cur >= 2 && cur[0] == '/' && cur[1] == '/') {
> +        cur += 2;
> +        aend = find_delim("/?#", cur, end);
> +
> +        /* userinfo */
> +        uc->userinfo = cur;
> +        p = find_delim("@", cur, aend);
> +        if (*p == '@')
> +            cur = p + 1;
> +
> +        /* host */
> +        uc->host = cur;
> +        if (*cur == '[') { /* hello IPv6, thanks for using colons! */
> +            p = find_delim("]", cur, aend);
> +            if (*p != ']')
> +                return AVERROR(EINVAL);
> +            if (p + 1 < aend && p[1] != ':')
> +                return AVERROR(EINVAL);
> +            cur = p + 1;

This is the only place where we might return failure. Maybe we could 
convert this to void() function to simplify usage a bit, and either
- assume no port, if it is not paraseable or
- not split host and port, so we don't have to parse IPv6 mess here, 
therefore the error can't happen.

Thanks,
Marton