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

From: Michael Niedermayer <michael@niedermayer.cc>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 6/6] avfilter/af_surround: Check av_channel_layout_channel_from_index() stays within the fixed array used
Date: Sun, 7 Jul 2024 23:59:17 +0200
Message-ID: <20240707215917.GT4991@pb2> (raw)
In-Reply-To: <AS8P250MB0744BDE9844F803FFCDC0C938FD92@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM>

[-- Attachment #1.1: Type: text/plain, Size: 2324 bytes --]

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@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

> > 
> 
> Apart from that: I think you are mistaken when you believe that this
> will "fix" the issue. Coverity will not think that these issues are
> fixed even with this check.

After this patch the issue is either detected as fixed or not,
if not then it becomes a false positive and either way is fixed

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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