[FFmpeg-devel] [PATCH] ffserver: fix memory leaks pointed out by valgrind.

[wm4]

On Fri, 14 Apr 2017 15:00:05 +0200
Zalewa <zalewapl at gmail.com> wrote:

> From 29d36664c55b3a7078ebe57f8642e1d7dc389f16 Mon Sep 17 00:00:00 2001
> From: Zalewa <zalewapl at gmail.com>
> Date: Fri, 14 Apr 2017 09:26:18 +0200
> Subject: [PATCH] ffserver: fix memory leaks pointed out by valgrind.
> 
> Many memory leaks were created upon HTTP client disconnect.
> Many clients connecting & disconnecting rapidly could very
> quickly create leaks going into Gigabytes of memory.
> ---
>  ffserver.c | 46 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/ffserver.c b/ffserver.c
> index 8b819b6..416438d 100644
> --- a/ffserver.c
> +++ b/ffserver.c
> @@ -237,6 +237,7 @@ static int rtp_new_av_stream(HTTPContext *c,
>  static size_t htmlencode (const char *src, char **dest);
>  static inline void cp_html_entity (char *buffer, const char *entity);
>  static inline int check_codec_match(LayeredAVStream *ccf, AVStream *ccs, int stream);
> +static void close_format_context(AVFormatContext *ctx);
>  
>  static const char *my_program_name;
>  
> @@ -936,9 +937,7 @@ static void close_connection(HTTPContext *c)
>          ctx = c->rtp_ctx[i];
>          if (ctx) {
>              av_write_trailer(ctx);
> -            av_dict_free(&ctx->metadata);
> -            av_freep(&ctx->streams[0]);
> -            av_freep(&ctx);
> +            avformat_free_context(ctx);
>          }
>          ffurl_close(c->rtp_handles[i]);
>      }
> @@ -954,11 +953,10 @@ static void close_connection(HTTPContext *c)
>                  avio_close_dyn_buf(ctx->pb, &c->pb_buffer);
>              }
>          }
> -        for(i=0; i<ctx->nb_streams; i++)
> -            av_freep(&ctx->streams[i]);
> -        av_freep(&ctx->streams);
> -        av_freep(&ctx->priv_data);
> -        }
> +        close_format_context(ctx);
> +        av_freep(&ctx->internal);
> +        av_freep(&c->pfmt_ctx);
> +    }
>  
>      if (c->stream && !c->post && c->stream->stream_type == STREAM_TYPE_LIVE)
>          current_bandwidth -= c->stream->bandwidth;
> @@ -3724,6 +3722,32 @@ int check_codec_match(LayeredAVStream *ccf, AVStream *ccs, int stream)
>      return matches;
>  }
>  
> +static void close_format_context(AVFormatContext *ctx)
> +{
> +    int i = 0;
> +
> +    if (ctx->oformat && ctx->oformat->deinit)
> +        ctx->oformat->deinit(ctx);
> +    for (i=0; i<ctx->nb_streams; i++) {
> +        if (ctx->streams[i]->internal) {
> +            avcodec_free_context(&ctx->streams[i]->internal->avctx);
> +        }
> +        av_freep(&ctx->streams[i]->info);
> +        av_freep(&ctx->streams[i]->priv_data);
> +        av_freep(&ctx->streams[i]->priv_pts);
> +        av_freep(&ctx->streams[i]->internal);
> +        av_freep(&ctx->streams[i]);
> +    }
> +    av_opt_free(ctx);
> +    if (ctx->iformat && ctx->iformat->priv_class && ctx->priv_data)
> +        av_opt_free(ctx->priv_data);
> +    if (ctx->oformat && ctx->oformat->priv_class && ctx->priv_data)
> +        av_opt_free(ctx->priv_data);
> +    av_freep(&ctx->streams);
> +    ctx->nb_streams = 0;
> +    av_freep(&ctx->priv_data);
> +}
> +

This accesses plenty of internal fields. (Maybe did so before, but no
matter.)

The plan is that if ffserver is not fixed before the next bump, it will
be removed.

So doing the freeing correctly (if even possible) would be better for
the survival of ffserver.

>  /* compute the needed AVStream for each feed */
>  static int build_feed_streams(void)
>  {
> @@ -3836,7 +3860,7 @@ drop:
>              }
>              s->oformat = feed->fmt;
>              for (i = 0; i<feed->nb_streams; i++) {
> -                AVStream *st = avformat_new_stream(s, NULL); // FIXME free this
> +                AVStream *st = avformat_new_stream(s, NULL);
>                  if (!st) {
>                      http_log("Failed to allocate stream\n");
>                      goto bail;
> @@ -3852,10 +3876,8 @@ drop:
>                  goto bail;
>              }
>              /* XXX: need better API */
> -            av_freep(&s->priv_data);
> +            close_format_context(s);
>              avio_closep(&s->pb);
> -            s->streams = NULL;
> -            s->nb_streams = 0;
>              avformat_free_context(s);
>          }
>