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

[Martin Storsjö]

On Sat, 5 Oct 2024, Sean McGovern wrote:

> Hi
>
> On Sat, Oct 5, 2024, 19:15 Lynne via ffmpeg-devel <ffmpeg-devel at 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