[FFmpeg-devel] [PATCH 1/3] avformat/lafdec: Check for EOF in header reading

[FFmpeg-devel] [PATCH 1/3] avformat/lafdec: Check for EOF in header reading

[Michael Niedermayer]

Fixes: OOM testcase
Fixes: 51527/clusterfuzz-testcase-minimized-ffmpeg_dem_LAF_fuzzer-5453663505612800

OOM can still happen after this as an arbitrary sized block is allocated and read
this would require a redesign or some limit on the sample rate.

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

diff --git a/libavformat/lafdec.c b/libavformat/lafdec.c
index 12b0d8540b..d02b479c4d 100644
--- a/libavformat/lafdec.c
+++ b/libavformat/lafdec.c
@@ -111,6 +111,9 @@ static int laf_read_header(AVFormatContext *ctx)
     sample_rate = avio_rl32(pb);
     duration = avio_rl64(pb) / st_count;
 
+    if (avio_feof(pb))
+        return AVERROR_INVALIDDATA;
+
     switch (quality) {
     case 0:
         codec_id = AV_CODEC_ID_PCM_U8;
-- 
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/mjpegbdec: Check for AVDISCARD_ALL

[Michael Niedermayer]

Fixes: Assertion failure
Fixes: 51825/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MJPEGB_fuzzer-6393802688692224

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

diff --git a/libavcodec/mjpegbdec.c b/libavcodec/mjpegbdec.c
index 98c64b44ca..a82a95d70a 100644
--- a/libavcodec/mjpegbdec.c
+++ b/libavcodec/mjpegbdec.c
@@ -141,9 +141,10 @@ read_header:
         av_log(avctx, AV_LOG_WARNING, "no picture\n");
         return buf_size;
     }
-
     av_frame_move_ref(rframe, s->picture_ptr);
     s->got_picture = 0;
+    if (avctx->skip_frame == AVDISCARD_ALL)
+        return AVERROR(EAGAIN);
     *got_frame = 1;
 
     if (!s->lossless && avctx->debug & FF_DEBUG_QP) {
-- 
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/mjpegbdec: Check for AVDISCARD_ALL

[Michael Niedermayer]

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

On Sun, Nov 13, 2022 at 12:44:00AM +0100, Michael Niedermayer wrote:
> Fixes: Assertion failure
> Fixes: 51825/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MJPEGB_fuzzer-6393802688692224
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/mjpegbdec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

will apply

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

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell

[-- 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/mjpegbdec: Check for AVDISCARD_ALL

[James Almer]

On 11/12/2022 8:44 PM, Michael Niedermayer wrote:
> Fixes: Assertion failure
> Fixes: 51825/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MJPEGB_fuzzer-6393802688692224
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavcodec/mjpegbdec.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mjpegbdec.c b/libavcodec/mjpegbdec.c
> index 98c64b44ca..a82a95d70a 100644
> --- a/libavcodec/mjpegbdec.c
> +++ b/libavcodec/mjpegbdec.c
> @@ -141,9 +141,10 @@ read_header:
>           av_log(avctx, AV_LOG_WARNING, "no picture\n");
>           return buf_size;
>       }
> -
>       av_frame_move_ref(rframe, s->picture_ptr);
>       s->got_picture = 0;
> +    if (avctx->skip_frame == AVDISCARD_ALL)
> +        return AVERROR(EAGAIN);

Decode callback decoders don't return EAGAIN. If they don't generate a 
frame but there was no error, then they return the amount of bytes 
consumed while ensuring got_frame is 0.

>       *got_frame = 1;
>   
>       if (!s->lossless && avctx->debug & FF_DEBUG_QP) {
_______________________________________________
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] avformat/mxfdec: Reduce overflows in essence_length computation

[Michael Niedermayer]

Fixes: signed integer overflow: -3741319169 - 9223372036823449370 cannot be represented in type 'long'
Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-513039428681728

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

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index e6118e141d..42109cb43a 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -100,7 +100,7 @@ typedef struct MXFPartition {
     uint64_t previous_partition;
     int index_sid;
     int body_sid;
-    int64_t this_partition;
+    uint64_t this_partition;
     int64_t essence_offset;         ///< absolute offset of essence
     int64_t essence_length;
     int32_t kag_size;
@@ -3519,8 +3519,12 @@ static void mxf_compute_essence_containers(AVFormatContext *s)
             p->essence_offset = p->first_essence_klv.offset;
 
             /* essence container spans to the next partition */
-            if (x < mxf->partitions_count - 1)
-                p->essence_length = mxf->partitions[x+1].this_partition - p->essence_offset;
+            if (x < mxf->partitions_count - 1) {
+                if (mxf->partitions[x+1].this_partition < p->essence_offset) {
+                    p->essence_length = -1;
+                } else
+                    p->essence_length = mxf->partitions[x+1].this_partition - p->essence_offset;
+            }
 
             if (p->essence_length < 0) {
                 /* next ThisPartition < essence_offset */
-- 
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] avformat/mxfdec: Reduce overflows in essence_length computation

[Tomas Härdin]

sön 2022-11-13 klockan 00:44 +0100 skrev Michael Niedermayer:
> Fixes: signed integer overflow: -3741319169 - 9223372036823449370
> cannot be represented in type 'long'
> Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> 513039428681728
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/mxfdec.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index e6118e141d..42109cb43a 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -100,7 +100,7 @@ typedef struct MXFPartition {
>      uint64_t previous_partition;
>      int index_sid;
>      int body_sid;
> -    int64_t this_partition;
> +    uint64_t this_partition;
>      int64_t essence_offset;         ///< absolute offset of essence
>      int64_t essence_length;
>      int32_t kag_size;
> @@ -3519,8 +3519,12 @@ static void
> mxf_compute_essence_containers(AVFormatContext *s)
>              p->essence_offset = p->first_essence_klv.offset;
>  
>              /* essence container spans to the next partition */
> -            if (x < mxf->partitions_count - 1)
> -                p->essence_length = mxf-
> >partitions[x+1].this_partition - p->essence_offset;
> +            if (x < mxf->partitions_count - 1) {
> +                if (mxf->partitions[x+1].this_partition < p-
> >essence_offset) {
> +                    p->essence_length = -1;
> +                } else
> +                    p->essence_length = mxf-
> >partitions[x+1].this_partition - p->essence_offset;
> +            }

A better solution might be to record the actual offset of the
partitions rather than relying on ThisPartition. Then we can guarantee
that they are strictly increasing.

/Tomas

_______________________________________________
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] avformat/mxfdec: Reduce overflows in essence_length computation

[Michael Niedermayer]

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

On Wed, Nov 16, 2022 at 12:49:10PM +0100, Tomas Härdin wrote:
> sön 2022-11-13 klockan 00:44 +0100 skrev Michael Niedermayer:
> > Fixes: signed integer overflow: -3741319169 - 9223372036823449370
> > cannot be represented in type 'long'
> > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> > 513039428681728
> > 
> > Found-by: continuous fuzzing process 
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/mxfdec.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > index e6118e141d..42109cb43a 100644
> > --- a/libavformat/mxfdec.c
> > +++ b/libavformat/mxfdec.c
> > @@ -100,7 +100,7 @@ typedef struct MXFPartition {
> >      uint64_t previous_partition;
> >      int index_sid;
> >      int body_sid;
> > -    int64_t this_partition;
> > +    uint64_t this_partition;
> >      int64_t essence_offset;         ///< absolute offset of essence
> >      int64_t essence_length;
> >      int32_t kag_size;
> > @@ -3519,8 +3519,12 @@ static void
> > mxf_compute_essence_containers(AVFormatContext *s)
> >              p->essence_offset = p->first_essence_klv.offset;
> >  
> >              /* essence container spans to the next partition */
> > -            if (x < mxf->partitions_count - 1)
> > -                p->essence_length = mxf-
> > >partitions[x+1].this_partition - p->essence_offset;
> > +            if (x < mxf->partitions_count - 1) {
> > +                if (mxf->partitions[x+1].this_partition < p-
> > >essence_offset) {
> > +                    p->essence_length = -1;
> > +                } else
> > +                    p->essence_length = mxf-
> > >partitions[x+1].this_partition - p->essence_offset;
> > +            }
> 
> A better solution might be to record the actual offset of the
> partitions rather than relying on ThisPartition. Then we can guarantee
> that they are strictly increasing.

do you mean that pack_ofs could be used here ?
(this is within 8 byte of this_partition) for the files i checked
or something else ?

but as pack_ofs is signed that either means dealing with potentially negative
or changing it to uint64_t or adding a new field and avio_tell() somewhere?

thx

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

It is a danger to trust the dream we wish for rather than
the science we have, -- Dr. Kenneth Brown

[-- 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 3/3] avformat/mxfdec: Reduce overflows in essence_length computation

[Tomas Härdin]

fre 2022-12-23 klockan 00:25 +0100 skrev Michael Niedermayer:
> On Wed, Nov 16, 2022 at 12:49:10PM +0100, Tomas Härdin wrote:
> > sön 2022-11-13 klockan 00:44 +0100 skrev Michael Niedermayer:
> > > Fixes: signed integer overflow: -3741319169 - 9223372036823449370
> > > cannot be represented in type 'long'
> > > Fixes: 51896/clusterfuzz-testcase-minimized-
> > > ffmpeg_dem_MXF_fuzzer-
> > > 513039428681728
> > > 
> > > Found-by: continuous fuzzing process 
> > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/mxfdec.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index e6118e141d..42109cb43a 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -100,7 +100,7 @@ typedef struct MXFPartition {
> > >      uint64_t previous_partition;
> > >      int index_sid;
> > >      int body_sid;
> > > -    int64_t this_partition;
> > > +    uint64_t this_partition;
> > >      int64_t essence_offset;         ///< absolute offset of
> > > essence
> > >      int64_t essence_length;
> > >      int32_t kag_size;
> > > @@ -3519,8 +3519,12 @@ static void
> > > mxf_compute_essence_containers(AVFormatContext *s)
> > >              p->essence_offset = p->first_essence_klv.offset;
> > >  
> > >              /* essence container spans to the next partition */
> > > -            if (x < mxf->partitions_count - 1)
> > > -                p->essence_length = mxf-
> > > > partitions[x+1].this_partition - p->essence_offset;
> > > +            if (x < mxf->partitions_count - 1) {
> > > +                if (mxf->partitions[x+1].this_partition < p-
> > > > essence_offset) {
> > > +                    p->essence_length = -1;
> > > +                } else
> > > +                    p->essence_length = mxf-
> > > > partitions[x+1].this_partition - p->essence_offset;
> > > +            }
> > 
> > A better solution might be to record the actual offset of the
> > partitions rather than relying on ThisPartition. Then we can
> > guarantee
> > that they are strictly increasing.
> 
> do you mean that pack_ofs could be used here ?

this_partition should be == pack_ofs - run_in. I think we can just nuke
this_partition in the struct and use pack_ofs - run_in everywhere. Emit
a warning or bail out if any partition does not obey this contraint,
since such files have likely been written by bad muxers.


> but as pack_ofs is signed that either means dealing with potentially
> negative
> or changing it to uint64_t or adding a new field and avio_tell()
> somewhere?

pack_ofs can only be negative in case of error, in which case we should
bail out anyway. Something must be very wrong if lavf can't avio_tell()

For every partition, pack_ofs >= run_in. this_partition >= 0, and in
particular this_partition == 0 for the very first partition.

This is reminding me that I had some finer points to fix related to
run_in and deleting the superfluous sync stuff that's currently in the
code. Baptiste is fine with that, as it's a relic from when mxfdec
didn't have proper seek support.

/Tomas

_______________________________________________
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] avformat/mxfdec: Reduce overflows in essence_length computation

[Michael Niedermayer]

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

On Fri, Dec 23, 2022 at 12:47:02PM +0100, Tomas Härdin wrote:
> fre 2022-12-23 klockan 00:25 +0100 skrev Michael Niedermayer:
> > On Wed, Nov 16, 2022 at 12:49:10PM +0100, Tomas Härdin wrote:
> > > sön 2022-11-13 klockan 00:44 +0100 skrev Michael Niedermayer:
> > > > Fixes: signed integer overflow: -3741319169 - 9223372036823449370
> > > > cannot be represented in type 'long'
> > > > Fixes: 51896/clusterfuzz-testcase-minimized-
> > > > ffmpeg_dem_MXF_fuzzer-
> > > > 513039428681728
> > > > 
> > > > Found-by: continuous fuzzing process 
> > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavformat/mxfdec.c | 10 +++++++---
> > > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > > index e6118e141d..42109cb43a 100644
> > > > --- a/libavformat/mxfdec.c
> > > > +++ b/libavformat/mxfdec.c
> > > > @@ -100,7 +100,7 @@ typedef struct MXFPartition {
> > > >      uint64_t previous_partition;
> > > >      int index_sid;
> > > >      int body_sid;
> > > > -    int64_t this_partition;
> > > > +    uint64_t this_partition;
> > > >      int64_t essence_offset;         ///< absolute offset of
> > > > essence
> > > >      int64_t essence_length;
> > > >      int32_t kag_size;
> > > > @@ -3519,8 +3519,12 @@ static void
> > > > mxf_compute_essence_containers(AVFormatContext *s)
> > > >              p->essence_offset = p->first_essence_klv.offset;
> > > >  
> > > >              /* essence container spans to the next partition */
> > > > -            if (x < mxf->partitions_count - 1)
> > > > -                p->essence_length = mxf-
> > > > > partitions[x+1].this_partition - p->essence_offset;
> > > > +            if (x < mxf->partitions_count - 1) {
> > > > +                if (mxf->partitions[x+1].this_partition < p-
> > > > > essence_offset) {
> > > > +                    p->essence_length = -1;
> > > > +                } else
> > > > +                    p->essence_length = mxf-
> > > > > partitions[x+1].this_partition - p->essence_offset;
> > > > +            }
> > > 
> > > A better solution might be to record the actual offset of the
> > > partitions rather than relying on ThisPartition. Then we can
> > > guarantee
> > > that they are strictly increasing.
> > 
> > do you mean that pack_ofs could be used here ?
> 
> this_partition should be == pack_ofs - run_in. I think we can just nuke
> this_partition in the struct and use pack_ofs - run_in everywhere. Emit
> a warning or bail out if any partition does not obey this contraint,
> since such files have likely been written by bad muxers.

ill post a patch doing that

thx

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

Elect your leaders based on what they did after the last election, not
based on what they say before an election.


[-- 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/lafdec: Check for EOF in header reading

[Michael Niedermayer]

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

On Sun, Nov 13, 2022 at 12:43:59AM +0100, Michael Niedermayer wrote:
> Fixes: OOM testcase
> Fixes: 51527/clusterfuzz-testcase-minimized-ffmpeg_dem_LAF_fuzzer-5453663505612800
> 
> OOM can still happen after this as an arbitrary sized block is allocated and read
> this would require a redesign or some limit on the sample rate.
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/lafdec.c | 3 +++
>  1 file changed, 3 insertions(+)

will apply

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

No great genius has ever existed without some touch of madness. -- 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".