diff mbox series

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

Message ID 20200727105629.5510-1-josef@pex.com
State Superseded
Headers show
Series [FFmpeg-devel,1/3] 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 27, 2020, 10:56 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 tmp_path is removed
- if the component is not empty, it is added to tmp_path

The duplicate logic was removed from ff_make_absolute_url.

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

Comments

Steven Liu July 27, 2020, 11:35 a.m. UTC | #1
Josef Zlomek <josef@pex.com> 于2020年7月27日周一 下午6:57写道:
>
> 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 tmp_path is removed
> - if the component is not empty, it is added to tmp_path
>
> The duplicate logic was removed from ff_make_absolute_url.
>
> Signed-off-by: Josef Zlomek <josef@pex.com>
> ---
>  libavformat/url.c | 70 +++++++++++++++++++++--------------------------
>  1 file changed, 31 insertions(+), 39 deletions(-)
>
> diff --git a/libavformat/url.c b/libavformat/url.c
> index 20463a6674..ccaa28a1ed 100644
> --- a/libavformat/url.c
> +++ b/libavformat/url.c
> @@ -83,8 +83,9 @@ 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;
> +    int tmp_len = 0;
> +    const char *sep;
> +    const char *next;
>
>      /* Get the path root of the url which start by "://" */
>      if (p && (sep = strstr(p, "://"))) {
> @@ -93,29 +94,39 @@ static void trim_double_dot_url(char *buf, const char *rel, int size)
>          if (!root)
>              return;
>      }
> +    if (*root == '/')
> +        ++root;
> +
> +    /* Split the path by "/" and remove ".." and its corresponding directory. */
> +    for (p = root; *p; p = next) {
> +        next = strchr(p, '/');
> +        if (!next)
> +            next = p + strlen(p);
> +
> +        if (next - p == 2 && !strncmp(p, "..", 2)) {
> +            /* remove the last directory from tmp_path */
> +            while (tmp_len > 0 && tmp_path[--tmp_len] != '/')
> +                ;
> +            tmp_path[tmp_len] = '\0';
> +        } else if (next > p) {
> +            /* copy the current path component to tmp_path (including '/') */
> +            if (tmp_len) {
> +                av_strlcpy(tmp_path + tmp_len, "/", sizeof(tmp_path) - tmp_len);
> +                ++tmp_len;
> +            }
> +            av_strlcpy(tmp_path + tmp_len, p, FFMIN(sizeof(tmp_path) - tmp_len,
> +                                                    next - p + 1));
> +            tmp_len += next - p;
> +            tmp_path[tmp_len] = '\0';
> +        }
>
> -    /* 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';
> +        /* skip "/" */
> +        while (*next == '/')
> +            ++next;
>      }
>
> -    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. */
>      av_strlcpy(buf, rel, root - rel + 1);
> -
> -    if (!av_stristart(tmp_path, "/", NULL) && root != rel)
> -        av_strlcat(buf, "/", size);
> -
>      av_strlcat(buf, tmp_path, size);
>  }
>
> @@ -194,26 +205,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);
> --
> 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".

maybe the last node is a file, not directory

(base) liuqi05:clang liuqi$ make fate-url
CC libavformat/tests/url.o
LD libavformat/tests/url
TEST    url
--- src/tests/ref/fate/url 2020-07-27 19:30:58.000000000 +0800
+++ tests/data/fate/url 2020-07-27 19:34:25.000000000 +0800
@@ -15,9 +15,9 @@
                              http://server/foo/bar //other/url
  => http://other/url
                              http://server/foo/bar
../../../../../other/url => http://server/other/url
                              http://server/foo/bar
a/b/../c/d/../e../..f/.../../other/url/test..mp3 =>
http://server/foo/a/c/e../..f/other/url/test..mp3
-                             http://server/foo/bar
a/b/../c/d/../e../..f/.../../other/url/.. =>
http://server/foo/a/c/e../..f/other/url/..
-                             http://server/foo/bar
b/../c/d/../e../..f/.../../other/url/.. =>
http://server/foo/c/e../..f/other/url/..
-                             http://server/foo/bar
b/../c/d/../e../..f/.../..other/url/.. =>
http://server/foo/c/e../..f/.../..other/url/..
+                             http://server/foo/bar
a/b/../c/d/../e../..f/.../../other/url/.. =>
http://server/foo/a/c/e../..f/other
+                             http://server/foo/bar
b/../c/d/../e../..f/.../../other/url/.. =>
http://server/foo/c/e../..f/other
+                             http://server/foo/bar
b/../c/d/../e../..f/.../..other/url/.. =>
http://server/foo/c/e../..f/.../..other
                              http://server/foo/bar
a/b/../c/d/../e../..f/.../other/url/test..mp3 =>
http://server/foo/a/c/e../..f/.../other/url/test..mp3
                              http://server/foo/bar
/a/b/../c/d/../e../..f/.../other/url/test..mp3 =>
http://server/a/c/e../..f/.../other/url/test..mp3
                              http://server/foo/bar
/../../../../../other/url => http://server/other/url
Test url failed. Look at tests/data/fate/url.err for details.
make: *** [fate-url] Error 1
Zlomek, Josef July 27, 2020, 11:45 a.m. UTC | #2
Hi Steven,

".." is always a directory.

your tests (diff starting with -) are not correct.
My results (lines starting with +) are correct.

Josef

On Mon, Jul 27, 2020 at 1:36 PM Steven Liu <lingjiujianke@gmail.com> wrote:

> Josef Zlomek <josef@pex.com> 于2020年7月27日周一 下午6:57写道:
> >
> > 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 tmp_path is
> removed
> > - if the component is not empty, it is added to tmp_path
> >
> > The duplicate logic was removed from ff_make_absolute_url.
> >
> > Signed-off-by: Josef Zlomek <josef@pex.com>
> > ---
> >  libavformat/url.c | 70 +++++++++++++++++++++--------------------------
> >  1 file changed, 31 insertions(+), 39 deletions(-)
> >
> > diff --git a/libavformat/url.c b/libavformat/url.c
> > index 20463a6674..ccaa28a1ed 100644
> > --- a/libavformat/url.c
> > +++ b/libavformat/url.c
> > @@ -83,8 +83,9 @@ 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;
> > +    int tmp_len = 0;
> > +    const char *sep;
> > +    const char *next;
> >
> >      /* Get the path root of the url which start by "://" */
> >      if (p && (sep = strstr(p, "://"))) {
> > @@ -93,29 +94,39 @@ static void trim_double_dot_url(char *buf, const
> char *rel, int size)
> >          if (!root)
> >              return;
> >      }
> > +    if (*root == '/')
> > +        ++root;
> > +
> > +    /* Split the path by "/" and remove ".." and its corresponding
> directory. */
> > +    for (p = root; *p; p = next) {
> > +        next = strchr(p, '/');
> > +        if (!next)
> > +            next = p + strlen(p);
> > +
> > +        if (next - p == 2 && !strncmp(p, "..", 2)) {
> > +            /* remove the last directory from tmp_path */
> > +            while (tmp_len > 0 && tmp_path[--tmp_len] != '/')
> > +                ;
> > +            tmp_path[tmp_len] = '\0';
> > +        } else if (next > p) {
> > +            /* copy the current path component to tmp_path (including
> '/') */
> > +            if (tmp_len) {
> > +                av_strlcpy(tmp_path + tmp_len, "/", sizeof(tmp_path) -
> tmp_len);
> > +                ++tmp_len;
> > +            }
> > +            av_strlcpy(tmp_path + tmp_len, p, FFMIN(sizeof(tmp_path) -
> tmp_len,
> > +                                                    next - p + 1));
> > +            tmp_len += next - p;
> > +            tmp_path[tmp_len] = '\0';
> > +        }
> >
> > -    /* 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';
> > +        /* skip "/" */
> > +        while (*next == '/')
> > +            ++next;
> >      }
> >
> > -    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. */
> >      av_strlcpy(buf, rel, root - rel + 1);
> > -
> > -    if (!av_stristart(tmp_path, "/", NULL) && root != rel)
> > -        av_strlcat(buf, "/", size);
> > -
> >      av_strlcat(buf, tmp_path, size);
> >  }
> >
> > @@ -194,26 +205,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);
> > --
> > 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".
>
> maybe the last node is a file, not directory
>
> (base) liuqi05:clang liuqi$ make fate-url
> CC libavformat/tests/url.o
> LD libavformat/tests/url
> TEST    url
> --- src/tests/ref/fate/url 2020-07-27 19:30:58.000000000 +0800
> +++ tests/data/fate/url 2020-07-27 19:34:25.000000000 +0800
> @@ -15,9 +15,9 @@
>                               http://server/foo/bar //other/url
>   => http://other/url
>                               http://server/foo/bar
> ../../../../../other/url => http://server/other/url
>                               http://server/foo/bar
> a/b/../c/d/../e../..f/.../../other/url/test..mp3 =>
> http://server/foo/a/c/e../..f/other/url/test..mp3
> -                             http://server/foo/bar
> a/b/../c/d/../e../..f/.../../other/url/.. =>
> http://server/foo/a/c/e../..f/other/url/..
> -                             http://server/foo/bar
> b/../c/d/../e../..f/.../../other/url/.. =>
> http://server/foo/c/e../..f/other/url/..
> -                             http://server/foo/bar
> b/../c/d/../e../..f/.../..other/url/.. =>
> http://server/foo/c/e../..f/.../..other/url/..
> +                             http://server/foo/bar
> a/b/../c/d/../e../..f/.../../other/url/.. =>
> http://server/foo/a/c/e../..f/other
> +                             http://server/foo/bar
> b/../c/d/../e../..f/.../../other/url/.. =>
> http://server/foo/c/e../..f/other
> +                             http://server/foo/bar
> b/../c/d/../e../..f/.../..other/url/.. =>
> http://server/foo/c/e../..f/.../..other
>                               http://server/foo/bar
> a/b/../c/d/../e../..f/.../other/url/test..mp3 =>
> http://server/foo/a/c/e../..f/.../other/url/test..mp3
>                               http://server/foo/bar
> /a/b/../c/d/../e../..f/.../other/url/test..mp3 =>
> http://server/a/c/e../..f/.../other/url/test..mp3
>                               http://server/foo/bar
> /../../../../../other/url => http://server/other/url
> Test url failed. Look at tests/data/fate/url.err for details.
> make: *** [fate-url] Error 1
>
Zlomek, Josef July 27, 2020, 11:57 a.m. UTC | #3
I also noticed that the 3rd part did not make it to patchwork.
Maybe it is because it modifies the same file. I will squash it with the
first part.

Josef

On Mon, Jul 27, 2020 at 1:36 PM Steven Liu <lingjiujianke@gmail.com> wrote:

> Josef Zlomek <josef@pex.com> 于2020年7月27日周一 下午6:57写道:
> >
> > Signed-off-by: Josef Zlomek <josef@pex.com>
> > ---
> >  libavformat/url.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/libavformat/url.c b/libavformat/url.c
> > index ccaa28a1ed..28d12fd3de 100644
> > --- a/libavformat/url.c
> > +++ b/libavformat/url.c
> > @@ -186,14 +186,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 */
> > --
> > 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".
>
> this patch cannot get from:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1869
>
>
> Thanks
> Steven
>
Steven Liu July 27, 2020, 12:05 p.m. UTC | #4
Zlomek, Josef <josef@pex.com> 于2020年7月27日周一 下午7:45写道:
>
> Hi Steven,
>
> ".." is always a directory.
>
> your tests (diff starting with -) are not correct.
> My results (lines starting with +) are correct.
>
> Josef
>
> On Mon, Jul 27, 2020 at 1:36 PM Steven Liu <lingjiujianke@gmail.com> wrote:
>>
>> Josef Zlomek <josef@pex.com> 于2020年7月27日周一 下午6:57写道:
>> >
>> > 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 tmp_path is removed
>> > - if the component is not empty, it is added to tmp_path
>> >
>> > The duplicate logic was removed from ff_make_absolute_url.
>> >
>> > Signed-off-by: Josef Zlomek <josef@pex.com>
>> > ---
>> >  libavformat/url.c | 70 +++++++++++++++++++++--------------------------
>> >  1 file changed, 31 insertions(+), 39 deletions(-)
>> >
>> > diff --git a/libavformat/url.c b/libavformat/url.c
>> > index 20463a6674..ccaa28a1ed 100644
>> > --- a/libavformat/url.c
>> > +++ b/libavformat/url.c
>> > @@ -83,8 +83,9 @@ 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;
>> > +    int tmp_len = 0;
>> > +    const char *sep;
>> > +    const char *next;
>> >
>> >      /* Get the path root of the url which start by "://" */
>> >      if (p && (sep = strstr(p, "://"))) {
>> > @@ -93,29 +94,39 @@ static void trim_double_dot_url(char *buf, const char *rel, int size)
>> >          if (!root)
>> >              return;
>> >      }
>> > +    if (*root == '/')
>> > +        ++root;
>> > +
>> > +    /* Split the path by "/" and remove ".." and its corresponding directory. */
>> > +    for (p = root; *p; p = next) {
>> > +        next = strchr(p, '/');
>> > +        if (!next)
>> > +            next = p + strlen(p);
>> > +
>> > +        if (next - p == 2 && !strncmp(p, "..", 2)) {
>> > +            /* remove the last directory from tmp_path */
>> > +            while (tmp_len > 0 && tmp_path[--tmp_len] != '/')
>> > +                ;
>> > +            tmp_path[tmp_len] = '\0';
>> > +        } else if (next > p) {
>> > +            /* copy the current path component to tmp_path (including '/') */
>> > +            if (tmp_len) {
>> > +                av_strlcpy(tmp_path + tmp_len, "/", sizeof(tmp_path) - tmp_len);
>> > +                ++tmp_len;
>> > +            }
>> > +            av_strlcpy(tmp_path + tmp_len, p, FFMIN(sizeof(tmp_path) - tmp_len,
>> > +                                                    next - p + 1));
>> > +            tmp_len += next - p;
>> > +            tmp_path[tmp_len] = '\0';
>> > +        }
And I think only one loop is ok than this way.
I will resubmit a new patch, maybe that is simpler than yours.
>> >
>> > -    /* 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';
>> > +        /* skip "/" */
>> > +        while (*next == '/')
>> > +            ++next;
>> >      }
>> >
>> > -    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. */
>> >      av_strlcpy(buf, rel, root - rel + 1);
>> > -
>> > -    if (!av_stristart(tmp_path, "/", NULL) && root != rel)
>> > -        av_strlcat(buf, "/", size);
>> > -
>> >      av_strlcat(buf, tmp_path, size);
>> >  }
>> >
>> > @@ -194,26 +205,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);
>> > --
>> > 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".
>>
>> maybe the last node is a file, not directory
>>
>> (base) liuqi05:clang liuqi$ make fate-url
>> CC libavformat/tests/url.o
>> LD libavformat/tests/url
>> TEST    url
>> --- src/tests/ref/fate/url 2020-07-27 19:30:58.000000000 +0800
>> +++ tests/data/fate/url 2020-07-27 19:34:25.000000000 +0800
>> @@ -15,9 +15,9 @@
>>                               http://server/foo/bar //other/url
>>   => http://other/url
>>                               http://server/foo/bar
>> ../../../../../other/url => http://server/other/url
>>                               http://server/foo/bar
>> a/b/../c/d/../e../..f/.../../other/url/test..mp3 =>
>> http://server/foo/a/c/e../..f/other/url/test..mp3
>> -                             http://server/foo/bar
>> a/b/../c/d/../e../..f/.../../other/url/.. =>
>> http://server/foo/a/c/e../..f/other/url/..
>> -                             http://server/foo/bar
>> b/../c/d/../e../..f/.../../other/url/.. =>
>> http://server/foo/c/e../..f/other/url/..
>> -                             http://server/foo/bar
>> b/../c/d/../e../..f/.../..other/url/.. =>
>> http://server/foo/c/e../..f/.../..other/url/..
>> +                             http://server/foo/bar
>> a/b/../c/d/../e../..f/.../../other/url/.. =>
>> http://server/foo/a/c/e../..f/other
>> +                             http://server/foo/bar
>> b/../c/d/../e../..f/.../../other/url/.. =>
>> http://server/foo/c/e../..f/other
>> +                             http://server/foo/bar
>> b/../c/d/../e../..f/.../..other/url/.. =>
>> http://server/foo/c/e../..f/.../..other
>>                               http://server/foo/bar
>> a/b/../c/d/../e../..f/.../other/url/test..mp3 =>
>> http://server/foo/a/c/e../..f/.../other/url/test..mp3
>>                               http://server/foo/bar
>> /a/b/../c/d/../e../..f/.../other/url/test..mp3 =>
>> http://server/a/c/e../..f/.../other/url/test..mp3
>>                               http://server/foo/bar
>> /../../../../../other/url => http://server/other/url
>> Test url failed. Look at tests/data/fate/url.err for details.
>> make: *** [fate-url] Error 1
>
>
>
> --
> Josef Zlomek
Zlomek, Josef July 27, 2020, 12:14 p.m. UTC | #5
If your code is correct then good.

I also resubmit improved version.

Josef

On Mon, Jul 27, 2020 at 2:05 PM Steven Liu <lingjiujianke@gmail.com> wrote:

> Zlomek, Josef <josef@pex.com> 于2020年7月27日周一 下午7:45写道:
> >
> > Hi Steven,
> >
> > ".." is always a directory.
> >
> > your tests (diff starting with -) are not correct.
> > My results (lines starting with +) are correct.
> >
> > Josef
> >
> > On Mon, Jul 27, 2020 at 1:36 PM Steven Liu <lingjiujianke@gmail.com>
> wrote:
> >>
> >> Josef Zlomek <josef@pex.com> 于2020年7月27日周一 下午6:57写道:
> >> >
> >> > 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 tmp_path is
> removed
> >> > - if the component is not empty, it is added to tmp_path
> >> >
> >> > The duplicate logic was removed from ff_make_absolute_url.
> >> >
> >> > Signed-off-by: Josef Zlomek <josef@pex.com>
> >> > ---
> >> >  libavformat/url.c | 70
> +++++++++++++++++++++--------------------------
> >> >  1 file changed, 31 insertions(+), 39 deletions(-)
> >> >
> >> > diff --git a/libavformat/url.c b/libavformat/url.c
> >> > index 20463a6674..ccaa28a1ed 100644
> >> > --- a/libavformat/url.c
> >> > +++ b/libavformat/url.c
> >> > @@ -83,8 +83,9 @@ 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;
> >> > +    int tmp_len = 0;
> >> > +    const char *sep;
> >> > +    const char *next;
> >> >
> >> >      /* Get the path root of the url which start by "://" */
> >> >      if (p && (sep = strstr(p, "://"))) {
> >> > @@ -93,29 +94,39 @@ static void trim_double_dot_url(char *buf, const
> char *rel, int size)
> >> >          if (!root)
> >> >              return;
> >> >      }
> >> > +    if (*root == '/')
> >> > +        ++root;
> >> > +
> >> > +    /* Split the path by "/" and remove ".." and its corresponding
> directory. */
> >> > +    for (p = root; *p; p = next) {
> >> > +        next = strchr(p, '/');
> >> > +        if (!next)
> >> > +            next = p + strlen(p);
> >> > +
> >> > +        if (next - p == 2 && !strncmp(p, "..", 2)) {
> >> > +            /* remove the last directory from tmp_path */
> >> > +            while (tmp_len > 0 && tmp_path[--tmp_len] != '/')
> >> > +                ;
> >> > +            tmp_path[tmp_len] = '\0';
> >> > +        } else if (next > p) {
> >> > +            /* copy the current path component to tmp_path
> (including '/') */
> >> > +            if (tmp_len) {
> >> > +                av_strlcpy(tmp_path + tmp_len, "/", sizeof(tmp_path)
> - tmp_len);
> >> > +                ++tmp_len;
> >> > +            }
> >> > +            av_strlcpy(tmp_path + tmp_len, p, FFMIN(sizeof(tmp_path)
> - tmp_len,
> >> > +                                                    next - p + 1));
> >> > +            tmp_len += next - p;
> >> > +            tmp_path[tmp_len] = '\0';
> >> > +        }
> And I think only one loop is ok than this way.
> I will resubmit a new patch, maybe that is simpler than yours.
> >> >
> >> > -    /* 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';
> >> > +        /* skip "/" */
> >> > +        while (*next == '/')
> >> > +            ++next;
> >> >      }
> >> >
> >> > -    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. */
> >> >      av_strlcpy(buf, rel, root - rel + 1);
> >> > -
> >> > -    if (!av_stristart(tmp_path, "/", NULL) && root != rel)
> >> > -        av_strlcat(buf, "/", size);
> >> > -
> >> >      av_strlcat(buf, tmp_path, size);
> >> >  }
> >> >
> >> > @@ -194,26 +205,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);
> >> > --
> >> > 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".
> >>
> >> maybe the last node is a file, not directory
> >>
> >> (base) liuqi05:clang liuqi$ make fate-url
> >> CC libavformat/tests/url.o
> >> LD libavformat/tests/url
> >> TEST    url
> >> --- src/tests/ref/fate/url 2020-07-27 19:30:58.000000000 +0800
> >> +++ tests/data/fate/url 2020-07-27 19:34:25.000000000 +0800
> >> @@ -15,9 +15,9 @@
> >>                               http://server/foo/bar //other/url
> >>   => http://other/url
> >>                               http://server/foo/bar
> >> ../../../../../other/url => http://server/other/url
> >>                               http://server/foo/bar
> >> a/b/../c/d/../e../..f/.../../other/url/test..mp3 =>
> >> http://server/foo/a/c/e../..f/other/url/test..mp3
> >> -                             http://server/foo/bar
> >> a/b/../c/d/../e../..f/.../../other/url/.. =>
> >> http://server/foo/a/c/e../..f/other/url/..
> >> -                             http://server/foo/bar
> >> b/../c/d/../e../..f/.../../other/url/.. =>
> >> http://server/foo/c/e../..f/other/url/..
> >> -                             http://server/foo/bar
> >> b/../c/d/../e../..f/.../..other/url/.. =>
> >> http://server/foo/c/e../..f/.../..other/url/..
> >> +                             http://server/foo/bar
> >> a/b/../c/d/../e../..f/.../../other/url/.. =>
> >> http://server/foo/a/c/e../..f/other
> >> +                             http://server/foo/bar
> >> b/../c/d/../e../..f/.../../other/url/.. =>
> >> http://server/foo/c/e../..f/other
> >> +                             http://server/foo/bar
> >> b/../c/d/../e../..f/.../..other/url/.. =>
> >> http://server/foo/c/e../..f/.../..other
> >>                               http://server/foo/bar
> >> a/b/../c/d/../e../..f/.../other/url/test..mp3 =>
> >> http://server/foo/a/c/e../..f/.../other/url/test..mp3
> >>                               http://server/foo/bar
> >> /a/b/../c/d/../e../..f/.../other/url/test..mp3 =>
> >> http://server/a/c/e../..f/.../other/url/test..mp3
> >>                               http://server/foo/bar
> >> /../../../../../other/url => http://server/other/url
> >> Test url failed. Look at tests/data/fate/url.err for details.
> >> make: *** [fate-url] Error 1
> >
> >
> >
> > --
> > Josef Zlomek
>
diff mbox series

Patch

diff --git a/libavformat/url.c b/libavformat/url.c
index 20463a6674..ccaa28a1ed 100644
--- a/libavformat/url.c
+++ b/libavformat/url.c
@@ -83,8 +83,9 @@  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;
+    int tmp_len = 0;
+    const char *sep;
+    const char *next;
 
     /* Get the path root of the url which start by "://" */
     if (p && (sep = strstr(p, "://"))) {
@@ -93,29 +94,39 @@  static void trim_double_dot_url(char *buf, const char *rel, int size)
         if (!root)
             return;
     }
+    if (*root == '/')
+        ++root;
+
+    /* Split the path by "/" and remove ".." and its corresponding directory. */
+    for (p = root; *p; p = next) {
+        next = strchr(p, '/');
+        if (!next)
+            next = p + strlen(p);
+
+        if (next - p == 2 && !strncmp(p, "..", 2)) {
+            /* remove the last directory from tmp_path */
+            while (tmp_len > 0 && tmp_path[--tmp_len] != '/')
+                ;
+            tmp_path[tmp_len] = '\0';
+        } else if (next > p) {
+            /* copy the current path component to tmp_path (including '/') */
+            if (tmp_len) {
+                av_strlcpy(tmp_path + tmp_len, "/", sizeof(tmp_path) - tmp_len);
+                ++tmp_len;
+            }
+            av_strlcpy(tmp_path + tmp_len, p, FFMIN(sizeof(tmp_path) - tmp_len,
+                                                    next - p + 1));
+            tmp_len += next - p;
+            tmp_path[tmp_len] = '\0';
+        }
 
-    /* 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';
+        /* skip "/" */
+        while (*next == '/')
+            ++next;
     }
 
-    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. */
     av_strlcpy(buf, rel, root - rel + 1);
-
-    if (!av_stristart(tmp_path, "/", NULL) && root != rel)
-        av_strlcat(buf, "/", size);
-
     av_strlcat(buf, tmp_path, size);
 }
 
@@ -194,26 +205,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);