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

[Michael Niedermayer]

On Tue, Mar 11, 2014 at 10:10:09PM +0100, Reimar Döffinger wrote:
> 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.

without checking the spec, isnt (int)NaN undefined in C ?


> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140312/eddd727d/attachment.asc>