Message ID | CAK+gHTKnu7bXtSfgds8XCydArZ9z6ZihGrH5rkim=ZTntbK5ww@mail.gmail.com |
---|---|
State | Superseded |
Headers | show |
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
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); +} + /* 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); } -- 1.9.1