diff mbox series

[FFmpeg-devel,5/7] avformat/mxfdec: More offset_temp checks

Message ID 20240912233337.2444412-5-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/7] avformat/mov_chan: Check for FF_SANE_NB_CHANNELS | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Michael Niedermayer Sept. 12, 2024, 11:33 p.m. UTC
Fixes: signed integer overflow: 9223372036854775807 - -1927491430256034080 cannot be represented in type 'long'
Fixes: 70607/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-5282235077951488

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 | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Tomas Härdin Sept. 15, 2024, 6:29 p.m. UTC | #1
fre 2024-09-13 klockan 01:33 +0200 skrev Michael Niedermayer:
> Fixes: signed integer overflow: 9223372036854775807 - -
> 1927491430256034080 cannot be represented in type 'long'
> Fixes: 70607/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> 5282235077951488
> 
> 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 | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 8eae9f87afa..41281c5196d 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1924,6 +1924,11 @@ static int
> mxf_edit_unit_absolute_offset(MXFContext *mxf, MXFIndexTable *index_t
>              return mxf_absolute_bodysid_offset(mxf, index_table-
> >body_sid, offset_temp, offset_out, partition_out);
>          } else {
>              /* EditUnitByteCount == 0 for VBR indexes, which is fine
> since they use explicit StreamOffsets */
> +            if (s->edit_unit_byte_count &&  s->index_duration >
> INT64_MAX / s->edit_unit_byte_count ||
> +                s->edit_unit_byte_count * s->index_duration >
> INT64_MAX - offset_temp
> +            )
> +                return AVERROR_INVALIDDATA;

Looks OK

/Tomas
Tomas Härdin Sept. 15, 2024, 8:28 p.m. UTC | #2
fre 2024-09-13 klockan 01:33 +0200 skrev Michael Niedermayer:
> Fixes: signed integer overflow: 9223372036854775807 - -
> 1927491430256034080 cannot be represented in type 'long'
> Fixes: 70607/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> 5282235077951488
> 
> 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 | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 8eae9f87afa..41281c5196d 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1924,6 +1924,11 @@ static int
> mxf_edit_unit_absolute_offset(MXFContext *mxf, MXFIndexTable *index_t
>              return mxf_absolute_bodysid_offset(mxf, index_table-
> >body_sid, offset_temp, offset_out, partition_out);
>          } else {
>              /* EditUnitByteCount == 0 for VBR indexes, which is fine
> since they use explicit StreamOffsets */
> +            if (s->edit_unit_byte_count &&  s->index_duration >
> INT64_MAX / s->edit_unit_byte_count ||
> +                s->edit_unit_byte_count * s->index_duration >
> INT64_MAX - offset_temp
> +            )
> +                return AVERROR_INVALIDDATA;

Actually there's one minor issue: kdevelop warns about lack of
parenthesis around the && and ||

/Tomas
Tomas Härdin Sept. 16, 2024, 7:59 a.m. UTC | #3
sön 2024-09-15 klockan 22:28 +0200 skrev Tomas Härdin:
> fre 2024-09-13 klockan 01:33 +0200 skrev Michael Niedermayer:
> > Fixes: signed integer overflow: 9223372036854775807 - -
> > 1927491430256034080 cannot be represented in type 'long'
> > Fixes: 70607/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> > 5282235077951488
> > 
> > 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 | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > index 8eae9f87afa..41281c5196d 100644
> > --- a/libavformat/mxfdec.c
> > +++ b/libavformat/mxfdec.c
> > @@ -1924,6 +1924,11 @@ static int
> > mxf_edit_unit_absolute_offset(MXFContext *mxf, MXFIndexTable
> > *index_t
> >              return mxf_absolute_bodysid_offset(mxf, index_table-
> > > body_sid, offset_temp, offset_out, partition_out);
> >          } else {
> >              /* EditUnitByteCount == 0 for VBR indexes, which is
> > fine
> > since they use explicit StreamOffsets */
> > +            if (s->edit_unit_byte_count &&  s->index_duration >
> > INT64_MAX / s->edit_unit_byte_count ||
> > +                s->edit_unit_byte_count * s->index_duration >
> > INT64_MAX - offset_temp
> > +            )
> > +                return AVERROR_INVALIDDATA;
> 
> Actually there's one minor issue: kdevelop warns about lack of
> parenthesis around the && and ||

It seems where the () are makes little difference, but I'm guessing we
want them around the || terms simply because that saves a few cycles

/Tomas
Michael Niedermayer Sept. 18, 2024, 10:31 p.m. UTC | #4
On Mon, Sep 16, 2024 at 09:59:11AM +0200, Tomas Härdin wrote:
> sön 2024-09-15 klockan 22:28 +0200 skrev Tomas Härdin:
> > fre 2024-09-13 klockan 01:33 +0200 skrev Michael Niedermayer:
> > > Fixes: signed integer overflow: 9223372036854775807 - -
> > > 1927491430256034080 cannot be represented in type 'long'
> > > Fixes: 70607/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> > > 5282235077951488
> > > 
> > > 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 | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index 8eae9f87afa..41281c5196d 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -1924,6 +1924,11 @@ static int
> > > mxf_edit_unit_absolute_offset(MXFContext *mxf, MXFIndexTable
> > > *index_t
> > >              return mxf_absolute_bodysid_offset(mxf, index_table-
> > > > body_sid, offset_temp, offset_out, partition_out);
> > >          } else {
> > >              /* EditUnitByteCount == 0 for VBR indexes, which is
> > > fine
> > > since they use explicit StreamOffsets */
> > > +            if (s->edit_unit_byte_count &&  s->index_duration >
> > > INT64_MAX / s->edit_unit_byte_count ||
> > > +                s->edit_unit_byte_count * s->index_duration >
> > > INT64_MAX - offset_temp
> > > +            )
> > > +                return AVERROR_INVALIDDATA;
> > 
> > Actually there's one minor issue: kdevelop warns about lack of
> > parenthesis around the && and ||
> 
> It seems where the () are makes little difference, but I'm guessing we
> want them around the || terms simply because that saves a few cycles

will apply with it around the ||

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 8eae9f87afa..41281c5196d 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -1924,6 +1924,11 @@  static int mxf_edit_unit_absolute_offset(MXFContext *mxf, MXFIndexTable *index_t
             return mxf_absolute_bodysid_offset(mxf, index_table->body_sid, offset_temp, offset_out, partition_out);
         } else {
             /* EditUnitByteCount == 0 for VBR indexes, which is fine since they use explicit StreamOffsets */
+            if (s->edit_unit_byte_count &&  s->index_duration > INT64_MAX / s->edit_unit_byte_count ||
+                s->edit_unit_byte_count * s->index_duration > INT64_MAX - offset_temp
+            )
+                return AVERROR_INVALIDDATA;
+
             offset_temp += s->edit_unit_byte_count * s->index_duration;
         }
     }