[FFmpeg-devel] libavformat/http: Fix memory leak in get_cookies.

Submitted by rshaffer@tunein.com on April 13, 2018, 11:42 p.m.

Details

Message ID 20180413234232.15963-1-rshaffer@tunein.com
State Accepted
Commit 9a8811f478db4c24e9acb597274c5feace11f364
Headers show

Commit Message

rshaffer@tunein.com April 13, 2018, 11:42 p.m.
From: Richard Shaffer <rshaffer@tunein.com>

---
 libavformat/http.c | 1 +
 1 file changed, 1 insertion(+)

Comments

rshaffer@tunein.com April 17, 2018, 5:48 p.m.
On Fri, Apr 13, 2018 at 4:42 PM, <rshaffer@tunein.com> wrote:

> From: Richard Shaffer <rshaffer@tunein.com>
>
> ---
>  libavformat/http.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 983034f083..b4a1919f24 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -1103,6 +1103,7 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
>              snprintf(*cookies, str_size, "%s; %s=%s", tmp,
> cookie_entry->key, cookie_entry->value);
>              av_free(tmp);
>          }
> +        av_dict_free(&cookie_params);
>      }
>
>      av_free(set_cookies);
> --
> 2.15.1 (Apple Git-101)
>
> Would one of the maintainers mind looking at and applying this? It's a
one-line change to fix a memory leak.

Thanks,

-Richard
wm4 April 17, 2018, 8:04 p.m.
On Tue, 17 Apr 2018 10:48:16 -0700
Richard Shaffer <rshaffer@tunein.com> wrote:

> On Fri, Apr 13, 2018 at 4:42 PM, <rshaffer@tunein.com> wrote:
> 
> > From: Richard Shaffer <rshaffer@tunein.com>
> >
> > ---
> >  libavformat/http.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/http.c b/libavformat/http.c
> > index 983034f083..b4a1919f24 100644
> > --- a/libavformat/http.c
> > +++ b/libavformat/http.c
> > @@ -1103,6 +1103,7 @@ static int get_cookies(HTTPContext *s, char
> > **cookies, const char *path,
> >              snprintf(*cookies, str_size, "%s; %s=%s", tmp,
> > cookie_entry->key, cookie_entry->value);
> >              av_free(tmp);
> >          }
> > +        av_dict_free(&cookie_params);
> >      }
> >
> >      av_free(set_cookies);
> > --
> > 2.15.1 (Apple Git-101)
> >
> > Would one of the maintainers mind looking at and applying this? It's a  
> one-line change to fix a memory leak.

Not a maintainer, but the "official" maintainer hasn't cared about this
code for a long time (despite being active FFmpeg contributor). Pushed.

I think there are some more leaks of this allocation, though.
rshaffer@tunein.com April 17, 2018, 8:29 p.m.
On Tue, Apr 17, 2018 at 1:04 PM, wm4 <nfxjfg@googlemail.com> wrote:

> On Tue, 17 Apr 2018 10:48:16 -0700
> Richard Shaffer <rshaffer@tunein.com> wrote:
>
> > On Fri, Apr 13, 2018 at 4:42 PM, <rshaffer@tunein.com> wrote:
> >
> > > From: Richard Shaffer <rshaffer@tunein.com>
> > >
> > > ---
> > >  libavformat/http.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/libavformat/http.c b/libavformat/http.c
> > > index 983034f083..b4a1919f24 100644
> > > --- a/libavformat/http.c
> > > +++ b/libavformat/http.c
> > > @@ -1103,6 +1103,7 @@ static int get_cookies(HTTPContext *s, char
> > > **cookies, const char *path,
> > >              snprintf(*cookies, str_size, "%s; %s=%s", tmp,
> > > cookie_entry->key, cookie_entry->value);
> > >              av_free(tmp);
> > >          }
> > > +        av_dict_free(&cookie_params);
> > >      }
> > >
> > >      av_free(set_cookies);
> > > --
> > > 2.15.1 (Apple Git-101)
> > >
> > > Would one of the maintainers mind looking at and applying this? It's
> a
> > one-line change to fix a memory leak.
>
> Not a maintainer, but the "official" maintainer hasn't cared about this
> code for a long time (despite being active FFmpeg contributor). Pushed.
>
> I think there are some more leaks of this allocation, though.
>

Yeah. Darn. I missed see the break statements inside the nested ifs. Those
are a rare case, since they're only executed if an allocation fails, but
they should still be handled. I'll send another patch for that. Maybe I can
refactor the while loop so that there aren't so many breaks or continues. I
did go over the rest of that loop, and I don't see any other cases where
the dictionary isn't freed.

Thanks for committing anyway. At least it fixes the most common code path.


> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Patch hide | download patch | download mbox

diff --git a/libavformat/http.c b/libavformat/http.c
index 983034f083..b4a1919f24 100644
--- a/libavformat/http.c
+++ b/libavformat/http.c
@@ -1103,6 +1103,7 @@  static int get_cookies(HTTPContext *s, char **cookies, const char *path,
             snprintf(*cookies, str_size, "%s; %s=%s", tmp, cookie_entry->key, cookie_entry->value);
             av_free(tmp);
         }
+        av_dict_free(&cookie_params);
     }
 
     av_free(set_cookies);