| Message ID | 20220516011605.18792-3-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/jpegxl_probe: Check init_get_bits8() for failure | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 5/15/2022 10:16 PM, Michael Niedermayer wrote: > Maybe helps coverity > Helps: CID1433771 > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/mpeg4videodec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c > index e2bde73639..715cb606c9 100644 > --- a/libavcodec/mpeg4videodec.c > +++ b/libavcodec/mpeg4videodec.c > @@ -1981,7 +1981,8 @@ static int mpeg4_decode_studio_block(MpegEncContext *s, int32_t block[64], int n > return AVERROR_INVALIDDATA; > j = scantable[idx++]; > block[j] = get_xbits(&s->gb, additional_code_len); > - } else if (group == 21) { > + } else { > + av_assert2(group == 21); Group is used as index to access two arrays with 22 elements each at the beginning of the while loop here. Maybe just also check for group > 21 and abort like we're doing for < 0, since it's clearly not a valid or expected value. > /* Escape */ > if (idx > 63) > return AVERROR_INVALIDDATA;
James Almer: > > > On 5/15/2022 10:16 PM, Michael Niedermayer wrote: >> Maybe helps coverity >> Helps: CID1433771 >> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/mpeg4videodec.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c >> index e2bde73639..715cb606c9 100644 >> --- a/libavcodec/mpeg4videodec.c >> +++ b/libavcodec/mpeg4videodec.c >> @@ -1981,7 +1981,8 @@ static int >> mpeg4_decode_studio_block(MpegEncContext *s, int32_t block[64], int n >> return AVERROR_INVALIDDATA; >> j = scantable[idx++]; >> block[j] = get_xbits(&s->gb, additional_code_len); >> - } else if (group == 21) { >> + } else { >> + av_assert2(group == 21); > > Group is used as index to access two arrays with 22 elements each at the > beginning of the while loop here. Maybe just also check for group > 21 > and abort like we're doing for < 0, since it's clearly not a valid or > expected value. > Looking at ff_mpeg4_studio_intra shows that this is not a possible value, so it should be an assert, not an ordinary check. So I'd move the av_assert2 to before group is used in conjunction with ac_state_tab. >> /* Escape */ >> if (idx > 63) >> return AVERROR_INVALIDDATA;
Michael Niedermayer: > Maybe helps coverity > Helps: CID1433771 > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/mpeg4videodec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c > index e2bde73639..715cb606c9 100644 > --- a/libavcodec/mpeg4videodec.c > +++ b/libavcodec/mpeg4videodec.c > @@ -1981,7 +1981,8 @@ static int mpeg4_decode_studio_block(MpegEncContext *s, int32_t block[64], int n > return AVERROR_INVALIDDATA; > j = scantable[idx++]; > block[j] = get_xbits(&s->gb, additional_code_len); > - } else if (group == 21) { > + } else { > + av_assert2(group == 21); > /* Escape */ > if (idx > 63) > return AVERROR_INVALIDDATA; This also reminds me of an old attempt of mine to add an AV_UNREACHABLE macro for such scenarios: https://github.com/mkver/FFmpeg/commits/unreachable. There are two reasons why I never sent it to the ML: a) It uses ASSERT_LEVEL (i.e. with a high ASSERT_LEVEL it degenarates into an actual assert). But this is only defined internally, so useless to an API user. Therefore I wonder whether this should be in a public header (the same issue exists for av_assert1 and av_assert2). b) Both Clang and MSVC have something more, namely a __builtin_assume(cond) resp. __assume(cond). I was unsure whether this should not be added, too. It could be translated to "if (!(cond)) __builtin_unreachable()" to GCC, but would be more natural in general. (Of course, cond must not have any side effects.) - Andreas
On Mon, May 16, 2022 at 01:05:28PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Maybe helps coverity > > Helps: CID1433771 > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/mpeg4videodec.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c > > index e2bde73639..715cb606c9 100644 > > --- a/libavcodec/mpeg4videodec.c > > +++ b/libavcodec/mpeg4videodec.c > > @@ -1981,7 +1981,8 @@ static int mpeg4_decode_studio_block(MpegEncContext *s, int32_t block[64], int n > > return AVERROR_INVALIDDATA; > > j = scantable[idx++]; > > block[j] = get_xbits(&s->gb, additional_code_len); > > - } else if (group == 21) { > > + } else { > > + av_assert2(group == 21); > > /* Escape */ > > if (idx > 63) > > return AVERROR_INVALIDDATA; > > This also reminds me of an old attempt of mine to add an AV_UNREACHABLE > macro for such scenarios: > https://github.com/mkver/FFmpeg/commits/unreachable. There are two > reasons why I never sent it to the ML: > a) It uses ASSERT_LEVEL (i.e. with a high ASSERT_LEVEL it degenarates > into an actual assert). But this is only defined internally, so useless > to an API user. Therefore I wonder whether this should be in a public > header (the same issue exists for av_assert1 and av_assert2). for av_assertX to be usefull to a user app, the developer of that app needs to be able to set ASSERT_LEVEL. It would be less useful if it was fixed to the value ffmpeg used in its built The developer should be able to set ASSERT_LEVEL before the include or with -D but some other way could be added too > b) Both Clang and MSVC have something more, namely a > __builtin_assume(cond) resp. __assume(cond). I was unsure whether this > should not be added, too. It could be translated to "if (!(cond)) > __builtin_unreachable()" to GCC, but would be more natural in general. > (Of course, cond must not have any side effects.) [...]
diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c index e2bde73639..715cb606c9 100644 --- a/libavcodec/mpeg4videodec.c +++ b/libavcodec/mpeg4videodec.c @@ -1981,7 +1981,8 @@ static int mpeg4_decode_studio_block(MpegEncContext *s, int32_t block[64], int n return AVERROR_INVALIDDATA; j = scantable[idx++]; block[j] = get_xbits(&s->gb, additional_code_len); - } else if (group == 21) { + } else { + av_assert2(group == 21); /* Escape */ if (idx > 63) return AVERROR_INVALIDDATA;
Maybe helps coverity Helps: CID1433771 Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/mpeg4videodec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)