Message ID | 20200408134807.19154-1-andreas.rheinhardt@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [FFmpeg-devel,1/3] avformat/hlsenc: Fix memleak when deleting old segments | expand |
Context | Check | Description |
---|---|---|
andriy/ffmpeg-patchwork | warning | Failed to apply patch |
> 2020年4月8日 下午9:48,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道: > > when deleting old segments. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Further todos for this function: > 1. Allocating vtt_dirname_r in order to get vtt_dirname is actually a > waste. One would need a dirname to AVBPrint function to avoid it. > 2. replace_*_data_in_filename already work with AVBPrints internally. > Refactor them to allow to directly output to a caller-supplied AVBPrint > and use it to avoid allocating dirname_repl. > > libavformat/hlsenc.c | 57 +++++++++++++++++++++----------------------- > 1 file changed, 27 insertions(+), 30 deletions(-) > > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c > index 0f1f558590..d7562ad20c 100644 > --- a/libavformat/hlsenc.c > +++ b/libavformat/hlsenc.c > @@ -489,18 +489,20 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls, > > HLSSegment *segment, *previous_segment = NULL; > float playlist_duration = 0.0f; > - int ret = 0, path_size, sub_path_size; > + int ret = 0; > int segment_cnt = 0; > - char *dirname = NULL, *sub_path; > + AVBPrint path; > + char *dirname = NULL; > char *dirname_r = NULL; > char *dirname_repl = NULL; > - char *path = NULL; > char *vtt_dirname = NULL; > char *vtt_dirname_r = NULL; > AVDictionary *options = NULL; > AVIOContext *out = NULL; > const char *proto = NULL; > > + av_bprint_init(&path, 0, AV_BPRINT_SIZE_UNLIMITED); > + > segment = vs->segments; > while (segment) { > playlist_duration += segment->duration; > @@ -550,73 +552,68 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls, > while (segment) { > av_log(hls, AV_LOG_DEBUG, "deleting old segment %s\n", > segment->filename); > - path_size = (hls->use_localtime_mkdir ? 0 : strlen(dirname)+1) + strlen(segment->filename) + 1; > - path = av_malloc(path_size); > - if (!path) { > + if (!hls->use_localtime_mkdir) // segment->filename contains basename only > + av_bprintf(&path, "%s%c", dirname, SEPARATOR); > + av_bprintf(&path, "%s", segment->filename); > + > + if (!av_bprint_is_complete(&path)) { > ret = AVERROR(ENOMEM); > goto fail; > } > > - if (hls->use_localtime_mkdir) > - av_strlcpy(path, segment->filename, path_size); > - else { // segment->filename contains basename only > - snprintf(path, path_size, "%s%c%s", dirname, SEPARATOR, segment->filename); > - } > - > proto = avio_find_protocol_name(s->url); > if (hls->method || (proto && !av_strcasecmp(proto, "http"))) { > av_dict_set(&options, "method", "DELETE", 0); > - if ((ret = vs->avf->io_open(vs->avf, &out, path, AVIO_FLAG_WRITE, &options)) < 0) { > + if ((ret = vs->avf->io_open(vs->avf, &out, path.str, > + AVIO_FLAG_WRITE, &options)) < 0) { > if (hls->ignore_io_errors) > ret = 0; > goto fail; > } > ff_format_io_close(vs->avf, &out); > - } else if (unlink(path) < 0) { > + } else if (unlink(path.str) < 0) { > av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n", > - path, strerror(errno)); > + path.str, strerror(errno)); > } > > if ((segment->sub_filename[0] != '\0')) { > vtt_dirname_r = av_strdup(vs->vtt_avf->url); > vtt_dirname = (char*)av_dirname(vtt_dirname_r); > - sub_path_size = strlen(segment->sub_filename) + 1 + strlen(vtt_dirname) + 1; > - sub_path = av_malloc(sub_path_size); > - if (!sub_path) { > + > + av_bprint_clear(&path); > + av_bprintf(&path, "%s%c%s", vtt_dirname, SEPARATOR, > + segment->sub_filename); > + av_freep(&vtt_dirname_r); > + > + if (!av_bprint_is_complete(&path)) { > ret = AVERROR(ENOMEM); > goto fail; > } > > - snprintf(sub_path, sub_path_size, "%s%c%s", vtt_dirname, SEPARATOR, segment->sub_filename); > - > - av_freep(&vtt_dirname_r); > - > if (hls->method || (proto && !av_strcasecmp(proto, "http"))) { > av_dict_set(&options, "method", "DELETE", 0); > - if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, sub_path, AVIO_FLAG_WRITE, &options)) < 0) { > + if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, path.str, > + AVIO_FLAG_WRITE, &options)) < 0) { > if (hls->ignore_io_errors) > ret = 0; > - av_freep(&sub_path); > goto fail; > } > ff_format_io_close(vs->vtt_avf, &out); > - } else if (unlink(sub_path) < 0) { > + } else if (unlink(path.str) < 0) { > av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n", > - sub_path, strerror(errno)); > + path.str, strerror(errno)); > } > - av_freep(&sub_path); > } > - av_freep(&path); > + av_bprint_clear(&path); > previous_segment = segment; > segment = previous_segment->next; > av_freep(&previous_segment); > } > > fail: > - av_freep(&path); > + av_bprint_finalize(&path, NULL); > av_freep(&dirname_r); > av_freep(&dirname_repl); > - av_freep(&vtt_dirname_r); > > return ret; > } > -- > 2.20.1 > > _______________________________________________ > 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". LGTM Thanks Steven Liu
Andreas Rheinhardt (12020-04-08): > when deleting old segments. Your commit message got wrapped. ;-) I do not maintain this part of the code. The use of BPrint seems ok, not changing the semantic. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Further todos for this function: > 1. Allocating vtt_dirname_r in order to get vtt_dirname is actually a > waste. One would need a dirname to AVBPrint function to avoid it. > 2. replace_*_data_in_filename already work with AVBPrints internally. > Refactor them to allow to directly output to a caller-supplied AVBPrint > and use it to avoid allocating dirname_repl. > > libavformat/hlsenc.c | 57 +++++++++++++++++++++----------------------- > 1 file changed, 27 insertions(+), 30 deletions(-) > > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c > index 0f1f558590..d7562ad20c 100644 > --- a/libavformat/hlsenc.c > +++ b/libavformat/hlsenc.c > @@ -489,18 +489,20 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls, > > HLSSegment *segment, *previous_segment = NULL; > float playlist_duration = 0.0f; > - int ret = 0, path_size, sub_path_size; > + int ret = 0; > int segment_cnt = 0; > - char *dirname = NULL, *sub_path; > + AVBPrint path; > + char *dirname = NULL; > char *dirname_r = NULL; > char *dirname_repl = NULL; > - char *path = NULL; > char *vtt_dirname = NULL; > char *vtt_dirname_r = NULL; > AVDictionary *options = NULL; > AVIOContext *out = NULL; > const char *proto = NULL; > > + av_bprint_init(&path, 0, AV_BPRINT_SIZE_UNLIMITED); > + > segment = vs->segments; > while (segment) { > playlist_duration += segment->duration; > @@ -550,73 +552,68 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls, > while (segment) { > av_log(hls, AV_LOG_DEBUG, "deleting old segment %s\n", > segment->filename); > - path_size = (hls->use_localtime_mkdir ? 0 : strlen(dirname)+1) + strlen(segment->filename) + 1; > - path = av_malloc(path_size); > - if (!path) { > + if (!hls->use_localtime_mkdir) // segment->filename contains basename only > + av_bprintf(&path, "%s%c", dirname, SEPARATOR); > + av_bprintf(&path, "%s", segment->filename); > + > + if (!av_bprint_is_complete(&path)) { > ret = AVERROR(ENOMEM); > goto fail; > } > > - if (hls->use_localtime_mkdir) > - av_strlcpy(path, segment->filename, path_size); > - else { // segment->filename contains basename only > - snprintf(path, path_size, "%s%c%s", dirname, SEPARATOR, segment->filename); > - } > - > proto = avio_find_protocol_name(s->url); > if (hls->method || (proto && !av_strcasecmp(proto, "http"))) { > av_dict_set(&options, "method", "DELETE", 0); > - if ((ret = vs->avf->io_open(vs->avf, &out, path, AVIO_FLAG_WRITE, &options)) < 0) { > + if ((ret = vs->avf->io_open(vs->avf, &out, path.str, > + AVIO_FLAG_WRITE, &options)) < 0) { > if (hls->ignore_io_errors) > ret = 0; > goto fail; > } > ff_format_io_close(vs->avf, &out); > - } else if (unlink(path) < 0) { > + } else if (unlink(path.str) < 0) { > av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n", > - path, strerror(errno)); > + path.str, strerror(errno)); > } > > if ((segment->sub_filename[0] != '\0')) { > vtt_dirname_r = av_strdup(vs->vtt_avf->url); > vtt_dirname = (char*)av_dirname(vtt_dirname_r); > - sub_path_size = strlen(segment->sub_filename) + 1 + strlen(vtt_dirname) + 1; > - sub_path = av_malloc(sub_path_size); > - if (!sub_path) { > + > + av_bprint_clear(&path); > + av_bprintf(&path, "%s%c%s", vtt_dirname, SEPARATOR, > + segment->sub_filename); > + av_freep(&vtt_dirname_r); > + > + if (!av_bprint_is_complete(&path)) { > ret = AVERROR(ENOMEM); > goto fail; > } > > - snprintf(sub_path, sub_path_size, "%s%c%s", vtt_dirname, SEPARATOR, segment->sub_filename); > - > - av_freep(&vtt_dirname_r); > - > if (hls->method || (proto && !av_strcasecmp(proto, "http"))) { > av_dict_set(&options, "method", "DELETE", 0); > - if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, sub_path, AVIO_FLAG_WRITE, &options)) < 0) { > + if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, path.str, > + AVIO_FLAG_WRITE, &options)) < 0) { > if (hls->ignore_io_errors) > ret = 0; > - av_freep(&sub_path); > goto fail; > } > ff_format_io_close(vs->vtt_avf, &out); > - } else if (unlink(sub_path) < 0) { > + } else if (unlink(path.str) < 0) { > av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n", > - sub_path, strerror(errno)); > + path.str, strerror(errno)); > } > - av_freep(&sub_path); > } > - av_freep(&path); > + av_bprint_clear(&path); I would advise to put this before the first use instead of here: it will be more robust if somebody adds "continue" in the loop. > previous_segment = segment; > segment = previous_segment->next; > av_freep(&previous_segment); > } > > fail: > - av_freep(&path); > + av_bprint_finalize(&path, NULL); > av_freep(&dirname_r); > av_freep(&dirname_repl); > - av_freep(&vtt_dirname_r); > > return ret; > } By the way, some time ago I proposed an API that wraps BPrint into something a little more elegant and a lot more versatile, designed for all cases where we have functions that build and return strings. Since we are focussing on BPrint these days, I would appreciate if you had input about it: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/254042.html https://ffmpeg.org/pipermail/ffmpeg-devel/2020-January/255767.html Regards,
diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 0f1f558590..d7562ad20c 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -489,18 +489,20 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls, HLSSegment *segment, *previous_segment = NULL; float playlist_duration = 0.0f; - int ret = 0, path_size, sub_path_size; + int ret = 0; int segment_cnt = 0; - char *dirname = NULL, *sub_path; + AVBPrint path; + char *dirname = NULL; char *dirname_r = NULL; char *dirname_repl = NULL; - char *path = NULL; char *vtt_dirname = NULL; char *vtt_dirname_r = NULL; AVDictionary *options = NULL; AVIOContext *out = NULL; const char *proto = NULL; + av_bprint_init(&path, 0, AV_BPRINT_SIZE_UNLIMITED); + segment = vs->segments; while (segment) { playlist_duration += segment->duration; @@ -550,73 +552,68 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls, while (segment) { av_log(hls, AV_LOG_DEBUG, "deleting old segment %s\n", segment->filename); - path_size = (hls->use_localtime_mkdir ? 0 : strlen(dirname)+1) + strlen(segment->filename) + 1; - path = av_malloc(path_size); - if (!path) { + if (!hls->use_localtime_mkdir) // segment->filename contains basename only + av_bprintf(&path, "%s%c", dirname, SEPARATOR); + av_bprintf(&path, "%s", segment->filename); + + if (!av_bprint_is_complete(&path)) { ret = AVERROR(ENOMEM); goto fail; } - if (hls->use_localtime_mkdir) - av_strlcpy(path, segment->filename, path_size); - else { // segment->filename contains basename only - snprintf(path, path_size, "%s%c%s", dirname, SEPARATOR, segment->filename); - } - proto = avio_find_protocol_name(s->url); if (hls->method || (proto && !av_strcasecmp(proto, "http"))) { av_dict_set(&options, "method", "DELETE", 0); - if ((ret = vs->avf->io_open(vs->avf, &out, path, AVIO_FLAG_WRITE, &options)) < 0) { + if ((ret = vs->avf->io_open(vs->avf, &out, path.str, + AVIO_FLAG_WRITE, &options)) < 0) { if (hls->ignore_io_errors) ret = 0; goto fail; } ff_format_io_close(vs->avf, &out); - } else if (unlink(path) < 0) { + } else if (unlink(path.str) < 0) { av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n", - path, strerror(errno)); + path.str, strerror(errno)); } if ((segment->sub_filename[0] != '\0')) { vtt_dirname_r = av_strdup(vs->vtt_avf->url); vtt_dirname = (char*)av_dirname(vtt_dirname_r); - sub_path_size = strlen(segment->sub_filename) + 1 + strlen(vtt_dirname) + 1; - sub_path = av_malloc(sub_path_size); - if (!sub_path) { + + av_bprint_clear(&path); + av_bprintf(&path, "%s%c%s", vtt_dirname, SEPARATOR, + segment->sub_filename); + av_freep(&vtt_dirname_r); + + if (!av_bprint_is_complete(&path)) { ret = AVERROR(ENOMEM); goto fail; } - snprintf(sub_path, sub_path_size, "%s%c%s", vtt_dirname, SEPARATOR, segment->sub_filename); - - av_freep(&vtt_dirname_r); - if (hls->method || (proto && !av_strcasecmp(proto, "http"))) { av_dict_set(&options, "method", "DELETE", 0); - if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, sub_path, AVIO_FLAG_WRITE, &options)) < 0) { + if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, path.str, + AVIO_FLAG_WRITE, &options)) < 0) { if (hls->ignore_io_errors) ret = 0; - av_freep(&sub_path); goto fail; } ff_format_io_close(vs->vtt_avf, &out); - } else if (unlink(sub_path) < 0) { + } else if (unlink(path.str) < 0) { av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n", - sub_path, strerror(errno)); + path.str, strerror(errno)); } - av_freep(&sub_path); } - av_freep(&path); + av_bprint_clear(&path); previous_segment = segment; segment = previous_segment->next; av_freep(&previous_segment); } fail: - av_freep(&path); + av_bprint_finalize(&path, NULL); av_freep(&dirname_r); av_freep(&dirname_repl); - av_freep(&vtt_dirname_r); return ret; }
when deleting old segments. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- Further todos for this function: 1. Allocating vtt_dirname_r in order to get vtt_dirname is actually a waste. One would need a dirname to AVBPrint function to avoid it. 2. replace_*_data_in_filename already work with AVBPrints internally. Refactor them to allow to directly output to a caller-supplied AVBPrint and use it to avoid allocating dirname_repl. libavformat/hlsenc.c | 57 +++++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 30 deletions(-)