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

[Reimar Döffinger]

On Fri, Mar 07, 2014 at 04:41:19PM +0100, Anders Rein wrote:
> On 03/07/2014 04:16 PM, Michael Niedermayer wrote:
> >On Fri, Mar 07, 2014 at 04:08:25PM +0100, Anders Rein wrote:
> >>The ffmpeg executable gave a segmentation fault when transcoding a
> >>wmv file to mp4 (h264, aac). The segfault happend in the aac encoder
> >>code and was caused by the wma2 decoder returning NaN samples. I
> >>tried to find the bug in the wma2 decoder, but eventually gave up. I
> >>did however add float clamping and checking for NaN in the aac
> >>encoder to make it more robust and avoid segmentation faults.
> >How can the NaN sample production be reproduced ?
> >I dont think decoders should return NaN samples
> Here is a link to the input file (I stripped away most of the file plus the
> video stream for copyright reasons):
> https://www.dropbox.com/s/4zzp03yes94mnrl/nan_example.wma
> 
> Command line to reproduce the segmentation fault:
> ffmpeg -i nan_example.wma -acodec aac -strict -2 out.mp4
> 
> While I agree that decoders shouldn't return NaN samples, I also don't see
> any reason to make the aac encoder more robust. An encoder shouldn't
> segfault no matter what data is put into it right?

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;
         return cost * lambda;
     }
     if (!scaled) {
@@ -204,6 +205,7 @@ static av_always_inline float quantize_and_encode_band_cost_template(
 
     if (bits)
         *bits = resbits;
+    if (cost != cost) cost = 0;
     return cost;
 }