[FFmpeg-devel] [PATCH] Avoid suppressing parse errors in matroska

[Reimar Döffinger]

On Sun, Feb 27, 2011 at 11:53:40AM -0800, Ami Fischman wrote:
> > FFmpeg error handling sucks, there's only the option of failing completely
> > (which often enough is really not reasonable) or given no indication of
> > error at all, at most a error message.
> > The primary guiding rule in either way is though: get as much possibly valid data
> > out of the file as possible.
> > Fail if there is nothing useful you can do with the data you get out of the file.
> > Print error/warning messages if something is wrong (and this really should be
> > complemented with extra error flags - though even more for decoders so that applications
> > can decide when and at what kind of curruption they do or do not want to display a frame.
> 
> Interesting.  The goal of extracting as much valid data as
> possible from an input file (as opposed to failing fast at the
> first sign of corruption) means that any data corruption that
> *is* encountered but not treated as fatal must be corrected for
> in the internal data structures.

Ignoring anything invalid often enough works just fine.
Either way incoming data must be validated, the issue is usually
missing validation, what to do if it fails is rather the smaller
issue in comparison.
There is also the issue that failing early makes it very difficult
to extensively test via fuzzing.

> The reason I started my
> investigation of this issue is that this kind of corruption (at
> least) is not covered up.  For example, in ffmpeg-mt, ffmpeg dies
> on the file in question with an FPE due to a division by 0:

Yes, and that issue has been fixed. I am almost certain that your
patch would only have hidden it and it's possible to craft a file
that will _still_ crash if only your patch is applied.
In which case your patch would have hidden the bug from view while
it would still be possible to use it in a targeted attack
(please note that I am exaggerating things to explain
my point, things aren't as black-and-white).

> Given my (very limited) understanding of the architecure of the
> code it's unclear to me how the goal you stated can be achieved.
> In particular when parsed structures are memset to 0 as
> initialization, and parsing returns early (but silently) when
> unexpected values are encountered, it seems like there must be
> many ways for decoding paths to end up being fed (invalid) zeros
> for fields that the parser never got far enough to parse.

Fields mustn't be (left) initialized to invalid values.
And that wasn't the issue here either, the timebase is initialized
to 1/90000 so there would be no issue if it is never set explicitly.
The problem is that the demuxer overwrote a valid value with an
invalid one from the file.
So in either case a check was missing, the question is between failing
completely or just printing a warning and _not_ using the invalid value.
The later one was done.

> > I am sure that libavformat can be more helpful to these applications,
> > but just failing is not a soution to playback issues.
> 
> The opposite POV is that failing where an app can check for
> it (i.e. via error return code) is preferable to failing
> unexpectedly (i.e. via SIGFPE).  It'd be nice to avoid the
> SIGFPE, whatever the mechanism :)

That has been done. Crashes will be fixed.