| Message ID | 20200407153039.13162-1-lance.lmwang@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v1] avformat/dashenc: use av_asprintf() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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
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 --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; }