diff mbox

[FFmpeg-devel] lavf/oggparseflac: Free flac extradata before reallocating.

Message ID CAADho6PNbbtnPC-ETK75QjKt6wuA83ae=7z1hgj2dy=x4Jj5CQ@mail.gmail.com
State New
Headers show

Commit Message

Matthew Wolenetz March 5, 2018, 9:54 p.m. UTC

Comments

James Almer March 5, 2018, 10:10 p.m. UTC | #1 
On 3/5/2018 6:54 PM, Matthew Wolenetz wrote:
> 
> 0001-lavf-oggparseflac-Free-flac-extradata-before-realloc.patch
> 
> 
> From 5d28b92d9d164b104e9a47b8183cd7ddedfde366 Mon Sep 17 00:00:00 2001
> From: Matt Wolenetz <wolenetz@chromium.org>
> Date: Mon, 5 Mar 2018 12:36:28 -0800
> Subject: [PATCH] lavf/oggparseflac: Free flac extradata before reallocating.
> 
> Otherwise ff_alloc_extradata() just leaks any existing allocated
> memory.

Maybe ff_alloc_extradata() is the one that should free any existing
extradata instead of littering the tree with av_freep() calls before
every ff_alloc_extradata() call. Otherwise you'll keep patching up
demuxers as your fuzzer generates files for them.

> 
> BUG=789835
> 
> Change-Id: I8e1c21a16749d28c7f050f5f5d8bffda3b419638
> Reviewed-on: https://chromium-review.googlesource.com/949415
> Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
> ---
>  libavformat/oggparseflac.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
> index b5f1416a3c..6cb3468022 100644
> --- a/libavformat/oggparseflac.c
> +++ b/libavformat/oggparseflac.c
> @@ -61,6 +61,7 @@ flac_header (AVFormatContext * s, int idx)
>          st->codecpar->codec_id = AV_CODEC_ID_FLAC;
>          st->need_parsing = AVSTREAM_PARSE_HEADERS;
>  
> +        av_freep(&st->codecpar->extradata);
>          if (ff_alloc_extradata(st->codecpar, FLAC_STREAMINFO_SIZE) < 0)
>              return AVERROR(ENOMEM);
>          memcpy(st->codecpar->extradata, streaminfo_start, st->codecpar->extradata_size);
> -- 2.16.2.395.g2e18187dfd-goog
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Matthew Wolenetz March 6, 2018, 1:04 a.m. UTC | #2 
Perhaps true, but there are a ton of paths to ff_alloc_extradata that are
not included in Chromium. Such a wider, general, fix seems more appropriate
to land upstream first with more testing.
For now, this is a particular known case that's occurring in Chromium that
needs fixing.

On Mon, Mar 5, 2018 at 2:10 PM, James Almer <jamrial@gmail.com> wrote:

> On 3/5/2018 6:54 PM, Matthew Wolenetz wrote:
> >
> > 0001-lavf-oggparseflac-Free-flac-extradata-before-realloc.patch
> >
> >
> > From 5d28b92d9d164b104e9a47b8183cd7ddedfde366 Mon Sep 17 00:00:00 2001
> > From: Matt Wolenetz <wolenetz@chromium.org>
> > Date: Mon, 5 Mar 2018 12:36:28 -0800
> > Subject: [PATCH] lavf/oggparseflac: Free flac extradata before
> reallocating.
> >
> > Otherwise ff_alloc_extradata() just leaks any existing allocated
> > memory.
>
> Maybe ff_alloc_extradata() is the one that should free any existing
> extradata instead of littering the tree with av_freep() calls before
> every ff_alloc_extradata() call. Otherwise you'll keep patching up
> demuxers as your fuzzer generates files for them.
>
> >
> > BUG=789835
> >
> > Change-Id: I8e1c21a16749d28c7f050f5f5d8bffda3b419638
> > Reviewed-on: https://chromium-review.googlesource.com/949415
> > Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
> > ---
> >  libavformat/oggparseflac.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
> > index b5f1416a3c..6cb3468022 100644
> > --- a/libavformat/oggparseflac.c
> > +++ b/libavformat/oggparseflac.c
> > @@ -61,6 +61,7 @@ flac_header (AVFormatContext * s, int idx)
> >          st->codecpar->codec_id = AV_CODEC_ID_FLAC;
> >          st->need_parsing = AVSTREAM_PARSE_HEADERS;
> >
> > +        av_freep(&st->codecpar->extradata);
> >          if (ff_alloc_extradata(st->codecpar, FLAC_STREAMINFO_SIZE) < 0)
> >              return AVERROR(ENOMEM);
> >          memcpy(st->codecpar->extradata, streaminfo_start,
> st->codecpar->extradata_size);
> > -- 2.16.2.395.g2e18187dfd-goog
> >
> >
> >
> > _______________________________________________
> > 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
>
James Almer March 6, 2018, 2:23 p.m. UTC | #3 
On 3/5/2018 10:04 PM, Matthew Wolenetz wrote:
> Perhaps true, but there are a ton of paths to ff_alloc_extradata that are
> not included in Chromium. Such a wider, general, fix seems more appropriate
> to land upstream first with more testing.

The function allocates a buffer and overwrites the pointer without
bothering to make sure it was NULL beforehand, so it's a latent source
of leaks if given improper usage.

I've sent a patchset to make it do a proper cleaning before allocating
new extradata, to see what other developers think.

> For now, this is a particular known case that's occurring in Chromium that
> needs fixing.
> 
> On Mon, Mar 5, 2018 at 2:10 PM, James Almer <jamrial@gmail.com> wrote:
> 
>> On 3/5/2018 6:54 PM, Matthew Wolenetz wrote:
>>>
>>> 0001-lavf-oggparseflac-Free-flac-extradata-before-realloc.patch
>>>
>>>
>>> From 5d28b92d9d164b104e9a47b8183cd7ddedfde366 Mon Sep 17 00:00:00 2001
>>> From: Matt Wolenetz <wolenetz@chromium.org>
>>> Date: Mon, 5 Mar 2018 12:36:28 -0800
>>> Subject: [PATCH] lavf/oggparseflac: Free flac extradata before
>> reallocating.
>>>
>>> Otherwise ff_alloc_extradata() just leaks any existing allocated
>>> memory.
>>
>> Maybe ff_alloc_extradata() is the one that should free any existing
>> extradata instead of littering the tree with av_freep() calls before
>> every ff_alloc_extradata() call. Otherwise you'll keep patching up
>> demuxers as your fuzzer generates files for them.
>>
>>>
>>> BUG=789835
>>>
>>> Change-Id: I8e1c21a16749d28c7f050f5f5d8bffda3b419638
>>> Reviewed-on: https://chromium-review.googlesource.com/949415
>>> Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
>>> ---
>>>  libavformat/oggparseflac.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
>>> index b5f1416a3c..6cb3468022 100644
>>> --- a/libavformat/oggparseflac.c
>>> +++ b/libavformat/oggparseflac.c
>>> @@ -61,6 +61,7 @@ flac_header (AVFormatContext * s, int idx)
>>>          st->codecpar->codec_id = AV_CODEC_ID_FLAC;
>>>          st->need_parsing = AVSTREAM_PARSE_HEADERS;
>>>
>>> +        av_freep(&st->codecpar->extradata);
>>>          if (ff_alloc_extradata(st->codecpar, FLAC_STREAMINFO_SIZE) < 0)
>>>              return AVERROR(ENOMEM);
>>>          memcpy(st->codecpar->extradata, streaminfo_start,
>> st->codecpar->extradata_size);
>>> -- 2.16.2.395.g2e18187dfd-goog
>>>
>>>
>>>
>>> _______________________________________________
>>> 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
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Matthew Wolenetz March 6, 2018, 5:36 p.m. UTC | #4 
SGTM. Thanks James.

On Tue, Mar 6, 2018 at 6:23 AM, James Almer <jamrial@gmail.com> wrote:

> On 3/5/2018 10:04 PM, Matthew Wolenetz wrote:
> > Perhaps true, but there are a ton of paths to ff_alloc_extradata that are
> > not included in Chromium. Such a wider, general, fix seems more
> appropriate
> > to land upstream first with more testing.
>
> The function allocates a buffer and overwrites the pointer without
> bothering to make sure it was NULL beforehand, so it's a latent source
> of leaks if given improper usage.
>
> I've sent a patchset to make it do a proper cleaning before allocating
> new extradata, to see what other developers think.
>
> > For now, this is a particular known case that's occurring in Chromium
> that
> > needs fixing.
> >
> > On Mon, Mar 5, 2018 at 2:10 PM, James Almer <jamrial@gmail.com> wrote:
> >
> >> On 3/5/2018 6:54 PM, Matthew Wolenetz wrote:
> >>>
> >>> 0001-lavf-oggparseflac-Free-flac-extradata-before-realloc.patch
> >>>
> >>>
> >>> From 5d28b92d9d164b104e9a47b8183cd7ddedfde366 Mon Sep 17 00:00:00 2001
> >>> From: Matt Wolenetz <wolenetz@chromium.org>
> >>> Date: Mon, 5 Mar 2018 12:36:28 -0800
> >>> Subject: [PATCH] lavf/oggparseflac: Free flac extradata before
> >> reallocating.
> >>>
> >>> Otherwise ff_alloc_extradata() just leaks any existing allocated
> >>> memory.
> >>
> >> Maybe ff_alloc_extradata() is the one that should free any existing
> >> extradata instead of littering the tree with av_freep() calls before
> >> every ff_alloc_extradata() call. Otherwise you'll keep patching up
> >> demuxers as your fuzzer generates files for them.
> >>
> >>>
> >>> BUG=789835
> >>>
> >>> Change-Id: I8e1c21a16749d28c7f050f5f5d8bffda3b419638
> >>> Reviewed-on: https://chromium-review.googlesource.com/949415
> >>> Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
> >>> ---
> >>>  libavformat/oggparseflac.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
> >>> index b5f1416a3c..6cb3468022 100644
> >>> --- a/libavformat/oggparseflac.c
> >>> +++ b/libavformat/oggparseflac.c
> >>> @@ -61,6 +61,7 @@ flac_header (AVFormatContext * s, int idx)
> >>>          st->codecpar->codec_id = AV_CODEC_ID_FLAC;
> >>>          st->need_parsing = AVSTREAM_PARSE_HEADERS;
> >>>
> >>> +        av_freep(&st->codecpar->extradata);
> >>>          if (ff_alloc_extradata(st->codecpar, FLAC_STREAMINFO_SIZE) <
> 0)
> >>>              return AVERROR(ENOMEM);
> >>>          memcpy(st->codecpar->extradata, streaminfo_start,
> >> st->codecpar->extradata_size);
> >>> -- 2.16.2.395.g2e18187dfd-goog
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> 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
> >>
> > _______________________________________________
> > 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
>
diff mbox

Patch

From 5d28b92d9d164b104e9a47b8183cd7ddedfde366 Mon Sep 17 00:00:00 2001
From: Matt Wolenetz <wolenetz@chromium.org>
Date: Mon, 5 Mar 2018 12:36:28 -0800
Subject: [PATCH] lavf/oggparseflac: Free flac extradata before reallocating.

Otherwise ff_alloc_extradata() just leaks any existing allocated
memory.

BUG=789835

Change-Id: I8e1c21a16749d28c7f050f5f5d8bffda3b419638
Reviewed-on: https://chromium-review.googlesource.com/949415
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
---
 libavformat/oggparseflac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
index b5f1416a3c..6cb3468022 100644
--- a/libavformat/oggparseflac.c
+++ b/libavformat/oggparseflac.c
@@ -61,6 +61,7 @@  flac_header (AVFormatContext * s, int idx)
         st->codecpar->codec_id = AV_CODEC_ID_FLAC;
         st->need_parsing = AVSTREAM_PARSE_HEADERS;
 
+        av_freep(&st->codecpar->extradata);
         if (ff_alloc_extradata(st->codecpar, FLAC_STREAMINFO_SIZE) < 0)
             return AVERROR(ENOMEM);
         memcpy(st->codecpar->extradata, streaminfo_start, st->codecpar->extradata_size);
-- 
2.16.2.395.g2e18187dfd-goog