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

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

[Sean McGovern]

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

_______________________________________________
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".

[FFmpeg-devel] [PATCH 1/1] aacenc_pred: prevent UB in ff_aac_adjust_common_pred()

[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;
-- 
2.39.2

_______________________________________________
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".

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

[Sean McGovern]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/1] aacenc_pred: prevent UB in ff_aac_adjust_common_pred()

[Andreas Rheinhardt]

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

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/1] aacenc_pred: prevent UB in ff_aac_adjust_common_pred()

[Sean McGovern]

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
_______________________________________________
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".

[FFmpeg-devel] [PATCH] aacenc_pred: prevent UB in ff_aac_adjust_common_pred()

[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

_______________________________________________
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".