[PATCH 0/1] fate-aac-encode-pred UBsan fix
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(-) -- 2.39.2
--- libavcodec/aacenc_pred.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libavcodec/aacenc_pred.c b/libavcodec/aacenc_pred.c index f87fcd5a00..d3efade85e 100644 --- a/libavcodec/aacenc_pred.c +++ b/libavcodec/aacenc_pred.c @@ -162,9 +162,11 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe) sce1->ics.window_sequence[0] == EIGHT_SHORT_SEQUENCE) return; + const int num_swb = FFMIN(sce0->ics.num_swb, sizeof(sce0->ics.prediction_used)); + for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) { start = 0; - for (g = 0; g < sce0->ics.num_swb; g++) { + for (g = 0; g < num_swb; g++) { int sfb = w*16+g; int sum = sce0->ics.prediction_used[sfb] + sce1->ics.prediction_used[sfb]; float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f; -- 2.39.2
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
Sean McGovern:
--- libavcodec/aacenc_pred.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libavcodec/aacenc_pred.c b/libavcodec/aacenc_pred.c index f87fcd5a00..d3efade85e 100644 --- a/libavcodec/aacenc_pred.c +++ b/libavcodec/aacenc_pred.c @@ -162,9 +162,11 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe) sce1->ics.window_sequence[0] == EIGHT_SHORT_SEQUENCE) return;
+ const int num_swb = FFMIN(sce0->ics.num_swb, sizeof(sce0->ics.prediction_used)); + for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) { start = 0; - for (g = 0; g < sce0->ics.num_swb; g++) { + for (g = 0; g < num_swb; g++) { int sfb = w*16+g; int sum = sce0->ics.prediction_used[sfb] + sce1->ics.prediction_used[sfb]; float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f;
As you can see, the actual index used for accesses is w*16 + g and not only g. So I was surprised that your fix fixed the test (as you claim). Digging into the code, num_windows can be either 1 or eight and it is only eight if window_sequence[0] is EIGHT_SHORT_SEQUENCE (see lines 477-488 in aacpsy.c as well as lines 877-897 in aacenc.c). In case window_sequence[0] is EIGHT_SHORT_SEQUENCE, we do not even enter this loop in ff_aac_adjust_common_pred(). This means that the outer loop above is actually not a loop at all and your fix would indeed fix the undefined behaviour. But this also shows that this whole code is a mess. Someone who actually knows it should take a look. Or maybe the grim reaper. Anyway, your fix would lead to a wdeclaration-after-statement warning. - Andreas
Hi Andreas, First off all, thanks for having a look! :) On Tue, Feb 27, 2024 at 1:37 PM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
Sean McGovern:
--- libavcodec/aacenc_pred.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libavcodec/aacenc_pred.c b/libavcodec/aacenc_pred.c index f87fcd5a00..d3efade85e 100644 --- a/libavcodec/aacenc_pred.c +++ b/libavcodec/aacenc_pred.c @@ -162,9 +162,11 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe) sce1->ics.window_sequence[0] == EIGHT_SHORT_SEQUENCE) return;
+ const int num_swb = FFMIN(sce0->ics.num_swb, sizeof(sce0->ics.prediction_used)); + for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) { start = 0; - for (g = 0; g < sce0->ics.num_swb; g++) { + for (g = 0; g < num_swb; g++) { int sfb = w*16+g; int sum = sce0->ics.prediction_used[sfb] + sce1->ics.prediction_used[sfb]; float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f;
As you can see, the actual index used for accesses is w*16 + g and not only g. So I was surprised that your fix fixed the test (as you claim). Digging into the code, num_windows can be either 1 or eight and it is only eight if window_sequence[0] is EIGHT_SHORT_SEQUENCE (see lines 477-488 in aacpsy.c as well as lines 877-897 in aacenc.c). In case window_sequence[0] is EIGHT_SHORT_SEQUENCE, we do not even enter this loop in ff_aac_adjust_common_pred(). This means that the outer loop above is actually not a loop at all and your fix would indeed fix the undefined behaviour. But this also shows that this whole code is a mess. Someone who actually knows it should take a look. Or maybe the grim reaper. Anyway, your fix would lead to a wdeclaration-after-statement warning.
Ooof, OK thanks. I was wondering about that when I looked on Patchwork. Thanks, -- Sean McGovern
Iterate over 'pmax' instead of 'num_swb'. --- libavcodec/aacenc_pred.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/aacenc_pred.c b/libavcodec/aacenc_pred.c index a486c44d42..c5b8aa9665 100644 --- a/libavcodec/aacenc_pred.c +++ b/libavcodec/aacenc_pred.c @@ -164,7 +164,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe) for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) { start = 0; - for (g = 0; g < sce0->ics.num_swb; g++) { + for (g = 0; g < pmax; g++) { int sfb = w*16+g; int sum = sce0->ics.prediction_used[sfb] + sce1->ics.prediction_used[sfb]; float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f; -- 2.39.2
--- libavcodec/aacenc_pred.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/libavcodec/aacenc_pred.c b/libavcodec/aacenc_pred.c index a486c44d42..a6dfaa25fb 100644 --- a/libavcodec/aacenc_pred.c +++ b/libavcodec/aacenc_pred.c @@ -153,9 +153,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe) int start, w, w2, g, i, count = 0; SingleChannelElement *sce0 = &cpe->ch[0]; SingleChannelElement *sce1 = &cpe->ch[1]; - const int pmax0 = FFMIN(sce0->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); - const int pmax1 = FFMIN(sce1->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); - const int pmax = FFMIN(pmax0, pmax1); + const int pmax = FFMIN(sce1->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); if (!cpe->common_window || sce0->ics.window_sequence[0] == EIGHT_SHORT_SEQUENCE || @@ -164,7 +162,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe) for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) { start = 0; - for (g = 0; g < sce0->ics.num_swb; g++) { + for (g = 0; g < pmax; g++) { int sfb = w*16+g; int sum = sce0->ics.prediction_used[sfb] + sce1->ics.prediction_used[sfb]; float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f; -- 2.39.5
On 05/10/2024 20:58, Sean McGovern wrote:
--- libavcodec/aacenc_pred.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/libavcodec/aacenc_pred.c b/libavcodec/aacenc_pred.c index a486c44d42..a6dfaa25fb 100644 --- a/libavcodec/aacenc_pred.c +++ b/libavcodec/aacenc_pred.c @@ -153,9 +153,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe) int start, w, w2, g, i, count = 0; SingleChannelElement *sce0 = &cpe->ch[0]; SingleChannelElement *sce1 = &cpe->ch[1]; - const int pmax0 = FFMIN(sce0->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); - const int pmax1 = FFMIN(sce1->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); - const int pmax = FFMIN(pmax0, pmax1); + const int pmax = FFMIN(sce1->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]);
if (!cpe->common_window || sce0->ics.window_sequence[0] == EIGHT_SHORT_SEQUENCE || @@ -164,7 +162,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe)
for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) { start = 0; - for (g = 0; g < sce0->ics.num_swb; g++) { + for (g = 0; g < pmax; g++) { int sfb = w*16+g; int sum = sce0->ics.prediction_used[sfb] + sce1->ics.prediction_used[sfb]; float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f;
I'm not sure I see the UB here?
Hi On Sat, Oct 5, 2024, 19:15 Lynne via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote:
On 05/10/2024 20:58, Sean McGovern wrote:
--- libavcodec/aacenc_pred.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/libavcodec/aacenc_pred.c b/libavcodec/aacenc_pred.c index a486c44d42..a6dfaa25fb 100644 --- a/libavcodec/aacenc_pred.c +++ b/libavcodec/aacenc_pred.c @@ -153,9 +153,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe) int start, w, w2, g, i, count = 0; SingleChannelElement *sce0 = &cpe->ch[0]; SingleChannelElement *sce1 = &cpe->ch[1]; - const int pmax0 = FFMIN(sce0->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); - const int pmax1 = FFMIN(sce1->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); - const int pmax = FFMIN(pmax0, pmax1); + const int pmax = FFMIN(sce1->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]);
if (!cpe->common_window || sce0->ics.window_sequence[0] == EIGHT_SHORT_SEQUENCE || @@ -164,7 +162,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe)
for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) { start = 0; - for (g = 0; g < sce0->ics.num_swb; g++) { + for (g = 0; g < pmax; g++) { int sfb = w*16+g; int sum = sce0->ics.prediction_used[sfb] + sce1->ics.prediction_used[sfb]; float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f;
I'm not sure I see the UB here?
It corrects the issue noted by both the x86_64 and PPC64 UBsan FATE nodes. _______________________________________________
ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sat, 5 Oct 2024, Sean McGovern wrote:
Hi
On Sat, Oct 5, 2024, 19:15 Lynne via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote:
On 05/10/2024 20:58, Sean McGovern wrote:
--- libavcodec/aacenc_pred.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/libavcodec/aacenc_pred.c b/libavcodec/aacenc_pred.c index a486c44d42..a6dfaa25fb 100644 --- a/libavcodec/aacenc_pred.c +++ b/libavcodec/aacenc_pred.c @@ -153,9 +153,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe) int start, w, w2, g, i, count = 0; SingleChannelElement *sce0 = &cpe->ch[0]; SingleChannelElement *sce1 = &cpe->ch[1]; - const int pmax0 = FFMIN(sce0->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); - const int pmax1 = FFMIN(sce1->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); - const int pmax = FFMIN(pmax0, pmax1); + const int pmax = FFMIN(sce1->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]);
if (!cpe->common_window || sce0->ics.window_sequence[0] == EIGHT_SHORT_SEQUENCE || @@ -164,7 +162,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe)
for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) { start = 0; - for (g = 0; g < sce0->ics.num_swb; g++) { + for (g = 0; g < pmax; g++) { int sfb = w*16+g; int sum = sce0->ics.prediction_used[sfb] + sce1->ics.prediction_used[sfb]; float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f;
I'm not sure I see the UB here?
It corrects the issue noted by both the x86_64 and PPC64 UBsan FATE nodes.
That issue will be impossible to find for people looking at this code, once such runs no longer are visible on FATE. Always summarize the issue and how you go about fixing it, in the commit message. // Martin
Hi, On Sun, Oct 6, 2024, 15:48 Martin Storsjö <martin@martin.st> wrote:
On Sat, 5 Oct 2024, Sean McGovern wrote:
Hi
On Sat, Oct 5, 2024, 19:15 Lynne via ffmpeg-devel < ffmpeg-devel@ffmpeg.org> wrote:
On 05/10/2024 20:58, Sean McGovern wrote:
--- libavcodec/aacenc_pred.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/libavcodec/aacenc_pred.c b/libavcodec/aacenc_pred.c index a486c44d42..a6dfaa25fb 100644 --- a/libavcodec/aacenc_pred.c +++ b/libavcodec/aacenc_pred.c @@ -153,9 +153,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe) int start, w, w2, g, i, count = 0; SingleChannelElement *sce0 = &cpe->ch[0]; SingleChannelElement *sce1 = &cpe->ch[1]; - const int pmax0 = FFMIN(sce0->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); - const int pmax1 = FFMIN(sce1->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]); - const int pmax = FFMIN(pmax0, pmax1); + const int pmax = FFMIN(sce1->ics.max_sfb, ff_aac_pred_sfb_max[s->samplerate_index]);
if (!cpe->common_window || sce0->ics.window_sequence[0] == EIGHT_SHORT_SEQUENCE || @@ -164,7 +162,7 @@ void ff_aac_adjust_common_pred(AACEncContext *s, ChannelElement *cpe)
for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) { start = 0; - for (g = 0; g < sce0->ics.num_swb; g++) { + for (g = 0; g < pmax; g++) { int sfb = w*16+g; int sum = sce0->ics.prediction_used[sfb] + sce1->ics.prediction_used[sfb]; float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f;
I'm not sure I see the UB here?
It corrects the issue noted by both the x86_64 and PPC64 UBsan FATE nodes.
That issue will be impossible to find for people looking at this code, once such runs no longer are visible on FATE.
Always summarize the issue and how you go about fixing it, in the commit message.
// Martin
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
I'll admit to knowing precious little about the internals of aacenc. Maybe someone would be so kind to help me cook up a sufficient commit message? Thanks, Sean McGovern
participants (4)
-
Andreas Rheinhardt -
Lynne -
Martin Storsjö -
Sean McGovern