[FFmpeg-devel] [PATCH] avcodec: validate codec parameters in avcodec_parameters_to_context

[Hendrik Leppkes]

On Tue, Oct 25, 2016 at 2:39 PM, wm4 <nfxjfg at googlemail.com> wrote:
> On Tue, 25 Oct 2016 09:47:29 +0200
> Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>
>> On Tue, Oct 25, 2016 at 1:50 AM, Andreas Cadhalpun
>> <andreas.cadhalpun at googlemail.com> wrote:
>> > This should reduce the impact of a demuxer (or API user) setting bogus
>> > codec parameters.
>> >
>> >
>>
>> This seems rather noisy and doesn't really solve anything, does it?
>> Decoders still need to validate values instead of blindly trusting
>> them, and this just hides some problems in these decoders, instead of
>> fixing them there. API users of avcodec would not fill
>> AVCodecParameters, they would fill a codec context directly.
>
> You could also argue that the demuxer shouldn't return invalid
> parameters either.

It should not, but this patch does not address this.

There is various combinations of component usage that are possible,
and in fact are used in the real world:

avformat -> avcodec
other demuxer -> avcodec
avformat -> other decoder

This patch only addresses the first case, and only if you actually use
this function (which I for example do not, since I have an abstraction
layer in between, so I never have AVCodecParameters and AVCodecContext
in the same function).
So in short, it just doesn't fix much, and you can still get invalid
output from avformat, and potentially still undefined behavior in
avcodec if its fed those values through other means.

>
> How about this: always convert the params to a temporary codecpar, and
> provide a function to determine the validity of a codecpar. This way
> the check could be done in multiple places without duplicating the code
> needed for it.

That still sounds odd, although slightly better. At the very least it
should be a dedicated function that checks the values in a key place,
say you want to check params that are fed to a decoder, then call this
check in avcodec_open, because thats something everyone has to call to
use avcodec.

- Hendrik