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

[Michael Niedermayer]

On Fri, Mar 07, 2014 at 08:26:44PM +0100, 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;

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


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
-------------- 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/20140310/40af35f1/attachment.asc>