| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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,
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 --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)
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(-)