[FFmpeg-devel] Fix leaked dictionary in mp3dec

Submitted by Thomas Guilbert on April 6, 2017, 9:07 p.m.

Details

Message ID CABrVPoY1qKCkM5MhormD=LTSRVjHYGGK8EgeRBpFx47RNj_Wtw@mail.gmail.com
State New
Headers show

Commit Message

Thomas Guilbert April 6, 2017, 9:07 p.m.
The patch didn't show up as properly formatted on
https://patchwork.ffmpeg.org/patch/3228/.

Re-submitting using no line wrap in the base64 attachment, and copying the
contents of the patch for ease of review:

From fced5ab0e09f529397adddcb560d1a08f2df4840 Mon Sep 17 00:00:00 2001
From: Thomas Guilbert <tguilbert@chromium.org>
Date: Thu, 30 Mar 2017 18:23:29 -0700
Subject: [PATCH] Fix dictionnary leak in mp3dec

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

Comments

wm4 April 7, 2017, 6:32 a.m.
On Thu, 6 Apr 2017 14:07:53 -0700
Thomas Guilbert <tguilbert@chromium.org> wrote:

> The patch didn't show up as properly formatted on
> https://patchwork.ffmpeg.org/patch/3228/.
> 
> Re-submitting using no line wrap in the base64 attachment, and copying the
> contents of the patch for ease of review:
> 
> From fced5ab0e09f529397adddcb560d1a08f2df4840 Mon Sep 17 00:00:00 2001
> From: Thomas Guilbert <tguilbert@chromium.org>
> Date: Thu, 30 Mar 2017 18:23:29 -0700
> Subject: [PATCH] Fix dictionnary leak in mp3dec
> 
> ---
>  libavformat/mp3dec.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 0924a57843..fd8184cc0b 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -349,6 +349,7 @@ static int mp3_read_header(AVFormatContext *s)
>      int ret;
>      int i;
> 
> +    av_dict_free(&s->metadata);
>      s->metadata = s->internal->id3v2_meta;
>      s->internal->id3v2_meta = NULL;
> 

So in which situations is s->metadata not NULL?
Thomas Guilbert April 7, 2017, 11:13 p.m.
In Chromium code, we set s->metadata ahead of time, with a custom entry to
avoid id3v1 tag parsing.

Some recent changes in mp3dec.c meant that we had to update this code, and
in the process, we discovered this reference leak on our end.

I am submitting this patch as a general code hardening patch. I also
understand if our use case is unusual and that one may assume that
s->metadata is always NULL.

On Thu, Apr 6, 2017 at 11:32 PM, wm4 <nfxjfg@googlemail.com> wrote:

> On Thu, 6 Apr 2017 14:07:53 -0700
> Thomas Guilbert <tguilbert@chromium.org> wrote:
>
> > The patch didn't show up as properly formatted on
> > https://patchwork.ffmpeg.org/patch/3228/.
> >
> > Re-submitting using no line wrap in the base64 attachment, and copying
> the
> > contents of the patch for ease of review:
> >
> > From fced5ab0e09f529397adddcb560d1a08f2df4840 Mon Sep 17 00:00:00 2001
> > From: Thomas Guilbert <tguilbert@chromium.org>
> > Date: Thu, 30 Mar 2017 18:23:29 -0700
> > Subject: [PATCH] Fix dictionnary leak in mp3dec
> >
> > ---
> >  libavformat/mp3dec.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 0924a57843..fd8184cc0b 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -349,6 +349,7 @@ static int mp3_read_header(AVFormatContext *s)
> >      int ret;
> >      int i;
> >
> > +    av_dict_free(&s->metadata);
> >      s->metadata = s->internal->id3v2_meta;
> >      s->internal->id3v2_meta = NULL;
> >
>
> So in which situations is s->metadata not NULL?
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Patch hide | download patch | download mbox

RnJvbSBmY2VkNWFiMGUwOWY1MjkzOTdhZGRkY2I1NjBkMWEwOGYyZGY0ODQwIE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBUaG9tYXMgR3VpbGJlcnQgPHRndWlsYmVydEBjaHJvbWl1bS5vcmc+CkRhdGU6IFRodSwgMzAgTWFyIDIwMTcgMTg6MjM6MjkgLTA3MDAKU3ViamVjdDogW1BBVENIXSBGaXggZGljdGlvbm5hcnkgbGVhayBpbiBtcDNkZWMKCi0tLQogbGliYXZmb3JtYXQvbXAzZGVjLmMgfCAxICsKIDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlvbigrKQoKZGlmZiAtLWdpdCBhL2xpYmF2Zm9ybWF0L21wM2RlYy5jIGIvbGliYXZmb3JtYXQvbXAzZGVjLmMKaW5kZXggMDkyNGE1Nzg0My4uZmQ4MTg0Y2MwYiAxMDA2NDQKLS0tIGEvbGliYXZmb3JtYXQvbXAzZGVjLmMKKysrIGIvbGliYXZmb3JtYXQvbXAzZGVjLmMKQEAgLTM0OSw2ICszNDksNyBAQCBzdGF0aWMgaW50IG1wM19yZWFkX2hlYWRlcihBVkZvcm1hdENvbnRleHQgKnMpCiAgICAgaW50IHJldDsKICAgICBpbnQgaTsKIAorICAgIGF2X2RpY3RfZnJlZSgmcy0+bWV0YWRhdGEpOwogICAgIHMtPm1ldGFkYXRhID0gcy0+aW50ZXJuYWwtPmlkM3YyX21ldGE7CiAgICAgcy0+aW50ZXJuYWwtPmlkM3YyX21ldGEgPSBOVUxMOwogCi0tIAoyLjEyLjIuNTY0LmcwNjNmZTg1OGI4LWdvb2cKCg==