[FFmpeg-devel] [PATCH 0/1] fate-aac-encode-pred UBsan fix

[Sean McGovern]

From: Sean McGovern <sean at elysia.seanmcgovern.ca>

Hi FFmpeg-devel,

I've started looking into the results posted by the UBsan FATE node -- http://fate.ffmpeg.org/history.cgi?slot=x86_64-archlinux-gcc-ubsan

Here is the error reported by FATE (snipped for brevity) for 'fate-aac-encode-pred':

[aist#0:0/pcm_s16le @ 0x77fee40] Guessed Channel Layout: stereo
Input #0, wav, from '/srv/VIDEO/fate-suite/audio-reference/luckynight_2ch_44kHz_s16.wav':
  Duration: 00:00:09.50, bitrate: 1411 kb/s
  Stream #0:0: Audio: pcm_s16le ([1][0][0][0] / 0x0001), 44100 Hz, stereo, s16, 1411 kb/s
Stream mapping:
  Stream #0:0 -> #0:0 (pcm_s16le (native) -> aac (native))
Output #0, adts, to '/home/sean/build/ffmpeg-ubsan/tests/data/fate/aac-pred-encode.adts':
  Stream #0:0: Audio: aac (Main), 44100 Hz, stereo, fltp, 128 kb/s
      Metadata:
        encoder         : Lavc aac
src/libavcodec/aacenc_pred.c:169:48: runtime error: index 41 out of bounds for type 'uint8_t [41]'
threads=8

Ignore the thread count, this fails the same when threads=1

Here is the loop it fails in -- for this test 'sfb == 49':

libavcodec/aacenc_pred.c:165:    for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) {
libavcodec/aacenc_pred.c:166:        start = 0;
libavcodec/aacenc_pred.c:167:        for (g = 0; g < sce0->ics.num_swb; g++) {
libavcodec/aacenc_pred.c:168:            int sfb = w*16+g;
libavcodec/aacenc_pred.c:169:            int sum = sce0->ics.prediction_used[sfb] + sce1->ics.prediction_used[sfb];
...

Here is the array definition for prediction_used:

libavcodec/aac.h:188: uint8_t prediction_used[41];

Here is the line num_swb is set on:

libavcodec/aacenc.c:899: ics->num_swb            = tag == TYPE_LFE ? ics->num_swb : s->psy.num_bands[ics->num_windows == 8];

(side note: interesting, I've never seen a boolean condition used an an array index before...)

My attached patch just uses FFMIN to ensure we don't navigate past the end of ics.prediction_used[]
It corrects the undefined behaviour, but it feels naïve.

Any thoughts?


Sean McGovern (1):
  aacenc_pred: prevent UB in ff_aac_adjust_common_pred()

 libavcodec/aacenc_pred.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.39.2