[FFmpeg-devel] [PATCH 6/6] avfilter/af_surround: Check av_channel_layout_channel_from_index() stays within the fixed array used

[James Almer]

On 7/7/2024 6:59 PM, Michael Niedermayer wrote:
> On Sun, Jul 07, 2024 at 09:12:06PM +0200, Andreas Rheinhardt wrote:
>> Andreas Rheinhardt:
>>> Michael Niedermayer:
>>>> Fixes: CID1516994 Out-of-bounds access
>>>> Fixes: CID1516996 Out-of-bounds access
>>>> Fixes: CID1516999 Out-of-bounds access
>>>>
>>>> Sponsored-by: Sovereign Tech Fund
>>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>>> ---
>>>>   libavfilter/af_surround.c | 3 +++
>>>>   1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/libavfilter/af_surround.c b/libavfilter/af_surround.c
>>>> index e37dddc3614..fab39a37ea9 100644
>>>> --- a/libavfilter/af_surround.c
>>>> +++ b/libavfilter/af_surround.c
>>>> @@ -269,6 +269,9 @@ static int config_output(AVFilterLink *outlink)
>>>>   
>>>>       for (int ch = 0; ch < outlink->ch_layout.nb_channels; ch++) {
>>>>           float iscale = 1.f;
>>>> +        const int chan = av_channel_layout_channel_from_index(&s->out_ch_layout, ch);
>>>> +        if (chan >= FF_ARRAY_ELEMS(sc_map))
>>>> +            return AVERROR_PATCHWELCOME;
>>>>   
>>>>           ret = av_tx_init(&s->irdft[ch], &s->itx_fn, AV_TX_FLOAT_RDFT,
>>>>                            1, s->win_size, &iscale, 0);
>>>
>>> Can this happen?
> 
> IMHO, this doesnt matter. A filter that depends on a audio channel layout
> API from another lib cannot depend on its implementation but just the
> public API/ABI
> So even if the av_channel_layout_* API didnt allow us to set such layout
> today we would need to check for it
> 
> now can this happen?
> try this:
> 
> ./ffmpeg -i matrixbench_mpeg2.mpg -af surround=chl_out="123456789" -f null -
> 
> I get a
> Segmentation fault (core dumped)
> 
> and it doesnt segfault after the patch

This is (probably) a regression since 66afa361e816.
Maybe an output layout sanity check should be added back to init() in 
some form instead, to return EINVAL after an "Unsupported upmix" warning 
message is printed, like it used to be the case.