Re: [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  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 
> ---
>  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) {
>  

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

[Nicolas George]

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,

-- 
  Nicolas George


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

[Steven Liu]

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



___
ffmpeg-devel mailing list