[FFmpeg-devel] 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.

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

> From 29d36664c55b3a7078ebe57f8642e1d7dc389f16 Mon Sep 17 00:00:00 2001
> From: Zalewa <zalewapl@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);
>          }
>

Hello,

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

ffserver has been serving our purpose in a 24/7 fashion for about 7 
years now. It will probably be serving us for the foreseeable future, 
whatever the community decides. I prefer to share any fix I make back 
with the community because allowing everyone to build on top of better 
software pays off. If the community drops the program, we most likely 
won't, even if it means being stuck with a single version for years to 
come. However, we still need to get it as stable as possible.

I also admit that I'm not versed in the codebase and whatever change I 
made was based purely on valgrind's output. I understand that my patch 
in its current state is not ideal and I probably should have attempted a 
cleaner fix before submitting. I'll try to do that.

Having stated this, let's get back to the technical topic:

> This accesses plenty of internal fields. (Maybe did so before, but no
> matter.)
>
> So doing the freeing correctly (if even possible) would be better for
> the survival of ffserver.

At first I was hoping that avformat_free_context() will do the job 
nicely for me, but unfortunately there are some shared objects that get 
destroyed in this call and ffserver eventually crashes. If you run such 
ffserver build in valgrind, it will warn about "free'd memory" accesses.

The problem seems to stem from the "COPY" done in unlayer_stream(). I 
will try to reverse this "COPY" before avformat_free_context() and see 
if it helps.

Best regards,
Z.

On Sat, 15 Apr 2017 09:47:58 +0200
Zalewa PL <zalewapl@gmail.com> wrote:

> Hello,
> 
> > The plan is that if ffserver is not fixed before the next bump, it will
> > be removed.  
> 
> ffserver has been serving our purpose in a 24/7 fashion for about 7 
> years now. It will probably be serving us for the foreseeable future, 
> whatever the community decides. I prefer to share any fix I make back 
> with the community because allowing everyone to build on top of better 
> software pays off. If the community drops the program, we most likely 
> won't, even if it means being stuck with a single version for years to 
> come. However, we still need to get it as stable as possible.

If your company thinks ffserver is so important, you should probably
invest work into it to fix it.

> I also admit that I'm not versed in the codebase and whatever change I 
> made was based purely on valgrind's output. I understand that my patch 
> in its current state is not ideal and I probably should have attempted a 
> cleaner fix before submitting. I'll try to do that.
> 
> Having stated this, let's get back to the technical topic:
> 
> > This accesses plenty of internal fields. (Maybe did so before, but no
> > matter.)
> >
> > So doing the freeing correctly (if even possible) would be better for
> > the survival of ffserver.  
> 
> At first I was hoping that avformat_free_context() will do the job 
> nicely for me, but unfortunately there are some shared objects that get 
> destroyed in this call and ffserver eventually crashes. If you run such 
> ffserver build in valgrind, it will warn about "free'd memory" accesses.

Which shared objects?

> The problem seems to stem from the "COPY" done in unlayer_stream(). I 
> will try to reverse this "COPY" before avformat_free_context() and see 
> if it helps.
> 
> Best regards,
> Z.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel