| Message ID | 20200404223842.29972-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/oggparsevorbis: Update context on double init | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Quoting Michael Niedermayer (2020-04-05 00:38:41)
> Fixes: memleak
Memleak of what/where/why? This is highly non-obvious.
On Mon, Apr 06, 2020 at 12:00:21PM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2020-04-05 00:38:41) > > Fixes: memleak > > Memleak of what/where/why? This is highly non-obvious. yes, i tend to be terse on "security" fixes so as not to provide a "how to exploit" what leaks is the AVVorbisParseContext it leaks as there is no check for it being already allocated. I attempted to add such a check but that was rejected by paul with no further comment. See: 0113 10:59 To FFmpeg devel (1,4K) [FFmpeg-devel] [PATCH] avformat/oggparsevorbis: Error out on double init of vp This patch works around that by preventing the demuxer allocated extradata from being replaced by the NULL extradata from the decoder As there is a check for double allocating the extradata that will protect also from AVVorbisParseContext double allocation that said, the whole back and forth copying of parameters we have in libavformat now a days is not pretty and every time i look at it it feels fragile. Iam a bit surprised this doesnt cause more problems There are of course other ways to fix this, i did tend towards a simple (hopefully) easy to backport fix What do you prefer ? Thanks [...]
On 4/6/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Apr 06, 2020 at 12:00:21PM +0200, Anton Khirnov wrote: >> Quoting Michael Niedermayer (2020-04-05 00:38:41) >> > Fixes: memleak >> >> Memleak of what/where/why? This is highly non-obvious. > > yes, i tend to be terse on "security" fixes so as not to provide a > "how to exploit" > > what leaks is the AVVorbisParseContext it leaks as there is no check for it > being already allocated. > I attempted to add such a check but that was rejected by paul with no > further > comment. > See: 0113 10:59 To FFmpeg devel (1,4K) [FFmpeg-devel] [PATCH] > avformat/oggparsevorbis: Error out on double init of vp > > This patch works around that by preventing the demuxer allocated extradata > from being replaced by the NULL extradata from the decoder > As there is a check for double allocating the extradata that will protect > also from AVVorbisParseContext double allocation > > that said, the whole back and forth copying of parameters we have in > libavformat now a days is not pretty and every time i look at it it > feels fragile. Iam a bit surprised this doesnt cause more problems > > There are of course other ways to fix this, i did tend towards a > simple (hopefully) easy to backport fix > > What do you prefer ? I rejected patch, because Lynee reported over IRC, which you thankfully completely ignored, bug that stops playing files. > > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Does the universe only have a finite lifespan? No, its going to go on > forever, its just that you wont like living in it. -- Hiranya Peiri >
Paul B Mahol (12020-04-07): > I rejected patch, because Lynee reported over IRC, which you > thankfully completely ignored, bug that stops playing files. Next time, save everybody some time and say it directly there. Politeness is not for everybody else but you.
Quoting Michael Niedermayer (2020-04-06 15:15:17) > On Mon, Apr 06, 2020 at 12:00:21PM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2020-04-05 00:38:41) > > > Fixes: memleak > > > > Memleak of what/where/why? This is highly non-obvious. > > yes, i tend to be terse on "security" fixes so as not to provide a > "how to exploit" I do not think that is a good approach from long-term perspective - someone reading that code in the future will not be able to tell what it fixes from git blame (and you might not remember yourself), and so might reintroduce the same bug. > > what leaks is the AVVorbisParseContext it leaks as there is no check for it > being already allocated. > I attempted to add such a check but that was rejected by paul with no further > comment. > See: 0113 10:59 To FFmpeg devel (1,4K) [FFmpeg-devel] [PATCH] avformat/oggparsevorbis: Error out on double init of vp I believe such non-constructive information-free comments should be - disregarded - treated as a breach of the code of conduct, once we have an enforceable one (which is hopefully soon) > > This patch works around that by preventing the demuxer allocated extradata > from being replaced by the NULL extradata from the decoder > As there is a check for double allocating the extradata that will protect > also from AVVorbisParseContext double allocation > > that said, the whole back and forth copying of parameters we have in > libavformat now a days is not pretty and every time i look at it it > feels fragile. Iam a bit surprised this doesnt cause more problems > > There are of course other ways to fix this, i did tend towards a > simple (hopefully) easy to backport fix > > What do you prefer ? Seems to me your original patch was preferable, though I'd put the check somewhere around the beginning of vorbis_header(). I suppose that doesn't matter much though.
On 4/7/20, Anton Khirnov <anton@khirnov.net> wrote: > Quoting Michael Niedermayer (2020-04-06 15:15:17) >> On Mon, Apr 06, 2020 at 12:00:21PM +0200, Anton Khirnov wrote: >> > Quoting Michael Niedermayer (2020-04-05 00:38:41) >> > > Fixes: memleak >> > >> > Memleak of what/where/why? This is highly non-obvious. >> >> yes, i tend to be terse on "security" fixes so as not to provide a >> "how to exploit" > > I do not think that is a good approach from long-term perspective - > someone reading that code in the future will not be able to tell what it > fixes from git blame (and you might not remember yourself), and so might > reintroduce the same bug. > >> >> what leaks is the AVVorbisParseContext it leaks as there is no check for >> it >> being already allocated. >> I attempted to add such a check but that was rejected by paul with no >> further >> comment. >> See: 0113 10:59 To FFmpeg devel (1,4K) [FFmpeg-devel] [PATCH] >> avformat/oggparsevorbis: Error out on double init of vp > > I believe such non-constructive information-free comments should be > - disregarded > - treated as a breach of the code of conduct, once we have an > enforceable one (which is hopefully soon) > NAK >> >> This patch works around that by preventing the demuxer allocated extradata >> from being replaced by the NULL extradata from the decoder >> As there is a check for double allocating the extradata that will protect >> also from AVVorbisParseContext double allocation >> >> that said, the whole back and forth copying of parameters we have in >> libavformat now a days is not pretty and every time i look at it it >> feels fragile. Iam a bit surprised this doesnt cause more problems >> >> There are of course other ways to fix this, i did tend towards a >> simple (hopefully) easy to backport fix >> >> What do you prefer ? > > Seems to me your original patch was preferable, though I'd put the check > somewhere around the beginning of vorbis_header(). I suppose that > doesn't matter much though. > > -- > Anton Khirnov > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Tue, Apr 07, 2020 at 10:55:39AM +0200, Paul B Mahol wrote: > On 4/6/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Mon, Apr 06, 2020 at 12:00:21PM +0200, Anton Khirnov wrote: > >> Quoting Michael Niedermayer (2020-04-05 00:38:41) > >> > Fixes: memleak > >> > >> Memleak of what/where/why? This is highly non-obvious. > > > > yes, i tend to be terse on "security" fixes so as not to provide a > > "how to exploit" > > > > what leaks is the AVVorbisParseContext it leaks as there is no check for it > > being already allocated. > > I attempted to add such a check but that was rejected by paul with no > > further > > comment. > > See: 0113 10:59 To FFmpeg devel (1,4K) [FFmpeg-devel] [PATCH] > > avformat/oggparsevorbis: Error out on double init of vp > > > > This patch works around that by preventing the demuxer allocated extradata > > from being replaced by the NULL extradata from the decoder > > As there is a check for double allocating the extradata that will protect > > also from AVVorbisParseContext double allocation > > > > that said, the whole back and forth copying of parameters we have in > > libavformat now a days is not pretty and every time i look at it it > > feels fragile. Iam a bit surprised this doesnt cause more problems > > > > There are of course other ways to fix this, i did tend towards a > > simple (hopefully) easy to backport fix > > > > What do you prefer ? > > I rejected patch, because Lynee reported over IRC, which you > thankfully completely ignored, bug that stops playing files. i must have forgotten. I dont remember Can you or someone tell me what bug this is about or which ticket this is about if theres a ticket? Thanks [...]
Am Di., 7. Apr. 2020 um 15:38 Uhr schrieb Anton Khirnov <anton@khirnov.net>: > I believe such non-constructive information-free comments should be > - disregarded > - treated as a breach of the code of conduct, once we have an > enforceable one (which is hopefully soon) This is not he first time that you send an absolutely unacceptable message to this mailing list including threatening other developers. This really has to stop. Carl Eugen
diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c index 8dd27e7770..d5784a5f5e 100644 --- a/libavformat/oggparsevorbis.c +++ b/libavformat/oggparsevorbis.c @@ -398,6 +398,7 @@ static int vorbis_header(AVFormatContext *s, int idx) st->codecpar->extradata_size = 0; return AVERROR_UNKNOWN; } + st->internal->need_context_update = 1; } return 1;
Fixes: memleak Fixes: 19949/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5743636058210304 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/oggparsevorbis.c | 1 + 1 file changed, 1 insertion(+)