[FFmpeg-devel] [PATCH] aacenc: Segmentation fault when frame contains samples with NaN values.

[Reimar Döffinger]

On Tue, Mar 11, 2014 at 09:39:11PM +0100, Michael Niedermayer wrote:
> On Tue, Mar 11, 2014 at 07:15:03PM +0100, Reimar Döffinger wrote:
> > I have no intention whatsoever to make the code "support" NaN,
> > I was trying to point out the specific places causing issues
> > in the hope that someone who understands well enough reviews it
> > for (as far as I can tell potentially exploitable) issues with
> > its floating-point handling.
> > We only know that it can crash for NaNs, but the "cost" failure
> > is complex enough that I am not at all sure it couldn't be triggered
> > by "normal" input.
> > That (besides that it should be able to all codecs and possible to
> > disable) is my main concern with the original patch:
> > that it will make it difficult to test the code for robustness of
> > its floating-point code while still leaving subtle bugs in it.
> > In general I very much believe that the only way to write a robust
> > program is by treating (almost?) any floating point numbers
> > as untrusted data, which this code clearly does not.
> 
> if you want to do the most robust thing then checking all audio
> encoder input somewhere like avcodec_encode_audio*
> could be done, would lead to a small slowdown though.

I very much do not consider that the "most robust thing".
IMHO that is kind of the minimum we'd need in the current situation.
But that doesn't guarantee that some codecs doesn't fail just
as much with crafted input, whether because NaNs are generated
inside the encoder or whether the numbers end up having less
precision than expected and someone assume f + 1 > f.

> I dont think that doing such checks on a per encoder basis is going to
> be very reliable unless someoe writes actual regression tests for it
> and debugs all failures

My suggestion would be to do as many of
1) review of float code
2) regression tests with NaN/Inf and other input
3) clamping/checking in avcodec_encode_audio by default (disabled for 2) of course)

Of course suggestions are worth little unless someone finds the
time and motivation to implement it, and my backlog is quite long ATM.

> treating any float as untrusted would require a check before
> every float->int i think ...

Well, I would assume the conversion itself to be safe, if it isn't
that's IMHO a OS/CPU issue.
So it would be fine to only check if that int is then used as a loop
condition or array index (more or less).
But yes, it would at least be effort and extra code. Speedwise I think
it might not be that bad.