[FFmpeg-devel] [PATCH 1/3] tools/target_dec_fuzzer: adjust threshold for RKA

[FFmpeg-devel] [PATCH 1/3] tools/target_dec_fuzzer: adjust threshold for RKA

[Michael Niedermayer]

Fixes: Timeout
Fixes: 59349/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RKA_fuzzer-5334280839233536

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 tools/target_dec_fuzzer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c
index 1dbdad50b6..b82efc01b0 100644
--- a/tools/target_dec_fuzzer.c
+++ b/tools/target_dec_fuzzer.c
@@ -272,7 +272,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
     case AV_CODEC_ID_QTRLE:       maxpixels  /= 16;    break;
     case AV_CODEC_ID_PAF_VIDEO:   maxpixels  /= 16;    break;
     case AV_CODEC_ID_PRORES:      maxpixels  /= 256;   break;
-    case AV_CODEC_ID_RKA:         maxsamples /= 256;   break;
+    case AV_CODEC_ID_RKA:         maxsamples /= 65536; break;
     case AV_CODEC_ID_RSCC:        maxpixels  /= 256;   break;
     case AV_CODEC_ID_RASC:        maxpixels  /= 16;    break;
     case AV_CODEC_ID_SANM:        maxpixels  /= 16;    break;
-- 
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] avcodec/cbs_av1: Clear obu.metadata on error

[Michael Niedermayer]

On error pointers can be left NULL while code later assumes these not to be NULL

Fixes: NULL pointer dereference
Fixes: 59359/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-6726080594313216

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/cbs_av1.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
index 8788fee099..7f3f4da2f5 100644
--- a/libavcodec/cbs_av1.c
+++ b/libavcodec/cbs_av1.c
@@ -1013,8 +1013,10 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx,
     case AV1_OBU_METADATA:
         {
             err = cbs_av1_read_metadata_obu(ctx, &gbc, &obu->obu.metadata);
-            if (err < 0)
+            if (err < 0) {
+                memset(&obu->obu.metadata, 0, sizeof(obu->obu.metadata));
                 return err;
+            }
         }
         break;
     case AV1_OBU_PADDING:
-- 
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 2/3] avcodec/cbs_av1: Clear obu.metadata on error

[Andreas Rheinhardt]

Michael Niedermayer:
> On error pointers can be left NULL while code later assumes these not to be NULL
> 
> Fixes: NULL pointer dereference
> Fixes: 59359/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-6726080594313216
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/cbs_av1.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> index 8788fee099..7f3f4da2f5 100644
> --- a/libavcodec/cbs_av1.c
> +++ b/libavcodec/cbs_av1.c
> @@ -1013,8 +1013,10 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx,
>      case AV1_OBU_METADATA:
>          {
>              err = cbs_av1_read_metadata_obu(ctx, &gbc, &obu->obu.metadata);
> -            if (err < 0)
> +            if (err < 0) {
> +                memset(&obu->obu.metadata, 0, sizeof(obu->obu.metadata));
>                  return err;
> +            }
>          }
>          break;
>      case AV1_OBU_PADDING:

1. Before 97f4263, the current_obu was reset (and the packet effectively
discarded) upon errors from ff_cbs_read_packet(); yet this is no longer
true and it seems that the contents of current_obu will be processed in
the next call to av1_receive_frame(). This change seems to have been
unintentional.
2. The commit message is weird: You claim that it is bad that a pointer
is NULL; and then you go on and zero it again. It would be better to
claim that the metadata is in an inconsistent state.
3. There is a possibility for inconsistency in cbs_av1_syntax_template,
namely if the allocation fails, metadata OBU nevertheless claims to have
a payload_size > 0; payload_size should only be set after the allocation
succeeded. But this does not seem to be the issue in this testcase.
Presumably there are not enough bits left for the itu_t_t35_country_code
or its extension? In this case, cbs_av1 did not even make an error.
4. Your fix is dangerous: In case the code were changed so that an error
can happen after a successful allocation, your memset would lead to
leaks. (The most likely possibility is the addition of a new type of
metadata; another way would be for the code to be changed to avoid
reading the metadata twice by reallocating (and overallocating) the
buffer as needed.)

- 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 2/3] avcodec/cbs_av1: Clear obu.metadata on error

[James Almer]

On 6/1/2023 9:28 PM, Andreas Rheinhardt wrote:
> 1. Before 97f4263, the current_obu was reset (and the packet effectively
> discarded) upon errors from ff_cbs_read_packet(); yet this is no longer
> true and it seems that the contents of current_obu will be processed in
> the next call to av1_receive_frame(). This change seems to have been
> unintentional.

I guess I assumed that ff_cbs_read_packet() failing would clear the 
CodedBitstreamFragment before returning, but if that's not the case then 
ff_cbs_fragment_reset() should be called.

Would something like

> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
> index 5cc5d87c64..d1a0f6eaa2 100644
> --- a/libavcodec/av1dec.c
> +++ b/libavcodec/av1dec.c
> @@ -1461,6 +1461,7 @@ static int av1_receive_frame(AVCodecContext *avctx, AVFrame *frame)
>              ret = ff_cbs_read_packet(s->cbc, &s->current_obu, s->pkt);
>              if (ret < 0) {
>                  av_packet_unref(s->pkt);
> +                ff_cbs_fragment_reset(&s->current_obu);
>                  av_log(avctx, AV_LOG_ERROR, "Failed to read packet.\n");
>                  return ret;
>              }

Be enough?
_______________________________________________
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] avcodec/cbs_av1: Clear obu.metadata on error

[Michael Niedermayer]

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

On Thu, Jun 01, 2023 at 09:33:14PM -0300, James Almer wrote:
> On 6/1/2023 9:28 PM, Andreas Rheinhardt wrote:
> > 1. Before 97f4263, the current_obu was reset (and the packet effectively
> > discarded) upon errors from ff_cbs_read_packet(); yet this is no longer
> > true and it seems that the contents of current_obu will be processed in
> > the next call to av1_receive_frame(). This change seems to have been
> > unintentional.
> 
> I guess I assumed that ff_cbs_read_packet() failing would clear the
> CodedBitstreamFragment before returning, but if that's not the case then
> ff_cbs_fragment_reset() should be called.
> 
> Would something like
> 
> > diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
> > index 5cc5d87c64..d1a0f6eaa2 100644
> > --- a/libavcodec/av1dec.c
> > +++ b/libavcodec/av1dec.c
> > @@ -1461,6 +1461,7 @@ static int av1_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> >              ret = ff_cbs_read_packet(s->cbc, &s->current_obu, s->pkt);
> >              if (ret < 0) {
> >                  av_packet_unref(s->pkt);
> > +                ff_cbs_fragment_reset(&s->current_obu);
> >                  av_log(avctx, AV_LOG_ERROR, "Failed to read packet.\n");
> >                  return ret;
> >              }
> 
> Be enough?

yes, that solves it and looks much better than my patch

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire

[-- 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] avcodec/cbs_av1: Clear obu.metadata on error

[Michael Niedermayer]

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

On Fri, Jun 02, 2023 at 02:28:24AM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On error pointers can be left NULL while code later assumes these not to be NULL
> > 
> > Fixes: NULL pointer dereference
> > Fixes: 59359/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-6726080594313216
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/cbs_av1.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> > index 8788fee099..7f3f4da2f5 100644
> > --- a/libavcodec/cbs_av1.c
> > +++ b/libavcodec/cbs_av1.c
> > @@ -1013,8 +1013,10 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx,
> >      case AV1_OBU_METADATA:
> >          {
> >              err = cbs_av1_read_metadata_obu(ctx, &gbc, &obu->obu.metadata);
> > -            if (err < 0)
> > +            if (err < 0) {
> > +                memset(&obu->obu.metadata, 0, sizeof(obu->obu.metadata));
> >                  return err;
> > +            }
> >          }
> >          break;
> >      case AV1_OBU_PADDING:
> 
> 1. Before 97f4263, the current_obu was reset (and the packet effectively
> discarded) upon errors from ff_cbs_read_packet(); yet this is no longer
> true and it seems that the contents of current_obu will be processed in
> the next call to av1_receive_frame(). This change seems to have been
> unintentional.
> 2. The commit message is weird: You claim that it is bad that a pointer
> is NULL; and then you go on and zero it again. It would be better to
> claim that the metadata is in an inconsistent state.
> 3. There is a possibility for inconsistency in cbs_av1_syntax_template,
> namely if the allocation fails, metadata OBU nevertheless claims to have
> a payload_size > 0; payload_size should only be set after the allocation
> succeeded. But this does not seem to be the issue in this testcase.
> Presumably there are not enough bits left for the itu_t_t35_country_code
> or its extension? In this case, cbs_av1 did not even make an error.
> 4. Your fix is dangerous: In case the code were changed so that an error
> can happen after a successful allocation, your memset would lead to
> leaks. (The most likely possibility is the addition of a new type of
> metadata; another way would be for the code to be changed to avoid
> reading the metadata twice by reallocating (and overallocating) the
> buffer as needed.)

posting a suboptimal fix, if one isnt sure what is the clean way to fix it
is a great way to have someone else make a better fix.

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 

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

[FFmpeg-devel] [PATCH 3/3] avcodec/mpeg4videodec: more unsigned in amv computation

[Michael Niedermayer]

Fixes: signed integer overflow: -2147483648 + -1048576 cannot be represented in type 'int'
Fixes: 59365/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-642654923954585

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/mpeg4videodec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
index d456e5dd11..72c8ad3048 100644
--- a/libavcodec/mpeg4videodec.c
+++ b/libavcodec/mpeg4videodec.c
@@ -861,7 +861,7 @@ static inline int get_amv(Mpeg4DecContext *ctx, int n)
         for (y = 0; y < 16; y++) {
             int v;
 
-            v = mb_v + dy * y;
+            v = mb_v + (unsigned)dy * y;
             // FIXME optimize
             for (x = 0; x < 16; x++) {
                 sum += v >> shift;
-- 
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 1/3] tools/target_dec_fuzzer: adjust threshold for RKA

[Michael Niedermayer]

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

On Fri, Jun 02, 2023 at 12:25:59AM +0200, Michael Niedermayer wrote:
> Fixes: Timeout
> Fixes: 59349/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RKA_fuzzer-5334280839233536
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  tools/target_dec_fuzzer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

will apply 1/3 and 3/3

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

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus

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