diff mbox

[FFmpeg-devel,05/16] avformat/hlsenc: Fix leak of options when writing packets

Message ID 20191216000418.24707-6-andreas.rheinhardt@gmail.com
State Accepted
Commit bd131b64bc308ab036d0bbe9da0a49f482ef94f9
Headers show

Commit Message

Andreas Rheinhardt Dec. 16, 2019, 12:04 a.m. UTC
Under certain circumstances hls_write_packet() would add options to an
AVDictionary. Said dictionary was never explicitly freed, instead it was
presumed that these options would be consumed when opening a new
IO-context. This left several possibilities for memleaks:

a) When no new IO-context would be opened at all. This is possible when
using both the flags temp_file and single_file together with a file
output.
b) When an error happens before one actually tries to open the new
IO-context.
c) When the new IO-context does not consume all options.

All three have been fixed; furthermore, the AVDictionary has been put
into a smaller scope (namely the only part of hls_write_packet() where
it is actually used).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
This patch is concerned with fixing memleaks. Yet I noticed a
discrepancy in the handling of options between the first try to open
output and the second one (the one for reflushing in case the first one
fails): The second one only gets the options not consumed in the first
one. Is this intended?

 libavformat/hlsenc.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

Steven Liu Dec. 23, 2019, 3:57 a.m. UTC | #1
> 在 2019年12月16日,08:04,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
> 
> Under certain circumstances hls_write_packet() would add options to an
> AVDictionary. Said dictionary was never explicitly freed, instead it was
> presumed that these options would be consumed when opening a new
> IO-context. This left several possibilities for memleaks:
> 
> a) When no new IO-context would be opened at all. This is possible when
> using both the flags temp_file and single_file together with a file
> output.
> b) When an error happens before one actually tries to open the new
> IO-context.
> c) When the new IO-context does not consume all options.
> 
> All three have been fixed; furthermore, the AVDictionary has been put
> into a smaller scope (namely the only part of hls_write_packet() where
> it is actually used).
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> This patch is concerned with fixing memleaks. Yet I noticed a
> discrepancy in the handling of options between the first try to open
> output and the second one (the one for reflushing in case the first one
> fails): The second one only gets the options not consumed in the first
> one. Is this intended?
> 
> libavformat/hlsenc.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 5695af2208..5b3856099c 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -2241,7 +2241,6 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>     int use_temp_file = 0;
>     uint8_t *buffer = NULL;
>     VariantStream *vs = NULL;
> -    AVDictionary *options = NULL;
>     char *old_filename = NULL;
> 
>     for (i = 0; i < hls->nb_varstreams; i++) {
> @@ -2343,11 +2342,6 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>             use_temp_file = proto && !strcmp(proto, "file") && (hls->flags & HLS_TEMP_FILE);
>         }
> 
> -        // look to rename the asset name
> -        if (use_temp_file) {
> -            av_dict_set(&options, "mpegts_flags", "resend_headers", 0);
> -        }
> -
>         if (hls->flags & HLS_SINGLE_FILE) {
>             ret = flush_dynbuf(vs, &range_length);
>             av_freep(&vs->temp_buffer);
> @@ -2356,8 +2350,8 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>             }
>             vs->size = range_length;
>         } else {
> -            set_http_options(s, &options, hls);
>             if ((hls->max_seg_size > 0 && (vs->size >= hls->max_seg_size)) || !byterange_mode) {
> +                AVDictionary *options = NULL;
>                 char *filename = NULL;
>                 if (hls->key_info_file || hls->encrypt) {
>                     av_dict_set(&options, "encryption_key", hls->key_string, 0);
> @@ -2367,12 +2361,21 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>                     filename = av_asprintf("%s", oc->url);
>                 }
>                 if (!filename) {
> +                    av_dict_free(&options);
>                     return AVERROR(ENOMEM);
>                 }
> +
> +                // look to rename the asset name
> +                if (use_temp_file)
> +                    av_dict_set(&options, "mpegts_flags", "resend_headers", 0);
> +
> +                set_http_options(s, &options, hls);
> +
>                 ret = hlsenc_io_open(s, &vs->out, filename, &options);
>                 if (ret < 0) {
>                     av_log(s, hls->ignore_io_errors ? AV_LOG_WARNING : AV_LOG_ERROR,
>                            "Failed to open file '%s'\n", filename);
> +                    av_dict_free(&options);
>                     return hls->ignore_io_errors ? 0 : ret;
>                 }
>                 if (hls->segment_type == SEGMENT_TYPE_FMP4) {
> @@ -2380,6 +2383,7 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>                 }
>                 ret = flush_dynbuf(vs, &range_length);
>                 if (ret < 0) {
> +                    av_dict_free(&options);
>                     return ret;
>                 }
>                 ret = hlsenc_io_close(s, &vs->out, filename);
> @@ -2391,6 +2395,7 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>                     reflush_dynbuf(vs, &range_length);
>                     ret = hlsenc_io_close(s, &vs->out, filename);
>                 }
> +                av_dict_free(&options);
>                 av_freep(&vs->temp_buffer);
>                 av_freep(&filename);
>             }
> -- 
> 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
diff mbox

Patch

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 5695af2208..5b3856099c 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -2241,7 +2241,6 @@  static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
     int use_temp_file = 0;
     uint8_t *buffer = NULL;
     VariantStream *vs = NULL;
-    AVDictionary *options = NULL;
     char *old_filename = NULL;
 
     for (i = 0; i < hls->nb_varstreams; i++) {
@@ -2343,11 +2342,6 @@  static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
             use_temp_file = proto && !strcmp(proto, "file") && (hls->flags & HLS_TEMP_FILE);
         }
 
-        // look to rename the asset name
-        if (use_temp_file) {
-            av_dict_set(&options, "mpegts_flags", "resend_headers", 0);
-        }
-
         if (hls->flags & HLS_SINGLE_FILE) {
             ret = flush_dynbuf(vs, &range_length);
             av_freep(&vs->temp_buffer);
@@ -2356,8 +2350,8 @@  static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
             }
             vs->size = range_length;
         } else {
-            set_http_options(s, &options, hls);
             if ((hls->max_seg_size > 0 && (vs->size >= hls->max_seg_size)) || !byterange_mode) {
+                AVDictionary *options = NULL;
                 char *filename = NULL;
                 if (hls->key_info_file || hls->encrypt) {
                     av_dict_set(&options, "encryption_key", hls->key_string, 0);
@@ -2367,12 +2361,21 @@  static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
                     filename = av_asprintf("%s", oc->url);
                 }
                 if (!filename) {
+                    av_dict_free(&options);
                     return AVERROR(ENOMEM);
                 }
+
+                // look to rename the asset name
+                if (use_temp_file)
+                    av_dict_set(&options, "mpegts_flags", "resend_headers", 0);
+
+                set_http_options(s, &options, hls);
+
                 ret = hlsenc_io_open(s, &vs->out, filename, &options);
                 if (ret < 0) {
                     av_log(s, hls->ignore_io_errors ? AV_LOG_WARNING : AV_LOG_ERROR,
                            "Failed to open file '%s'\n", filename);
+                    av_dict_free(&options);
                     return hls->ignore_io_errors ? 0 : ret;
                 }
                 if (hls->segment_type == SEGMENT_TYPE_FMP4) {
@@ -2380,6 +2383,7 @@  static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
                 }
                 ret = flush_dynbuf(vs, &range_length);
                 if (ret < 0) {
+                    av_dict_free(&options);
                     return ret;
                 }
                 ret = hlsenc_io_close(s, &vs->out, filename);
@@ -2391,6 +2395,7 @@  static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
                     reflush_dynbuf(vs, &range_length);
                     ret = hlsenc_io_close(s, &vs->out, filename);
                 }
+                av_dict_free(&options);
                 av_freep(&vs->temp_buffer);
                 av_freep(&filename);
             }