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

[Michael Niedermayer]

On Tue, Mar 11, 2014 at 07:15:03PM +0100, Reimar Döffinger wrote:
> On Mon, Mar 10, 2014 at 08:49:25PM +0100, Michael Niedermayer wrote:
> > On Fri, Mar 07, 2014 at 08:26:44PM +0100, Reimar Döffinger wrote:
> > > IMHO robustness means understanding the issue and not have exploitable
> > > bugs as soon as somehow an NaN manages to sneak in.
> > > Though I suspect that the encoder cannot work properly if it
> > > gets values outside the 0-1 range and thus needs to be "protected"
> > > anyway.
> > > I found two issues:
> > > 1) aaccoder.c:79
> > >         out[i] = (int)FFMIN(qc + 0.4054, (double)maxval);
> > > To me, this code is a giant WTF, I have no idea why it is done this way,
> > > but it's basically done in exactly the only possible way that
> > > will cause a crash with NaNs.
> > > Swapping the order of the FFMIN arguments for example would
> > > make it work correctly, however this way around the NaN will
> > > go through, and casting to int then gives INT_MIN.
> > > I also don't know what speaks against first converting
> > > to int and then doing FFMIN, except possible overflow issues?
> > > 
> > > 
> > > 2) cost calculation in quantize_and_encode_band_cost_template.
> > > If the cost returned is NaN there will be crashes later on.
> > > I have not analyzed in more detail, so those might be issues
> > > that can be triggered even without a NaN.
> > > 
> > > A quick hack avoiding the crash would look like this:
> > > --- a/libavcodec/aaccoder.c
> > > +++ b/libavcodec/aaccoder.c
> > > @@ -76,7 +76,7 @@ static void quantize_bands(int *out, const float *in, const float *scaled,
> > >      double qc;
> > >      for (i = 0; i < size; i++) {
> > >          qc = scaled[i] * Q34;
> > > -        out[i] = (int)FFMIN(qc + 0.4054, (double)maxval);
> > > +        out[i] = (int)FFMIN((double)maxval, qc + 0.4054);
> > >          if (is_signed && in[i] < 0.0f) {
> > >              out[i] = -out[i];
> > >          }
> > > @@ -128,6 +128,7 @@ static av_always_inline float quantize_and_encode_band_cost_template(
> > >              cost += in[i]*in[i];
> > >          if (bits)
> > >              *bits = 0;
> > > +        if (cost != cost) cost = 0;
> > 
> > should be isnan
> > 
> > about the patch, feel free to push it but iam not sure
> > if these kind of hacks to support NaN input make sense
> 
> 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 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

such generic "is any NaN" check could be limited to when the assert
level is high

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator
-------------- 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/20140311/7c8f6167/attachment.asc>