diff mbox series

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

Message ID 20200727130748.90906-1-lq@chinaffmpeg.org
State New
Headers show
Series [FFmpeg-devel,v3,1/2] avformat/url: rework for trim_double_dot_url
Related show

Checks

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

Commit Message

Steven Liu July 27, 2020, 1:07 p.m. UTC
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@chinaffmpeg.org>
---
 libavformat/url.c | 83 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 64 insertions(+), 19 deletions(-)

Comments

Nicolas George July 27, 2020, 1:47 p.m. UTC | #1
Steven Liu (12020-07-27):
> 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.

?!?!??!!!?

Why do you think you need extra buffers?

This patch turns a rather straightforward task into a nightmare of
string copies.

Regards,
Zlomek, Josef July 27, 2020, 1:50 p.m. UTC | #2
Generally, this seems to be too complicated with too much of copying.

On Mon, Jul 27, 2020 at 3:08 PM Steven Liu <lq@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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

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;
+
     /* 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 (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)