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

[Reimar Döffinger]

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.