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

[Paul B Mahol]

On 4/7/20, Anton Khirnov <anton at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".