[FFmpeg-devel] [PATCH v3 1/2] avformat/url: rework for trim_double_dot_url

[Zlomek, Josef]

Generally, this seems to be too complicated with too much of copying.

On Mon, Jul 27, 2020 at 3:08 PM Steven Liu <lq at chinaffmpeg.org> wrote:

> use two buffer for storage the path and node of URI
> remove the last node of the path if the next node is ..
> change the static strings to dynamic alloc space by size argument.
>
> Signed-off-by: Steven Liu <lq at chinaffmpeg.org>
> ---
>  libavformat/url.c | 83 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 64 insertions(+), 19 deletions(-)
>
> diff --git a/libavformat/url.c b/libavformat/url.c
> index 20463a6674..4fb703de78 100644
> --- a/libavformat/url.c
> +++ b/libavformat/url.c
> @@ -82,41 +82,78 @@ 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 *tmp_path = NULL;
> +    char *second_buf = NULL;
> +    char *first_buf = NULL;
>      char *sep;
>      char *node;
>
> +    tmp_path = av_mallocz(size);
> +    if (!tmp_path)
> +        return;
> +
> +    second_buf = av_mallocz(size);
> +    if (!second_buf)
> +        goto fail;
> +
> +    first_buf = av_mallocz(size);
> +    if (!first_buf)
> +        goto fail;
>

Why 3 buffers? 1 would be enough.

+
>      /* Get the path root of the url which start by "://" */
>      if (p && (sep = strstr(p, "://"))) {
>          sep += 3;
>          root = strchr(sep, '/');
>          if (!root)
> -            return;
> +            goto fail;
>      }
> +    av_strlcat(tmp_path, p, root - p + 1);
>
>      /* 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 (*p == '/')
> +        p++;
> +    while (p && (node = strchr(p, '/'))) {
> +        av_strlcpy(second_buf, p, node - p + 1);
> +        p = node + 1;
> +        if (!strcmp(second_buf, "..")) {
>

If you used if(node - p == 2 && !strncmp(node, "..", 2)) you would not have
to copy to the second buffer.

+            if (strlen(first_buf) <= 0)
> +                continue;
> +            sep = strrchr(first_buf, '/');
> +            if (sep)
> +                sep[0] = '\0';
> +            else
> +                memset(first_buf, 0, size);
> +
> +            memset(second_buf, 0, size);
> +        } else {
> +            av_strlcat(first_buf, "/", size);
> +            av_strlcat(first_buf, second_buf, size);
> +        }
> +    }
> +    if (!strcmp(p, "..")) {
> +        sep = strrchr(first_buf, '/');
>          if (sep)
>              sep[0] = '\0';
>          else
> -            tmp_path[0] = '\0';
> +            memset(first_buf, 0, size);
> +    } else {
> +        av_strlcat(first_buf, "/", size);
> +        av_strlcat(first_buf, p, size);
>      }
>
> -    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);
> +    if (p > root) {
> +        av_strlcat(tmp_path, first_buf, size);
> +    } else {
> +        av_strlcat(tmp_path, p, size);
> +    }
>
>      av_strlcat(buf, tmp_path, size);
> +
> +fail:
> +    av_free(second_buf);
> +    av_free(first_buf);
> +    av_free(tmp_path);
>  }
>
>  void ff_make_absolute_url(char *buf, int size, const char *base,
> @@ -124,9 +161,11 @@ void ff_make_absolute_url(char *buf, int size, const
> char *base,
>  {
>      char *sep, *path_query;
>      char *root, *p;
> -    char tmp_path[MAX_URL_SIZE];
> +    char *tmp_path = av_mallocz(size);
> +
> +    if (!tmp_path)
> +        return;
>
> -    memset(tmp_path, 0, sizeof(tmp_path));
>      /* Absolute path, relative to the current server */
>      if (base && strstr(base, "://") && rel[0] == '/') {
>          if (base != buf)
> @@ -148,12 +187,14 @@ void ff_make_absolute_url(char *buf, int size, const
> char *base,
>          trim_double_dot_url(tmp_path, buf, size);
>          memset(buf, 0, size);
>          av_strlcpy(buf, tmp_path, size);
> +        av_free(tmp_path);
>          return;
>      }
>      /* If rel actually is an absolute url, just copy it */
>      if (!base || strstr(rel, "://") || rel[0] == '/') {
>          memset(buf, 0, size);
>          trim_double_dot_url(buf, rel, size);
> +        av_free(tmp_path);
>          return;
>      }
>      if (base != buf)
> @@ -170,6 +211,7 @@ void ff_make_absolute_url(char *buf, int size, const
> char *base,
>          trim_double_dot_url(tmp_path, buf, size);
>          memset(buf, 0, size);
>          av_strlcpy(buf, tmp_path, size);
> +        av_free(tmp_path);
>          return;
>      }
>
> @@ -180,8 +222,10 @@ void ff_make_absolute_url(char *buf, int size, const
> char *base,
>          if (sep) {
>              sep += 3;
>              root = strchr(sep, '/');
> -            if (!root)
> +            if (!root) {
> +                av_free(tmp_path);
>                  return;
> +            }
>          }
>      }
>
> @@ -218,6 +262,7 @@ void ff_make_absolute_url(char *buf, int size, const
> char *base,
>      trim_double_dot_url(tmp_path, buf, size);
>      memset(buf, 0, size);
>      av_strlcpy(buf, tmp_path, size);
> +    av_free(tmp_path);
>  }
>
>  AVIODirEntry *ff_alloc_dir_entry(void)
> --
> 2.25.0
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".



-- 
Josef Zlomek