mbox series

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

Message ID 20240225184429.2962707-1-gseanmcg@gmail.com
Headers show
Series fate-aac-encode-pred UBsan fix | expand

Message

Sean McGovern Feb. 25, 2024, 6:44 p.m. UTC 
From: Sean McGovern <sean@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(-)

Comments

Sean McGovern Feb. 25, 2024, 6:55 p.m. UTC | #1 
On Sun, Feb 25, 2024 at 1:44 PM Sean McGovern <gseanmcg@gmail.com> wrote:
>
>
> Here is the error reported by FATE (snipped for brevity) for 'fate-aac-encode-pred':
>

Ooops! I should copy-paste more often -- that should be
'fate-aac-pred-encode' :)

-- Sean McGovern