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

Andreas Cadhalpun andreas.cadhalpun at googlemail.com
Wed Oct 26 02:59:59 EEST 2016


On 25.10.2016 14:56, Hendrik Leppkes wrote:
> 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

I've added a macro to make it less noisy.

>>> and doesn't really solve anything, does it?

As you analyze below, it was fixing only a part of the problem.

>>> Decoders still need to validate values instead of blindly trusting
>>> them, and this just hides some problems in these decoders,

Yes, the problem hiding is bad, which is why I added av_assert2's
so that developers can easily check if the validation fails.

>>> 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.

Indeed, the demuxers should be fixed in addition to this patch.

> 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.

For the third case, the demuxers have to be fixed. Having the asserts
in the central code makes it much easier to find these problems.

>> 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.

I tried to implement the suggestions of both of you, see attached patch.

Note that the added asserts are triggered by *many* of my fuzzed samples.
I'm happy to write patches for these cases, if we achieve agreement
that the central check alone is insufficient.
Another noteworthy point is that this patch makes it easy to trigger
the av_assert0 in iff_read_packet. I think that assert should be replaced
with an error return.

Best regards,
Andreas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avcodec-validate-codec-parameters.patch
Type: text/x-diff
Size: 6345 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161026/c0151e90/attachment.patch>


More information about the ffmpeg-devel mailing list