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

[Anton Khirnov]

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.

-- 
Anton Khirnov