[FFmpeg-devel] [PATCH 1/3] avformat/jpegxl_probe: Check init_get_bits8() for failure

[FFmpeg-devel] [PATCH 1/3] avformat/jpegxl_probe: Check init_get_bits8() for failure

[Michael Niedermayer]

Fixes: missing error check
Fixes: CID1504270

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/jpegxl_probe.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavformat/jpegxl_probe.c b/libavformat/jpegxl_probe.c
index 9cd00da194..3de002f004 100644
--- a/libavformat/jpegxl_probe.c
+++ b/libavformat/jpegxl_probe.c
@@ -250,8 +250,11 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen)
     int xyb_encoded = 1, have_icc_profile = 0;
     uint32_t num_extra_channels;
     uint64_t extensions;
+    int ret;
 
-    init_get_bits8(gb, buf, buflen);
+    ret = init_get_bits8(gb, buf, buflen);
+    if (ret < 0)
+        return ret;
 
     if (jxl_bits(16) != FF_JPEGXL_CODESTREAM_SIGNATURE_LE)
         return -1;
-- 
2.17.1

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

[FFmpeg-devel] [PATCH 2/3] avformat/act: Check ff_get_wav_header() for failure

[Michael Niedermayer]

Fixes: missing error check
Fixes: CID717495

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/act.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavformat/act.c b/libavformat/act.c
index 6dd9f62a87..da73fcceca 100644
--- a/libavformat/act.c
+++ b/libavformat/act.c
@@ -68,6 +68,7 @@ static int read_header(AVFormatContext *s)
     AVIOContext *pb = s->pb;
     int size;
     AVStream* st;
+    int ret;
 
     int min,sec,msec;
 
@@ -77,7 +78,9 @@ static int read_header(AVFormatContext *s)
 
     avio_skip(pb, 16);
     size=avio_rl32(pb);
-    ff_get_wav_header(s, pb, st->codecpar, size, 0);
+    ret = ff_get_wav_header(s, pb, st->codecpar, size, 0);
+    if (ret < 0)
+        return ret;
 
     /*
       8000Hz (Fine-rec) file format has 10 bytes long
-- 
2.17.1

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

[FFmpeg-devel] [PATCH 3/3] avcodec/mpeg4videodec: Replace always true check by assert

[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;
-- 
2.17.1

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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/mpeg4videodec: Replace always true check by assert

[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.

>               /* Escape */
>               if (idx > 63)
>                   return AVERROR_INVALIDDATA;
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/mpeg4videodec: Replace always true check by assert

[Andreas Rheinhardt]

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;



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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/mpeg4videodec: Replace always true check by assert

[Andreas Rheinhardt]

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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/mpeg4videodec: Replace always true check by assert

[Michael Niedermayer]

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

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.)


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Nations do behave wisely once they have exhausted all other alternatives. 
-- Abba Eban

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

Re: [FFmpeg-devel] [PATCH 2/3] avformat/act: Check ff_get_wav_header() for failure

[Michael Niedermayer]

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

On Mon, May 16, 2022 at 03:16:04AM +0200, Michael Niedermayer wrote:
> Fixes: missing error check
> Fixes: CID717495
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/act.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

will apply

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras

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

Re: [FFmpeg-devel] [PATCH 1/3] avformat/jpegxl_probe: Check init_get_bits8() for failure

[Michael Niedermayer]

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

On Mon, May 16, 2022 at 03:16:03AM +0200, Michael Niedermayer wrote:
> Fixes: missing error check
> Fixes: CID1504270
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/jpegxl_probe.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

will apply

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.

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