Re: [FFmpeg-devel] Fix leaked dictionary in mp3dec

[Thomas Guilbert]

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 <nfx...@googlemail.com> wrote:

> On Thu, 6 Apr 2017 14:07:53 -0700
> Thomas Guilbert <tguilb...@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 <tguilb...@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(>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
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Fix leaked dictionary in mp3dec

[Thomas Guilbert]

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 <tguilb...@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(>metadata);
 s->metadata = s->internal->id3v2_meta;
 s->internal->id3v2_meta = NULL;

-- 
2.12.2.564.g063fe858b8-goog


On Fri, Mar 31, 2017 at 12:39 PM, Thomas Guilbert <tguilb...@chromium.org>
wrote:

> Commit '65862f57ad2f7f49d715f334a9d892e0b20d42f1' overwrites s->metada
> with s->internal->id3v2_meta, which leaks an AVDictionary* if
> s->metada was not null.
>
> Please excuse any formatting problems in this email, this is my first
> time uploading a patch :)
>
> Thank you,
> Thomas
>
RnJvbSBmY2VkNWFiMGUwOWY1MjkzOTdhZGRkY2I1NjBkMWEwOGYyZGY0ODQwIE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBUaG9tYXMgR3VpbGJlcnQgPHRndWlsYmVydEBjaHJvbWl1bS5vcmc+CkRhdGU6IFRodSwgMzAgTWFyIDIwMTcgMTg6MjM6MjkgLTA3MDAKU3ViamVjdDogW1BBVENIXSBGaXggZGljdGlvbm5hcnkgbGVhayBpbiBtcDNkZWMKCi0tLQogbGliYXZmb3JtYXQvbXAzZGVjLmMgfCAxICsKIDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlvbigrKQoKZGlmZiAtLWdpdCBhL2xpYmF2Zm9ybWF0L21wM2RlYy5jIGIvbGliYXZmb3JtYXQvbXAzZGVjLmMKaW5kZXggMDkyNGE1Nzg0My4uZmQ4MTg0Y2MwYiAxMDA2NDQKLS0tIGEvbGliYXZmb3JtYXQvbXAzZGVjLmMKKysrIGIvbGliYXZmb3JtYXQvbXAzZGVjLmMKQEAgLTM0OSw2ICszNDksNyBAQCBzdGF0aWMgaW50IG1wM19yZWFkX2hlYWRlcihBVkZvcm1hdENvbnRleHQgKnMpCiAgICAgaW50IHJldDsKICAgICBpbnQgaTsKIAorICAgIGF2X2RpY3RfZnJlZSgmcy0+bWV0YWRhdGEpOwogICAgIHMtPm1ldGFkYXRhID0gcy0+aW50ZXJuYWwtPmlkM3YyX21ldGE7CiAgICAgcy0+aW50ZXJuYWwtPmlkM3YyX21ldGEgPSBOVUxMOwogCi0tIAoyLjEyLjIuNTY0LmcwNjNmZTg1OGI4LWdvb2cKCg==
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] Fix leaked dictionary in mp3dec

[Thomas Guilbert]

Commit '65862f57ad2f7f49d715f334a9d892e0b20d42f1' overwrites s->metada
with s->internal->id3v2_meta, which leaks an AVDictionary* if
s->metada was not null.

Please excuse any formatting problems in this email, this is my first
time uploading a patch :)

Thank you,
Thomas
RnJvbSBmY2VkNWFiMGUwOWY1MjkzOTdhZGRkY2I1NjBkMWEwOGYyZGY0ODQwIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBUaG9tYXMgR3VpbGJlcnQgPHRndWlsYmVydEBjaHJvbWl1bS5v
cmc+CkRhdGU6IFRodSwgMzAgTWFyIDIwMTcgMTg6MjM6MjkgLTA3MDAKU3ViamVjdDogW1BBVENI
XSBGaXggZGljdGlvbm5hcnkgbGVhayBpbiBtcDNkZWMKCi0tLQogbGliYXZmb3JtYXQvbXAzZGVj
LmMgfCAxICsKIDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlvbigrKQoKZGlmZiAtLWdpdCBhL2xp
YmF2Zm9ybWF0L21wM2RlYy5jIGIvbGliYXZmb3JtYXQvbXAzZGVjLmMKaW5kZXggMDkyNGE1Nzg0
My4uZmQ4MTg0Y2MwYiAxMDA2NDQKLS0tIGEvbGliYXZmb3JtYXQvbXAzZGVjLmMKKysrIGIvbGli
YXZmb3JtYXQvbXAzZGVjLmMKQEAgLTM0OSw2ICszNDksNyBAQCBzdGF0aWMgaW50IG1wM19yZWFk
X2hlYWRlcihBVkZvcm1hdENvbnRleHQgKnMpCiAgICAgaW50IHJldDsKICAgICBpbnQgaTsKIAor
ICAgIGF2X2RpY3RfZnJlZSgmcy0+bWV0YWRhdGEpOwogICAgIHMtPm1ldGFkYXRhID0gcy0+aW50
ZXJuYWwtPmlkM3YyX21ldGE7CiAgICAgcy0+aW50ZXJuYWwtPmlkM3YyX21ldGEgPSBOVUxMOwog
Ci0tIAoyLjEyLjIuNTY0LmcwNjNmZTg1OGI4LWdvb2cKCg==
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/vp8: Fix hang with slice threads

[Thomas Guilbert]

Thank you!

On Thu, Mar 9, 2017 at 5:20 PM, Michael Niedermayer <mich...@niedermayer.cc>
wrote:

> On Thu, Mar 09, 2017 at 08:17:37PM -0500, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Thu, Mar 9, 2017 at 8:12 PM, Michael Niedermayer
> <mich...@niedermayer.cc>
> > wrote:
> >
> > > From: Thomas Guilbert <tguilb...@google.com>
> > >
> > > Fixes: 447860.webm
> > >
> > > Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc>
> > > ---
> > >  libavcodec/vp8.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
> > > index 6759b310f0..068223920e 100644
> > > --- a/libavcodec/vp8.c
> > > +++ b/libavcodec/vp8.c
> > > @@ -2508,8 +2508,10 @@ int vp78_decode_mb_row_sliced(AVCodecContext
> > > *avctx, void *tdata, int jobnr,
> > >  for (mb_y = jobnr; mb_y < s->mb_height; mb_y += num_jobs) {
> > >  td->thread_mb_pos = mb_y << 16;
> > >  ret = s->decode_mb_row_no_filter(avctx, tdata, jobnr,
> threadnr);
> > > -if (ret < 0)
> > > +if (ret < 0) {
> > > +update_pos(td, s->mb_height, INT_MAX & 0x);
> > >  return ret;
> > > +}
> > >  if (s->deblock_filter)
> > >  s->filter_mb_row(avctx, tdata, jobnr, threadnr);
> > >  update_pos(td, mb_y, INT_MAX & 0x);
> > > --
> > > 2.11.0
> >
> >
> > OK.
>
> applied
>
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Complexity theory is the science of finding the exact solution to an
> approximation. Benchmarking OTOH is finding an approximation of the exact
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel