diff mbox series

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

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

Checks

Context Check Description
yinshiyou/make_fate_loongarch64 success Make fate finished
yinshiyou/make_loongarch64 warning New warnings during build
andriy/make_fate_x86 success Make fate finished
andriy/make_x86 warning New warnings during build

Commit Message

Sean McGovern Feb. 25, 2024, 6:44 p.m. UTC 
---
 libavcodec/aacenc_pred.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt Feb. 27, 2024, 6:39 p.m. UTC | #1 
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
Sean McGovern Feb. 27, 2024, 6:59 p.m. UTC | #2 
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
diff mbox series

Patch

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;