[FFmpeg-devel] [PATCH] ffmdec: sanitize codec parameters

[Andreas Cadhalpun]

On 22.11.2016 01:27, Carl Eugen Hoyos wrote:
> 2016-11-22 0:40 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun at googlemail.com>:
> 
>> For example what should be done about overflows, e.g. when
>> calculating the bit rate? Does this count as demuxer failing?
> 
> I don't understand this question:
> There are formats for which we don't know the specification (or
> it may not exist): Of course we always want to fix all undefined
> behaviour, all crashes and similar - this is not related to the
> specifications in question.

The fundamental problem is that multiplying two int32_t variables
does not always fit in an int32_t. If the result of the multiplication
is too large, it wraps around into a negative value, that can cause
problems later on.
In a way this is similar to reading a negative value from a file,
and could be handled with a warning by default, however it could
also indicate a limitation in our framework and just setting the
value to 0 in case of overflow could cause the file to be decoded
incorrectly. Thus it might be better to always error out in such
cases.

> FFmpeg should never by default refuse to decode media files
> that can be decoded and it should never stop reading such files.

'Never' is certainly not correct, since e.g. the ffm muxer is a
special case, as you agreed. Also there are currently many cases
in which FFmpeg errors out, even though it could try to ignore
the problem.

>> And what should be done if the spec says a field is unsigned,
>> but our framework only supports the signed variant?
> 
> Is there a sample for which this makes a difference? If yes, we
> should try to fix it.

There are certainly fuzzed samples, where this is an issue.
How do you propose to fix this?

Best regards,
Andreas