diff mbox series

[FFmpeg-devel,v1] avformat/dashenc: use av_asprintf()

Message ID 20200407153039.13162-1-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,v1] avformat/dashenc: use av_asprintf()
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Limin Wang April 7, 2020, 3:30 p.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/dashenc.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

Comments

Andreas Rheinhardt April 7, 2020, 5:08 p.m. UTC | #1
lance.lmwang@gmail.com:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/dashenc.c | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> index 94d4639..0c281a4 100644
> --- a/libavformat/dashenc.c
> +++ b/libavformat/dashenc.c
> @@ -1832,27 +1832,16 @@ static void dashenc_delete_file(AVFormatContext *s, char *filename) {
>  static int dashenc_delete_segment_file(AVFormatContext *s, const char* file)
>  {
>      DASHContext *c = s->priv_data;
> -    size_t dirname_len, file_len;
> -    char filename[1024];
> -
> -    dirname_len = strlen(c->dirname);
> -    if (dirname_len >= sizeof(filename)) {
> -        av_log(s, AV_LOG_WARNING, "Cannot delete segments as the directory path is too long: %"PRIu64" characters: %s\n",
> -            (uint64_t)dirname_len, c->dirname);
> -        return AVERROR(ENAMETOOLONG);
> -    }
> +    char *filename;
>  
> -    memcpy(filename, c->dirname, dirname_len);
> -
> -    file_len = strlen(file);
> -    if ((dirname_len + file_len) >= sizeof(filename)) {
> -        av_log(s, AV_LOG_WARNING, "Cannot delete segments as the path is too long: %"PRIu64" characters: %s%s\n",
> -            (uint64_t)(dirname_len + file_len), c->dirname, file);
> -        return AVERROR(ENAMETOOLONG);
> +    filename = av_asprintf("%s%s", c->dirname, file);
> +    if (!filename) {
> +        av_log(s, AV_LOG_WARNING, "Out of memory for filename\n");
> +        return AVERROR(ENOMEM);
>      }
>  
> -    memcpy(filename + dirname_len, file, file_len + 1); // include the terminating zero
>      dashenc_delete_file(s, filename);
> +    av_free(filename);
>  
>      return 0;
>  }
> 
It's good to remove these arbitrary restrictions, but you are adding an
allocation whereas the old code had none. You can get the best of both
approaches by using an AVBPrint.

And the commit title should mention that you remove an arbitrary
restriction on the length of filenames. This is more important than the
switch from one function to another.

- Andreas
Limin Wang April 7, 2020, 10:37 p.m. UTC | #2
On Tue, Apr 07, 2020 at 07:08:56PM +0200, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/dashenc.c | 23 ++++++-----------------
> >  1 file changed, 6 insertions(+), 17 deletions(-)
> > 
> > diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> > index 94d4639..0c281a4 100644
> > --- a/libavformat/dashenc.c
> > +++ b/libavformat/dashenc.c
> > @@ -1832,27 +1832,16 @@ static void dashenc_delete_file(AVFormatContext *s, char *filename) {
> >  static int dashenc_delete_segment_file(AVFormatContext *s, const char* file)
> >  {
> >      DASHContext *c = s->priv_data;
> > -    size_t dirname_len, file_len;
> > -    char filename[1024];
> > -
> > -    dirname_len = strlen(c->dirname);
> > -    if (dirname_len >= sizeof(filename)) {
> > -        av_log(s, AV_LOG_WARNING, "Cannot delete segments as the directory path is too long: %"PRIu64" characters: %s\n",
> > -            (uint64_t)dirname_len, c->dirname);
> > -        return AVERROR(ENAMETOOLONG);
> > -    }
> > +    char *filename;
> >  
> > -    memcpy(filename, c->dirname, dirname_len);
> > -
> > -    file_len = strlen(file);
> > -    if ((dirname_len + file_len) >= sizeof(filename)) {
> > -        av_log(s, AV_LOG_WARNING, "Cannot delete segments as the path is too long: %"PRIu64" characters: %s%s\n",
> > -            (uint64_t)(dirname_len + file_len), c->dirname, file);
> > -        return AVERROR(ENAMETOOLONG);
> > +    filename = av_asprintf("%s%s", c->dirname, file);
> > +    if (!filename) {
> > +        av_log(s, AV_LOG_WARNING, "Out of memory for filename\n");
> > +        return AVERROR(ENOMEM);
> >      }
> >  
> > -    memcpy(filename + dirname_len, file, file_len + 1); // include the terminating zero
> >      dashenc_delete_file(s, filename);
> > +    av_free(filename);
> >  
> >      return 0;
> >  }
> > 
> It's good to remove these arbitrary restrictions, but you are adding an
> allocation whereas the old code had none. You can get the best of both
> approaches by using an AVBPrint.
sure, I'll change to use the AVBPrint.

> 
> And the commit title should mention that you remove an arbitrary
> restriction on the length of filenames. This is more important than the
> switch from one function to another.

OK, will update it

> 
> - Andreas
> _______________________________________________
> 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/dashenc.c b/libavformat/dashenc.c
index 94d4639..0c281a4 100644
--- a/libavformat/dashenc.c
+++ b/libavformat/dashenc.c
@@ -1832,27 +1832,16 @@  static void dashenc_delete_file(AVFormatContext *s, char *filename) {
 static int dashenc_delete_segment_file(AVFormatContext *s, const char* file)
 {
     DASHContext *c = s->priv_data;
-    size_t dirname_len, file_len;
-    char filename[1024];
-
-    dirname_len = strlen(c->dirname);
-    if (dirname_len >= sizeof(filename)) {
-        av_log(s, AV_LOG_WARNING, "Cannot delete segments as the directory path is too long: %"PRIu64" characters: %s\n",
-            (uint64_t)dirname_len, c->dirname);
-        return AVERROR(ENAMETOOLONG);
-    }
+    char *filename;
 
-    memcpy(filename, c->dirname, dirname_len);
-
-    file_len = strlen(file);
-    if ((dirname_len + file_len) >= sizeof(filename)) {
-        av_log(s, AV_LOG_WARNING, "Cannot delete segments as the path is too long: %"PRIu64" characters: %s%s\n",
-            (uint64_t)(dirname_len + file_len), c->dirname, file);
-        return AVERROR(ENAMETOOLONG);
+    filename = av_asprintf("%s%s", c->dirname, file);
+    if (!filename) {
+        av_log(s, AV_LOG_WARNING, "Out of memory for filename\n");
+        return AVERROR(ENOMEM);
     }
 
-    memcpy(filename + dirname_len, file, file_len + 1); // include the terminating zero
     dashenc_delete_file(s, filename);
+    av_free(filename);
 
     return 0;
 }