Message ID | CAADho6PNbbtnPC-ETK75QjKt6wuA83ae=7z1hgj2dy=x4Jj5CQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
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 >
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 >
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 >
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 >
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