diff mbox series

[FFmpeg-devel,v3,1/2] avformat/url: fix logic for removing ".." path components

Message ID 20200728041058.14557-1-josef@pex.com
State New
Headers show
Series [FFmpeg-devel,v3,1/2] avformat/url: fix logic for removing ".." path components
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Zlomek, Josef July 28, 2020, 4:10 a.m. UTC
Fixes: 8814

The logic for removing ".." path components and their corresponding
upper directories was reworked.

Now, the function trim_double_dot_url splits the path by "/" into
components, and processes the components one ny one:
- if the component is "..", the last path component in output buffer is removed
- if the component is not empty, it is added to the output buffer
No temporary buffers are used anymore.

Also the path containing no '/' after '://' is returned as it is.

The duplicate logic was removed from ff_make_absolute_url.

Signed-off-by: Josef Zlomek <josef@pex.com>
---
 libavformat/url.c | 101 +++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 51 deletions(-)

Comments

Zane van Iperen July 28, 2020, 10:02 a.m. UTC | #1
On Tue, 28 Jul 2020 06:10:57 +0200
"Josef Zlomek" <josef@pex.com> wrote:

> 
> Now, the function trim_double_dot_url splits the path by "/" into
> components, and processes the components one ny one:

Typo: ny should be by, I'm assuming?
Zlomek, Josef July 28, 2020, 10:25 a.m. UTC | #2
You are right, it is a typo.

On Tue, Jul 28, 2020 at 12:02 PM Zane van Iperen <zane@zanevaniperen.com>
wrote:

> On Tue, 28 Jul 2020 06:10:57 +0200
> "Josef Zlomek" <josef@pex.com> wrote:
>
> >
> > Now, the function trim_double_dot_url splits the path by "/" into
> > components, and processes the components one ny one:
>
> Typo: ny should be by, I'm assuming?
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Steven Liu July 28, 2020, 10:37 a.m. UTC | #3
Josef Zlomek <josef@pex.com> 于2020年7月28日周二 下午12:11写道:
>
> Fixes: 8814
>
> The logic for removing ".." path components and their corresponding
> upper directories was reworked.
>
> Now, the function trim_double_dot_url splits the path by "/" into
> components, and processes the components one ny one:
> - if the component is "..", the last path component in output buffer is removed
> - if the component is not empty, it is added to the output buffer
> No temporary buffers are used anymore.
>
> Also the path containing no '/' after '://' is returned as it is.
>
> The duplicate logic was removed from ff_make_absolute_url.
>
> Signed-off-by: Josef Zlomek <josef@pex.com>
> ---
>  libavformat/url.c | 101 +++++++++++++++++++++++-----------------------
>  1 file changed, 50 insertions(+), 51 deletions(-)
>
> diff --git a/libavformat/url.c b/libavformat/url.c
> index 20463a6674..46a08e88e3 100644
> --- a/libavformat/url.c
> +++ b/libavformat/url.c
> @@ -82,41 +82,62 @@ static void trim_double_dot_url(char *buf, const char *rel, int size)
>  {
>      const char *p = rel;
>      const char *root = rel;
> -    char tmp_path[MAX_URL_SIZE] = {0, };
> -    char *sep;
> -    char *node;
> +    char *dst;
> +    int dst_len = 0;
> +    const char *sep;
> +    const char *next;
> +    int last_is_dir;
>
>      /* Get the path root of the url which start by "://" */
>      if (p && (sep = strstr(p, "://"))) {
>          sep += 3;
>          root = strchr(sep, '/');
> -        if (!root)
> +        if (!root) {
> +            av_strlcpy(buf, rel, size);
>              return;
> +        }
>      }
> +    if (*root == '/')
> +        ++root;
>
> -    /* set new current position if the root node is changed */
> -    p = root;
> -    while (p && (node = strstr(p, ".."))) {
> -        av_strlcat(tmp_path, p, node - p + strlen(tmp_path));
> -        p = node + 3;
> -        sep = strrchr(tmp_path, '/');
> -        if (sep)
> -            sep[0] = '\0';
> -        else
> -            tmp_path[0] = '\0';
> -    }
> -
> -    if (!av_stristart(p, "/", NULL) && root != rel)
> -        av_strlcat(tmp_path, "/", size);
> -
> -    av_strlcat(tmp_path, p, size);
> -    /* start set buf after temp path process. */
> +    /* Copy the root to the output buffer */
>      av_strlcpy(buf, rel, root - rel + 1);
> +    size -= root - rel;
> +    dst = &buf[root - rel];
> +
> +    /* Split the path by "/" and remove ".." and its corresponding directory. */
> +    last_is_dir = 1;
> +    for (p = root; *p; p = next) {
> +        next = strchr(p, '/');
> +        if (!next) {
> +            next = p + strlen(p);
> +            last_is_dir = 0;
> +        }
>
> -    if (!av_stristart(tmp_path, "/", NULL) && root != rel)
> -        av_strlcat(buf, "/", size);
> +        if (next - p == 2 && !strncmp(p, "..", 2)) {
> +            /* remove the last directory from dst */
> +            while (dst_len > 0 && dst[--dst_len] != '/')
> +                ;
> +            dst[dst_len] = '\0';
> +            last_is_dir = 1;
> +        } else if (next > p) {
> +            int len_including_zero;
> +            /* copy the current path component to dst (including '/') */
> +            if (dst_len) {
> +                av_strlcpy(dst + dst_len, "/", size - dst_len);
> +                ++dst_len;
> +            }
> +            len_including_zero = FFMIN(size - dst_len, next - p + 1);
> +            av_strlcpy(dst + dst_len, p, len_including_zero);
> +            dst_len += len_including_zero - 1;
> +        }
>
> -    av_strlcat(buf, tmp_path, size);
> +        /* skip "/" */
> +        while (*next == '/')
> +            ++next;
> +    }
> +    if (last_is_dir && dst_len)
> +        av_strlcpy(dst + dst_len, "/", size - dst_len);
>  }
>
>  void ff_make_absolute_url(char *buf, int size, const char *base,
> @@ -175,14 +196,11 @@ void ff_make_absolute_url(char *buf, int size, const char *base,
>
>      root = p = buf;
>      /* Get the path root of the url which start by "://" */
> -    if (p && strstr(p, "://")) {
> -        sep = strstr(p, "://");
> -        if (sep) {
> -            sep += 3;
> -            root = strchr(sep, '/');
> -            if (!root)
> -                return;
> -        }
> +    if (p && (sep = strstr(p, "://"))) {
> +        sep += 3;
> +        root = strchr(sep, '/');
> +        if (!root)
> +            return;
>      }
>
>      /* Remove the file name from the base url */
> @@ -194,26 +212,7 @@ void ff_make_absolute_url(char *buf, int size, const char *base,
>          sep[1] = '\0';
>      else
>          buf[0] = '\0';
> -    while (av_strstart(rel, "..", NULL) && sep) {
> -        /* Remove the path delimiter at the end */
> -        if (sep > root) {
> -            sep[0] = '\0';
> -            sep = strrchr(buf, '/');
> -        }
>
> -        /* If the next directory name to pop off is "..", break here */
> -        if (!strcmp(sep ? &sep[1] : buf, "..")) {
> -            /* Readd the slash we just removed */
> -            av_strlcat(buf, "/", size);
> -            break;
> -        }
> -        /* Cut off the directory name */
> -        if (sep)
> -            sep[1] = '\0';
> -        else
> -            buf[0] = '\0';
> -        rel += 3;
> -    }
>      av_strlcat(buf, rel, size);
>      trim_double_dot_url(tmp_path, buf, size);
As Marton said, tmp_path should remove too.

>      memset(buf, 0, size);
> --
> 2.17.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Zlomek, Josef July 28, 2020, 10:57 a.m. UTC | #4
see version 3

On Tue, Jul 28, 2020 at 12:37 PM Steven Liu <lingjiujianke@gmail.com> wrote:

> Josef Zlomek <josef@pex.com> 于2020年7月28日周二 下午12:11写道:
> >
> > Fixes: 8814
> >
> > The logic for removing ".." path components and their corresponding
> > upper directories was reworked.
> >
> > Now, the function trim_double_dot_url splits the path by "/" into
> > components, and processes the components one ny one:
> > - if the component is "..", the last path component in output buffer is
> removed
> > - if the component is not empty, it is added to the output buffer
> > No temporary buffers are used anymore.
> >
> > Also the path containing no '/' after '://' is returned as it is.
> >
> > The duplicate logic was removed from ff_make_absolute_url.
> >
> > Signed-off-by: Josef Zlomek <josef@pex.com>
> > ---
> >  libavformat/url.c | 101 +++++++++++++++++++++++-----------------------
> >  1 file changed, 50 insertions(+), 51 deletions(-)
> >
> > diff --git a/libavformat/url.c b/libavformat/url.c
> > index 20463a6674..46a08e88e3 100644
> > --- a/libavformat/url.c
> > +++ b/libavformat/url.c
> > @@ -82,41 +82,62 @@ static void trim_double_dot_url(char *buf, const
> char *rel, int size)
> >  {
> >      const char *p = rel;
> >      const char *root = rel;
> > -    char tmp_path[MAX_URL_SIZE] = {0, };
> > -    char *sep;
> > -    char *node;
> > +    char *dst;
> > +    int dst_len = 0;
> > +    const char *sep;
> > +    const char *next;
> > +    int last_is_dir;
> >
> >      /* Get the path root of the url which start by "://" */
> >      if (p && (sep = strstr(p, "://"))) {
> >          sep += 3;
> >          root = strchr(sep, '/');
> > -        if (!root)
> > +        if (!root) {
> > +            av_strlcpy(buf, rel, size);
> >              return;
> > +        }
> >      }
> > +    if (*root == '/')
> > +        ++root;
> >
> > -    /* set new current position if the root node is changed */
> > -    p = root;
> > -    while (p && (node = strstr(p, ".."))) {
> > -        av_strlcat(tmp_path, p, node - p + strlen(tmp_path));
> > -        p = node + 3;
> > -        sep = strrchr(tmp_path, '/');
> > -        if (sep)
> > -            sep[0] = '\0';
> > -        else
> > -            tmp_path[0] = '\0';
> > -    }
> > -
> > -    if (!av_stristart(p, "/", NULL) && root != rel)
> > -        av_strlcat(tmp_path, "/", size);
> > -
> > -    av_strlcat(tmp_path, p, size);
> > -    /* start set buf after temp path process. */
> > +    /* Copy the root to the output buffer */
> >      av_strlcpy(buf, rel, root - rel + 1);
> > +    size -= root - rel;
> > +    dst = &buf[root - rel];
> > +
> > +    /* Split the path by "/" and remove ".." and its corresponding
> directory. */
> > +    last_is_dir = 1;
> > +    for (p = root; *p; p = next) {
> > +        next = strchr(p, '/');
> > +        if (!next) {
> > +            next = p + strlen(p);
> > +            last_is_dir = 0;
> > +        }
> >
> > -    if (!av_stristart(tmp_path, "/", NULL) && root != rel)
> > -        av_strlcat(buf, "/", size);
> > +        if (next - p == 2 && !strncmp(p, "..", 2)) {
> > +            /* remove the last directory from dst */
> > +            while (dst_len > 0 && dst[--dst_len] != '/')
> > +                ;
> > +            dst[dst_len] = '\0';
> > +            last_is_dir = 1;
> > +        } else if (next > p) {
> > +            int len_including_zero;
> > +            /* copy the current path component to dst (including '/') */
> > +            if (dst_len) {
> > +                av_strlcpy(dst + dst_len, "/", size - dst_len);
> > +                ++dst_len;
> > +            }
> > +            len_including_zero = FFMIN(size - dst_len, next - p + 1);
> > +            av_strlcpy(dst + dst_len, p, len_including_zero);
> > +            dst_len += len_including_zero - 1;
> > +        }
> >
> > -    av_strlcat(buf, tmp_path, size);
> > +        /* skip "/" */
> > +        while (*next == '/')
> > +            ++next;
> > +    }
> > +    if (last_is_dir && dst_len)
> > +        av_strlcpy(dst + dst_len, "/", size - dst_len);
> >  }
> >
> >  void ff_make_absolute_url(char *buf, int size, const char *base,
> > @@ -175,14 +196,11 @@ void ff_make_absolute_url(char *buf, int size,
> const char *base,
> >
> >      root = p = buf;
> >      /* Get the path root of the url which start by "://" */
> > -    if (p && strstr(p, "://")) {
> > -        sep = strstr(p, "://");
> > -        if (sep) {
> > -            sep += 3;
> > -            root = strchr(sep, '/');
> > -            if (!root)
> > -                return;
> > -        }
> > +    if (p && (sep = strstr(p, "://"))) {
> > +        sep += 3;
> > +        root = strchr(sep, '/');
> > +        if (!root)
> > +            return;
> >      }
> >
> >      /* Remove the file name from the base url */
> > @@ -194,26 +212,7 @@ void ff_make_absolute_url(char *buf, int size,
> const char *base,
> >          sep[1] = '\0';
> >      else
> >          buf[0] = '\0';
> > -    while (av_strstart(rel, "..", NULL) && sep) {
> > -        /* Remove the path delimiter at the end */
> > -        if (sep > root) {
> > -            sep[0] = '\0';
> > -            sep = strrchr(buf, '/');
> > -        }
> >
> > -        /* If the next directory name to pop off is "..", break here */
> > -        if (!strcmp(sep ? &sep[1] : buf, "..")) {
> > -            /* Readd the slash we just removed */
> > -            av_strlcat(buf, "/", size);
> > -            break;
> > -        }
> > -        /* Cut off the directory name */
> > -        if (sep)
> > -            sep[1] = '\0';
> > -        else
> > -            buf[0] = '\0';
> > -        rel += 3;
> > -    }
> >      av_strlcat(buf, rel, size);
> >      trim_double_dot_url(tmp_path, buf, size);
> As Marton said, tmp_path should remove too.
>
> >      memset(buf, 0, size);
> > --
> > 2.17.1
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Steven Liu July 28, 2020, 10:59 a.m. UTC | #5
Zlomek, Josef <josef@pex.com> 于2020年7月28日周二 下午6:58写道:
>
> see version 3
Isn't this version? or maybe your mean is version 4?
>
> On Tue, Jul 28, 2020 at 12:37 PM Steven Liu <lingjiujianke@gmail.com> wrote:
>
> > Josef Zlomek <josef@pex.com> 于2020年7月28日周二 下午12:11写道:
> > >
> > > Fixes: 8814
> > >
> > > The logic for removing ".." path components and their corresponding
> > > upper directories was reworked.
> > >
> > > Now, the function trim_double_dot_url splits the path by "/" into
> > > components, and processes the components one ny one:
> > > - if the component is "..", the last path component in output buffer is
> > removed
> > > - if the component is not empty, it is added to the output buffer
> > > No temporary buffers are used anymore.
> > >
> > > Also the path containing no '/' after '://' is returned as it is.
> > >
> > > The duplicate logic was removed from ff_make_absolute_url.
> > >
> > > Signed-off-by: Josef Zlomek <josef@pex.com>
> > > ---
> > >  libavformat/url.c | 101 +++++++++++++++++++++++-----------------------
> > >  1 file changed, 50 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/libavformat/url.c b/libavformat/url.c
> > > index 20463a6674..46a08e88e3 100644
> > > --- a/libavformat/url.c
> > > +++ b/libavformat/url.c
> > > @@ -82,41 +82,62 @@ static void trim_double_dot_url(char *buf, const
> > char *rel, int size)
> > >  {
> > >      const char *p = rel;
> > >      const char *root = rel;
> > > -    char tmp_path[MAX_URL_SIZE] = {0, };
> > > -    char *sep;
> > > -    char *node;
> > > +    char *dst;
> > > +    int dst_len = 0;
> > > +    const char *sep;
> > > +    const char *next;
> > > +    int last_is_dir;
> > >
> > >      /* Get the path root of the url which start by "://" */
> > >      if (p && (sep = strstr(p, "://"))) {
> > >          sep += 3;
> > >          root = strchr(sep, '/');
> > > -        if (!root)
> > > +        if (!root) {
> > > +            av_strlcpy(buf, rel, size);
> > >              return;
> > > +        }
> > >      }
> > > +    if (*root == '/')
> > > +        ++root;
> > >
> > > -    /* set new current position if the root node is changed */
> > > -    p = root;
> > > -    while (p && (node = strstr(p, ".."))) {
> > > -        av_strlcat(tmp_path, p, node - p + strlen(tmp_path));
> > > -        p = node + 3;
> > > -        sep = strrchr(tmp_path, '/');
> > > -        if (sep)
> > > -            sep[0] = '\0';
> > > -        else
> > > -            tmp_path[0] = '\0';
> > > -    }
> > > -
> > > -    if (!av_stristart(p, "/", NULL) && root != rel)
> > > -        av_strlcat(tmp_path, "/", size);
> > > -
> > > -    av_strlcat(tmp_path, p, size);
> > > -    /* start set buf after temp path process. */
> > > +    /* Copy the root to the output buffer */
> > >      av_strlcpy(buf, rel, root - rel + 1);
> > > +    size -= root - rel;
> > > +    dst = &buf[root - rel];
> > > +
> > > +    /* Split the path by "/" and remove ".." and its corresponding
> > directory. */
> > > +    last_is_dir = 1;
> > > +    for (p = root; *p; p = next) {
> > > +        next = strchr(p, '/');
> > > +        if (!next) {
> > > +            next = p + strlen(p);
> > > +            last_is_dir = 0;
> > > +        }
> > >
> > > -    if (!av_stristart(tmp_path, "/", NULL) && root != rel)
> > > -        av_strlcat(buf, "/", size);
> > > +        if (next - p == 2 && !strncmp(p, "..", 2)) {
> > > +            /* remove the last directory from dst */
> > > +            while (dst_len > 0 && dst[--dst_len] != '/')
> > > +                ;
> > > +            dst[dst_len] = '\0';
> > > +            last_is_dir = 1;
> > > +        } else if (next > p) {
> > > +            int len_including_zero;
> > > +            /* copy the current path component to dst (including '/') */
> > > +            if (dst_len) {
> > > +                av_strlcpy(dst + dst_len, "/", size - dst_len);
> > > +                ++dst_len;
> > > +            }
> > > +            len_including_zero = FFMIN(size - dst_len, next - p + 1);
> > > +            av_strlcpy(dst + dst_len, p, len_including_zero);
> > > +            dst_len += len_including_zero - 1;
> > > +        }
> > >
> > > -    av_strlcat(buf, tmp_path, size);
> > > +        /* skip "/" */
> > > +        while (*next == '/')
> > > +            ++next;
> > > +    }
> > > +    if (last_is_dir && dst_len)
> > > +        av_strlcpy(dst + dst_len, "/", size - dst_len);
> > >  }
> > >
> > >  void ff_make_absolute_url(char *buf, int size, const char *base,
> > > @@ -175,14 +196,11 @@ void ff_make_absolute_url(char *buf, int size,
> > const char *base,
> > >
> > >      root = p = buf;
> > >      /* Get the path root of the url which start by "://" */
> > > -    if (p && strstr(p, "://")) {
> > > -        sep = strstr(p, "://");
> > > -        if (sep) {
> > > -            sep += 3;
> > > -            root = strchr(sep, '/');
> > > -            if (!root)
> > > -                return;
> > > -        }
> > > +    if (p && (sep = strstr(p, "://"))) {
> > > +        sep += 3;
> > > +        root = strchr(sep, '/');
> > > +        if (!root)
> > > +            return;
> > >      }
> > >
> > >      /* Remove the file name from the base url */
> > > @@ -194,26 +212,7 @@ void ff_make_absolute_url(char *buf, int size,
> > const char *base,
> > >          sep[1] = '\0';
> > >      else
> > >          buf[0] = '\0';
> > > -    while (av_strstart(rel, "..", NULL) && sep) {
> > > -        /* Remove the path delimiter at the end */
> > > -        if (sep > root) {
> > > -            sep[0] = '\0';
> > > -            sep = strrchr(buf, '/');
> > > -        }
> > >
> > > -        /* If the next directory name to pop off is "..", break here */
> > > -        if (!strcmp(sep ? &sep[1] : buf, "..")) {
> > > -            /* Readd the slash we just removed */
> > > -            av_strlcat(buf, "/", size);
> > > -            break;
> > > -        }
> > > -        /* Cut off the directory name */
> > > -        if (sep)
> > > -            sep[1] = '\0';
> > > -        else
> > > -            buf[0] = '\0';
> > > -        rel += 3;
> > > -    }
> > >      av_strlcat(buf, rel, size);
> > >      trim_double_dot_url(tmp_path, buf, size);
> > As Marton said, tmp_path should remove too.
> >
> > >      memset(buf, 0, size);
> > > --
> > > 2.17.1
> > >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
>
>
> --
> Josef Zlomek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Zlomek, Josef July 28, 2020, 11:04 a.m. UTC | #6
I have removed tmp_path from function  trim_double_dot_url
I did not change the function ff_make_absolute_url much, as I wanted to
just fix handling of "..".

Josef

On Tue, Jul 28, 2020 at 12:59 PM Steven Liu <lingjiujianke@gmail.com> wrote:

> Zlomek, Josef <josef@pex.com> 于2020年7月28日周二 下午6:58写道:
> >
> > see version 3
> Isn't this version? or maybe your mean is version 4?
> >
> > On Tue, Jul 28, 2020 at 12:37 PM Steven Liu <lingjiujianke@gmail.com>
> wrote:
> >
> > > Josef Zlomek <josef@pex.com> 于2020年7月28日周二 下午12:11写道:
> > > >
> > > > Fixes: 8814
> > > >
> > > > The logic for removing ".." path components and their corresponding
> > > > upper directories was reworked.
> > > >
> > > > Now, the function trim_double_dot_url splits the path by "/" into
> > > > components, and processes the components one ny one:
> > > > - if the component is "..", the last path component in output buffer
> is
> > > removed
> > > > - if the component is not empty, it is added to the output buffer
> > > > No temporary buffers are used anymore.
> > > >
> > > > Also the path containing no '/' after '://' is returned as it is.
> > > >
> > > > The duplicate logic was removed from ff_make_absolute_url.
> > > >
> > > > Signed-off-by: Josef Zlomek <josef@pex.com>
> > > > ---
> > > >  libavformat/url.c | 101
> +++++++++++++++++++++++-----------------------
> > > >  1 file changed, 50 insertions(+), 51 deletions(-)
> > > >
> > > > diff --git a/libavformat/url.c b/libavformat/url.c
> > > > index 20463a6674..46a08e88e3 100644
> > > > --- a/libavformat/url.c
> > > > +++ b/libavformat/url.c
> > > > @@ -82,41 +82,62 @@ static void trim_double_dot_url(char *buf, const
> > > char *rel, int size)
> > > >  {
> > > >      const char *p = rel;
> > > >      const char *root = rel;
> > > > -    char tmp_path[MAX_URL_SIZE] = {0, };
> > > > -    char *sep;
> > > > -    char *node;
> > > > +    char *dst;
> > > > +    int dst_len = 0;
> > > > +    const char *sep;
> > > > +    const char *next;
> > > > +    int last_is_dir;
> > > >
> > > >      /* Get the path root of the url which start by "://" */
> > > >      if (p && (sep = strstr(p, "://"))) {
> > > >          sep += 3;
> > > >          root = strchr(sep, '/');
> > > > -        if (!root)
> > > > +        if (!root) {
> > > > +            av_strlcpy(buf, rel, size);
> > > >              return;
> > > > +        }
> > > >      }
> > > > +    if (*root == '/')
> > > > +        ++root;
> > > >
> > > > -    /* set new current position if the root node is changed */
> > > > -    p = root;
> > > > -    while (p && (node = strstr(p, ".."))) {
> > > > -        av_strlcat(tmp_path, p, node - p + strlen(tmp_path));
> > > > -        p = node + 3;
> > > > -        sep = strrchr(tmp_path, '/');
> > > > -        if (sep)
> > > > -            sep[0] = '\0';
> > > > -        else
> > > > -            tmp_path[0] = '\0';
> > > > -    }
> > > > -
> > > > -    if (!av_stristart(p, "/", NULL) && root != rel)
> > > > -        av_strlcat(tmp_path, "/", size);
> > > > -
> > > > -    av_strlcat(tmp_path, p, size);
> > > > -    /* start set buf after temp path process. */
> > > > +    /* Copy the root to the output buffer */
> > > >      av_strlcpy(buf, rel, root - rel + 1);
> > > > +    size -= root - rel;
> > > > +    dst = &buf[root - rel];
> > > > +
> > > > +    /* Split the path by "/" and remove ".." and its corresponding
> > > directory. */
> > > > +    last_is_dir = 1;
> > > > +    for (p = root; *p; p = next) {
> > > > +        next = strchr(p, '/');
> > > > +        if (!next) {
> > > > +            next = p + strlen(p);
> > > > +            last_is_dir = 0;
> > > > +        }
> > > >
> > > > -    if (!av_stristart(tmp_path, "/", NULL) && root != rel)
> > > > -        av_strlcat(buf, "/", size);
> > > > +        if (next - p == 2 && !strncmp(p, "..", 2)) {
> > > > +            /* remove the last directory from dst */
> > > > +            while (dst_len > 0 && dst[--dst_len] != '/')
> > > > +                ;
> > > > +            dst[dst_len] = '\0';
> > > > +            last_is_dir = 1;
> > > > +        } else if (next > p) {
> > > > +            int len_including_zero;
> > > > +            /* copy the current path component to dst (including
> '/') */
> > > > +            if (dst_len) {
> > > > +                av_strlcpy(dst + dst_len, "/", size - dst_len);
> > > > +                ++dst_len;
> > > > +            }
> > > > +            len_including_zero = FFMIN(size - dst_len, next - p +
> 1);
> > > > +            av_strlcpy(dst + dst_len, p, len_including_zero);
> > > > +            dst_len += len_including_zero - 1;
> > > > +        }
> > > >
> > > > -    av_strlcat(buf, tmp_path, size);
> > > > +        /* skip "/" */
> > > > +        while (*next == '/')
> > > > +            ++next;
> > > > +    }
> > > > +    if (last_is_dir && dst_len)
> > > > +        av_strlcpy(dst + dst_len, "/", size - dst_len);
> > > >  }
> > > >
> > > >  void ff_make_absolute_url(char *buf, int size, const char *base,
> > > > @@ -175,14 +196,11 @@ void ff_make_absolute_url(char *buf, int size,
> > > const char *base,
> > > >
> > > >      root = p = buf;
> > > >      /* Get the path root of the url which start by "://" */
> > > > -    if (p && strstr(p, "://")) {
> > > > -        sep = strstr(p, "://");
> > > > -        if (sep) {
> > > > -            sep += 3;
> > > > -            root = strchr(sep, '/');
> > > > -            if (!root)
> > > > -                return;
> > > > -        }
> > > > +    if (p && (sep = strstr(p, "://"))) {
> > > > +        sep += 3;
> > > > +        root = strchr(sep, '/');
> > > > +        if (!root)
> > > > +            return;
> > > >      }
> > > >
> > > >      /* Remove the file name from the base url */
> > > > @@ -194,26 +212,7 @@ void ff_make_absolute_url(char *buf, int size,
> > > const char *base,
> > > >          sep[1] = '\0';
> > > >      else
> > > >          buf[0] = '\0';
> > > > -    while (av_strstart(rel, "..", NULL) && sep) {
> > > > -        /* Remove the path delimiter at the end */
> > > > -        if (sep > root) {
> > > > -            sep[0] = '\0';
> > > > -            sep = strrchr(buf, '/');
> > > > -        }
> > > >
> > > > -        /* If the next directory name to pop off is "..", break
> here */
> > > > -        if (!strcmp(sep ? &sep[1] : buf, "..")) {
> > > > -            /* Readd the slash we just removed */
> > > > -            av_strlcat(buf, "/", size);
> > > > -            break;
> > > > -        }
> > > > -        /* Cut off the directory name */
> > > > -        if (sep)
> > > > -            sep[1] = '\0';
> > > > -        else
> > > > -            buf[0] = '\0';
> > > > -        rel += 3;
> > > > -    }
> > > >      av_strlcat(buf, rel, size);
> > > >      trim_double_dot_url(tmp_path, buf, size);
> > > As Marton said, tmp_path should remove too.
> > >
> > > >      memset(buf, 0, size);
> > > > --
> > > > 2.17.1
> > > >
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel@ffmpeg.org
> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >
> > > > To unsubscribe, visit link above, or email
> > > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >
> >
> >
> > --
> > Josef Zlomek
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George July 29, 2020, 10:01 a.m. UTC | #7
Josef Zlomek (12020-07-28):
> Fixes: 8814
> 
> The logic for removing ".." path components and their corresponding
> upper directories was reworked.
> 
> Now, the function trim_double_dot_url splits the path by "/" into
> components, and processes the components one ny one:
> - if the component is "..", the last path component in output buffer is removed
> - if the component is not empty, it is added to the output buffer
> No temporary buffers are used anymore.
> 
> Also the path containing no '/' after '://' is returned as it is.
> 
> The duplicate logic was removed from ff_make_absolute_url.

I am afraid ff_make_absolute_url() as it is is broken beyond repair. For
example:

http://server/menu redirect?url=http://otherserver/target =>
  redirect?url=http://otherserver/target

while the result should have been

http://server/redirect?url=http://otherserver/target

I will have a shot at rewriting it cleanly, with proper parsing of URL
components.

Regards,
Zlomek, Josef July 29, 2020, 10:54 a.m. UTC | #8
On Wed, Jul 29, 2020 at 12:01 PM Nicolas George <george@nsup.org> wrote:

> I am afraid ff_make_absolute_url() as it is is broken beyond repair. For
> example:
>
> http://server/menu redirect?url=http://otherserver/target =>
>   redirect?url=http://otherserver/target
>
> while the result should have been
>
> http://server/redirect?url=http://otherserver/target
>
> I will have a shot at rewriting it cleanly, with proper parsing of URL
> components.
>

I also noticed that there are many more bugs in ff_make_absolute_url()
I just fixed one of them.
I am looking forward for your complete fix.

Best regards,
Josef
Nicolas George July 29, 2020, 12:41 p.m. UTC | #9
Zlomek, Josef (12020-07-29):
> I also noticed that there are many more bugs in ff_make_absolute_url()
> I just fixed one of them.
> I am looking forward for your complete fix.

I think the key to a working version is to properly parse URLs first.
Then we can parse the base and relative part, and build the absolute URL
from the components, simplifying the path component, and only the path
component, at the same time.

Here is my preliminary code for parsing, in case somebody wants to look
at it.

Next step is to use it for rewriting ff_make_absolute_url(). Possibly,
use it for av_url_split() too.

Regards,
Marton Balint July 29, 2020, 8:02 p.m. UTC | #10
On Wed, 29 Jul 2020, Nicolas George wrote:

> Zlomek, Josef (12020-07-29):
>> I also noticed that there are many more bugs in ff_make_absolute_url()
>> I just fixed one of them.
>> I am looking forward for your complete fix.
>
> I think the key to a working version is to properly parse URLs first.
> Then we can parse the base and relative part, and build the absolute URL
> from the components, simplifying the path component, and only the path
> component, at the same time.
>
> Here is my preliminary code for parsing, in case somebody wants to look
> at it.

Thanks for working on this. I agree that proper (as RFC compliant as it 
can be) URL parsing is needed here. Probably we should clearly document 
differences from RFC compliant parsing, if we cannot do it entirely RFC 
compliantly...

> diff --git a/libavformat/url.c b/libavformat/url.c
> index 20463a6674..7612a2ee6e 100644
> --- a/libavformat/url.c
> +++ b/libavformat/url.c
> @@ -78,6 +78,75 @@ 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 (!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);

? and # can also separate authority from the rest.

> +
> +        /* 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;
> +        } else {
> +            cur = find_delim(":", cur, aend);
> +        }
> +
> +        /* port */
> +        uc->port = cur;
> +        cur = aend;
> +    } else {
> +        uc->userinfo = uc->host = uc->port = cur;
> +    }
> +
> +    /* path */
> +    uc->path = cur;
> +    cur = find_delim("?#", cur, end);
> +
> +    /* query */
> +    uc->query = cur;
> +    if (*cur == '?')
> +        cur = find_delim("#", cur, end);
> +
> +    /* fragment */
> +    uc->fragment = cur;
> +
> +    uc->end = end;
> +    return 0;
> +}
> +
>  static void trim_double_dot_url(char *buf, const char *rel, int size)
>  {
>      const char *p = rel;
> diff --git a/libavformat/url.h b/libavformat/url.h
> index de0d30aca0..99d453b378 100644
> --- a/libavformat/url.h
> +++ b/libavformat/url.h
> @@ -344,4 +344,41 @@ const AVClass *ff_urlcontext_child_class_iterate(void **iter);
>  const URLProtocol **ffurl_get_protocols(const char *whitelist,
>                                          const char *blacklist);
> 
> +typedef struct URLComponents {
> +    const char *url;        /**< whole URL, for reference */
> +    const char *scheme;     /**< possibly including lavf-specific options */
> +    const char *authority;  /**< "//" if it is a real URL */
> +    const char *userinfo;   /**< including final '@' if present */
> +    const char *host;
> +    const char *port;       /**< including initial ':' if present */
> +    const char *path;
> +    const char *query;      /**< including initial '?' if present */
> +    const char *fragment;   /**< including initial '#' if present */
> +    const char *end;
> +} URLComponents;
> +
> +#define url_component_end_scheme      authority
> +#define url_component_end_authority   userinfo
> +#define url_component_end_userinfo    host
> +#define url_component_end_host        port
> +#define url_component_end_port        path
> +#define url_component_end_path        query
> +#define url_component_end_query       fragment
> +#define url_component_end_fragment    end

I am not sure about this approach, the known characters at the end 
or at the start will make further operations a bit harder. I'd just 
simply add another field for each URL component to signal the end, e.g.

url
url_end
scheme
scheme_end
...

Regards,
Marton
Nicolas George July 29, 2020, 10:07 p.m. UTC | #11
Marton Balint (12020-07-29):
> Thanks for working on this. I agree that proper (as RFC compliant as it can
> be) URL parsing is needed here. Probably we should clearly document
> differences from RFC compliant parsing, if we cannot do it entirely RFC
> compliantly...

Indeed. Unfortunately, that may cause some trouble, but not for right
now.

> ? and # can also separate authority from the rest.

Thanks, fixed.

> I am not sure about this approach, the known characters at the end or at the
> start will make further operations a bit harder. I'd just simply add another
> field for each URL component to signal the end, e.g.

I considered this, but it was not actually convenient.

I have something that works, but it needs polishing and more testing.
But you can have a peek.

Regards,
diff mbox series

Patch

diff --git a/libavformat/url.c b/libavformat/url.c
index 20463a6674..46a08e88e3 100644
--- a/libavformat/url.c
+++ b/libavformat/url.c
@@ -82,41 +82,62 @@  static void trim_double_dot_url(char *buf, const char *rel, int size)
 {
     const char *p = rel;
     const char *root = rel;
-    char tmp_path[MAX_URL_SIZE] = {0, };
-    char *sep;
-    char *node;
+    char *dst;
+    int dst_len = 0;
+    const char *sep;
+    const char *next;
+    int last_is_dir;
 
     /* Get the path root of the url which start by "://" */
     if (p && (sep = strstr(p, "://"))) {
         sep += 3;
         root = strchr(sep, '/');
-        if (!root)
+        if (!root) {
+            av_strlcpy(buf, rel, size);
             return;
+        }
     }
+    if (*root == '/')
+        ++root;
 
-    /* set new current position if the root node is changed */
-    p = root;
-    while (p && (node = strstr(p, ".."))) {
-        av_strlcat(tmp_path, p, node - p + strlen(tmp_path));
-        p = node + 3;
-        sep = strrchr(tmp_path, '/');
-        if (sep)
-            sep[0] = '\0';
-        else
-            tmp_path[0] = '\0';
-    }
-
-    if (!av_stristart(p, "/", NULL) && root != rel)
-        av_strlcat(tmp_path, "/", size);
-
-    av_strlcat(tmp_path, p, size);
-    /* start set buf after temp path process. */
+    /* Copy the root to the output buffer */
     av_strlcpy(buf, rel, root - rel + 1);
+    size -= root - rel;
+    dst = &buf[root - rel];
+
+    /* Split the path by "/" and remove ".." and its corresponding directory. */
+    last_is_dir = 1;
+    for (p = root; *p; p = next) {
+        next = strchr(p, '/');
+        if (!next) {
+            next = p + strlen(p);
+            last_is_dir = 0;
+        }
 
-    if (!av_stristart(tmp_path, "/", NULL) && root != rel)
-        av_strlcat(buf, "/", size);
+        if (next - p == 2 && !strncmp(p, "..", 2)) {
+            /* remove the last directory from dst */
+            while (dst_len > 0 && dst[--dst_len] != '/')
+                ;
+            dst[dst_len] = '\0';
+            last_is_dir = 1;
+        } else if (next > p) {
+            int len_including_zero;
+            /* copy the current path component to dst (including '/') */
+            if (dst_len) {
+                av_strlcpy(dst + dst_len, "/", size - dst_len);
+                ++dst_len;
+            }
+            len_including_zero = FFMIN(size - dst_len, next - p + 1);
+            av_strlcpy(dst + dst_len, p, len_including_zero);
+            dst_len += len_including_zero - 1;
+        }
 
-    av_strlcat(buf, tmp_path, size);
+        /* skip "/" */
+        while (*next == '/')
+            ++next;
+    }
+    if (last_is_dir && dst_len)
+        av_strlcpy(dst + dst_len, "/", size - dst_len);
 }
 
 void ff_make_absolute_url(char *buf, int size, const char *base,
@@ -175,14 +196,11 @@  void ff_make_absolute_url(char *buf, int size, const char *base,
 
     root = p = buf;
     /* Get the path root of the url which start by "://" */
-    if (p && strstr(p, "://")) {
-        sep = strstr(p, "://");
-        if (sep) {
-            sep += 3;
-            root = strchr(sep, '/');
-            if (!root)
-                return;
-        }
+    if (p && (sep = strstr(p, "://"))) {
+        sep += 3;
+        root = strchr(sep, '/');
+        if (!root)
+            return;
     }
 
     /* Remove the file name from the base url */
@@ -194,26 +212,7 @@  void ff_make_absolute_url(char *buf, int size, const char *base,
         sep[1] = '\0';
     else
         buf[0] = '\0';
-    while (av_strstart(rel, "..", NULL) && sep) {
-        /* Remove the path delimiter at the end */
-        if (sep > root) {
-            sep[0] = '\0';
-            sep = strrchr(buf, '/');
-        }
 
-        /* If the next directory name to pop off is "..", break here */
-        if (!strcmp(sep ? &sep[1] : buf, "..")) {
-            /* Readd the slash we just removed */
-            av_strlcat(buf, "/", size);
-            break;
-        }
-        /* Cut off the directory name */
-        if (sep)
-            sep[1] = '\0';
-        else
-            buf[0] = '\0';
-        rel += 3;
-    }
     av_strlcat(buf, rel, size);
     trim_double_dot_url(tmp_path, buf, size);
     memset(buf, 0, size);