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

[Anders Rein]

On 03/07/2014 08:26 PM, Reimar Döffinger wrote:
> 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;
>   }
>   
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
I think this looks much better than my patch. Is there any reason to not 
include this patch?