diff mbox series

[FFmpeg-devel,1/2] avformat/oggparsevorbis: Update context on double init

Message ID 20200404223842.29972-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/2] avformat/oggparsevorbis: Update context on double init
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Michael Niedermayer April 4, 2020, 10:38 p.m. UTC
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(+)

Comments

Anton Khirnov April 6, 2020, 10 a.m. UTC | #1
Quoting Michael Niedermayer (2020-04-05 00:38:41)
> Fixes: memleak

Memleak of what/where/why? This is highly non-obvious.
Michael Niedermayer April 6, 2020, 1:15 p.m. UTC | #2
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

[...]
Paul B Mahol April 7, 2020, 8:55 a.m. UTC | #3
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
>
Nicolas George April 7, 2020, 10:02 a.m. UTC | #4
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.
Anton Khirnov April 7, 2020, 1:38 p.m. UTC | #5
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.
Paul B Mahol April 7, 2020, 2:20 p.m. UTC | #6
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".
Michael Niedermayer April 7, 2020, 11:04 p.m. UTC | #7
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

[...]
Carl Eugen Hoyos April 7, 2020, 11:20 p.m. UTC | #8
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 mbox series

Patch

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;