| Message ID | 20240707184729.3525852-6-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avcodec/tiff: Check value on positive signed targets | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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? - Andreas
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? > 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. - Andreas
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 [...]
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@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.
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);
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(+)