| Message ID | 20240616230831.912377-5-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/9] avcodec/targaenc: Allocate space for the palette | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
On 6/16/2024 8:08 PM, Michael Niedermayer wrote: > Fixes: signed integer overflow: 2314885530818453536 + 9151314442816847872 cannot be represented in type 'long' > Fixes: 68359/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-6571950311800832 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/mov.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 9016cd5ad08..46cbce98040 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -8131,7 +8131,9 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom) > } > for (int j = 0; j < extent_count; j++) { > if (rb_size(pb, &extent_offset, offset_size) < 0 || > - rb_size(pb, &extent_length, length_size) < 0) > + rb_size(pb, &extent_length, length_size) < 0 || > + base_offset < 0 || extent_offset < 0 || > + base_offset + (uint64_t)extent_offset > INT64_MAX) You can do the negative value check directly in rb_size() instead. And I'd prefer the other check to be (base_offset > INT64_MAX - extent_offset). > return AVERROR_INVALIDDATA; > if (offset_type == 1) > c->heif_item[i].is_idat_relative = 1;
Le 17 juin 2024 01:08:27 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc> a écrit : >Fixes: signed integer overflow: 2314885530818453536 + 9151314442816847872 cannot be represented in type 'long' >Fixes: 68359/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-6571950311800832 > >Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >--- > libavformat/mov.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > >diff --git a/libavformat/mov.c b/libavformat/mov.c >index 9016cd5ad08..46cbce98040 100644 >--- a/libavformat/mov.c >+++ b/libavformat/mov.c >@@ -8131,7 +8131,9 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom) > } > for (int j = 0; j < extent_count; j++) { > if (rb_size(pb, &extent_offset, offset_size) < 0 || >- rb_size(pb, &extent_length, length_size) < 0) >+ rb_size(pb, &extent_length, length_size) < 0 || >+ base_offset < 0 || extent_offset < 0 || >+ base_offset + (uint64_t)extent_offset > INT64_MAX) Can we please stop with the bespoke arithmetic overflow checks and add dedicated helpers instead, similar to what GCC and C23 have? > return AVERROR_INVALIDDATA; > if (offset_type == 1) > c->heif_item[i].is_idat_relative = 1;
Rémi Denis-Courmont: > > > Le 17 juin 2024 01:08:27 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc> a écrit : >> Fixes: signed integer overflow: 2314885530818453536 + 9151314442816847872 cannot be represented in type 'long' >> Fixes: 68359/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-6571950311800832 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavformat/mov.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/libavformat/mov.c b/libavformat/mov.c >> index 9016cd5ad08..46cbce98040 100644 >> --- a/libavformat/mov.c >> +++ b/libavformat/mov.c >> @@ -8131,7 +8131,9 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom) >> } >> for (int j = 0; j < extent_count; j++) { >> if (rb_size(pb, &extent_offset, offset_size) < 0 || >> - rb_size(pb, &extent_length, length_size) < 0) >> + rb_size(pb, &extent_length, length_size) < 0 || >> + base_offset < 0 || extent_offset < 0 || >> + base_offset + (uint64_t)extent_offset > INT64_MAX) > > Can we please stop with the bespoke arithmetic overflow checks and add dedicated helpers instead, similar to what GCC and C23 have? > +1 >> return AVERROR_INVALIDDATA; >> if (offset_type == 1) >> c->heif_item[i].is_idat_relative = 1;
On 6/18/2024 4:07 AM, Rémi Denis-Courmont wrote: > > > Le 17 juin 2024 01:08:27 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc> a écrit : >> Fixes: signed integer overflow: 2314885530818453536 + 9151314442816847872 cannot be represented in type 'long' >> Fixes: 68359/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-6571950311800832 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavformat/mov.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/libavformat/mov.c b/libavformat/mov.c >> index 9016cd5ad08..46cbce98040 100644 >> --- a/libavformat/mov.c >> +++ b/libavformat/mov.c >> @@ -8131,7 +8131,9 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom) >> } >> for (int j = 0; j < extent_count; j++) { >> if (rb_size(pb, &extent_offset, offset_size) < 0 || >> - rb_size(pb, &extent_length, length_size) < 0) >> + rb_size(pb, &extent_length, length_size) < 0 || >> + base_offset < 0 || extent_offset < 0 || >> + base_offset + (uint64_t)extent_offset > INT64_MAX) > > Can we please stop with the bespoke arithmetic overflow checks and add dedicated helpers instead, similar to what GCC and C23 have? You mean the __builtin_*_overflow() ones? > >> return AVERROR_INVALIDDATA; >> if (offset_type == 1) >> c->heif_item[i].is_idat_relative = 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".
Le 19 juin 2024 14:34:59 GMT+02:00, James Almer <jamrial@gmail.com> a écrit : >On 6/18/2024 4:07 AM, Rémi Denis-Courmont wrote: >> >> >> Le 17 juin 2024 01:08:27 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc> a écrit : >>> Fixes: signed integer overflow: 2314885530818453536 + 9151314442816847872 cannot be represented in type 'long' >>> Fixes: 68359/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-6571950311800832 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavformat/mov.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavformat/mov.c b/libavformat/mov.c >>> index 9016cd5ad08..46cbce98040 100644 >>> --- a/libavformat/mov.c >>> +++ b/libavformat/mov.c >>> @@ -8131,7 +8131,9 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom) >>> } >>> for (int j = 0; j < extent_count; j++) { >>> if (rb_size(pb, &extent_offset, offset_size) < 0 || >>> - rb_size(pb, &extent_length, length_size) < 0) >>> + rb_size(pb, &extent_length, length_size) < 0 || >>> + base_offset < 0 || extent_offset < 0 || >>> + base_offset + (uint64_t)extent_offset > INT64_MAX) >> >> Can we please stop with the bespoke arithmetic overflow checks and add dedicated helpers instead, similar to what GCC and C23 have? > >You mean the __builtin_*_overflow() one? I'd rather the ckd_*() stuff but the differences are mostly stylistic. >>> return AVERROR_INVALIDDATA; >>> if (offset_type == 1) >>> c->heif_item[i].is_idat_relative = 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 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".
On Wed, Jun 19, 2024 at 03:08:58PM +0200, Rémi Denis-Courmont wrote: > > > Le 19 juin 2024 14:34:59 GMT+02:00, James Almer <jamrial@gmail.com> a écrit : > >On 6/18/2024 4:07 AM, Rémi Denis-Courmont wrote: > >> > >> > >> Le 17 juin 2024 01:08:27 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc> a écrit : > >>> Fixes: signed integer overflow: 2314885530818453536 + 9151314442816847872 cannot be represented in type 'long' > >>> Fixes: 68359/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-6571950311800832 > >>> > >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>> --- > >>> libavformat/mov.c | 4 +++- > >>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/libavformat/mov.c b/libavformat/mov.c > >>> index 9016cd5ad08..46cbce98040 100644 > >>> --- a/libavformat/mov.c > >>> +++ b/libavformat/mov.c > >>> @@ -8131,7 +8131,9 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom) > >>> } > >>> for (int j = 0; j < extent_count; j++) { > >>> if (rb_size(pb, &extent_offset, offset_size) < 0 || > >>> - rb_size(pb, &extent_length, length_size) < 0) > >>> + rb_size(pb, &extent_length, length_size) < 0 || > >>> + base_offset < 0 || extent_offset < 0 || > >>> + base_offset + (uint64_t)extent_offset > INT64_MAX) > >> > >> Can we please stop with the bespoke arithmetic overflow checks and add dedicated helpers instead, similar to what GCC and C23 have? > > > >You mean the __builtin_*_overflow() one? > > I'd rather the ckd_*() stuff but the differences are mostly stylistic. Whatever is used must be supported by all currently supported platforms that especially also includes past releases we backport things to. In practice that means continuing to use the classical way to check as well as our av_sat_addXY() stuff. We cannot backport things that depend on C23 as that was not a requirement in the past. So I also cannot use this in bug fixes. thx [...]
On 6/20/2024 7:54 PM, Michael Niedermayer wrote: > On Wed, Jun 19, 2024 at 03:08:58PM +0200, Rémi Denis-Courmont wrote: >> >> >> Le 19 juin 2024 14:34:59 GMT+02:00, James Almer <jamrial@gmail.com> a écrit : >>> On 6/18/2024 4:07 AM, Rémi Denis-Courmont wrote: >>>> >>>> >>>> Le 17 juin 2024 01:08:27 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc> a écrit : >>>>> Fixes: signed integer overflow: 2314885530818453536 + 9151314442816847872 cannot be represented in type 'long' >>>>> Fixes: 68359/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-6571950311800832 >>>>> >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>>> --- >>>>> libavformat/mov.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c >>>>> index 9016cd5ad08..46cbce98040 100644 >>>>> --- a/libavformat/mov.c >>>>> +++ b/libavformat/mov.c >>>>> @@ -8131,7 +8131,9 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom) >>>>> } >>>>> for (int j = 0; j < extent_count; j++) { >>>>> if (rb_size(pb, &extent_offset, offset_size) < 0 || >>>>> - rb_size(pb, &extent_length, length_size) < 0) >>>>> + rb_size(pb, &extent_length, length_size) < 0 || >>>>> + base_offset < 0 || extent_offset < 0 || >>>>> + base_offset + (uint64_t)extent_offset > INT64_MAX) >>>> >>>> Can we please stop with the bespoke arithmetic overflow checks and add dedicated helpers instead, similar to what GCC and C23 have? >>> >>> You mean the __builtin_*_overflow() one? >> >> I'd rather the ckd_*() stuff but the differences are mostly stylistic. > > Whatever is used must be supported by all currently supported platforms > that especially also includes past releases we backport things to. > > In practice that means continuing to use the classical way to check > as well as our av_sat_addXY() stuff. > > We cannot backport things that depend on C23 as that was not a requirement > in the past. So I also cannot use this in bug fixes. This change is ok for now (With my suggestion amended to it). We can add wrappers around the GCC builtins and C23 ckd_* macros later (With pure C fallbacks), and start using them for future changes. > > thx > > [...] > > > _______________________________________________ > 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".
diff --git a/libavformat/mov.c b/libavformat/mov.c index 9016cd5ad08..46cbce98040 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -8131,7 +8131,9 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom) } for (int j = 0; j < extent_count; j++) { if (rb_size(pb, &extent_offset, offset_size) < 0 || - rb_size(pb, &extent_length, length_size) < 0) + rb_size(pb, &extent_length, length_size) < 0 || + base_offset < 0 || extent_offset < 0 || + base_offset + (uint64_t)extent_offset > INT64_MAX) return AVERROR_INVALIDDATA; if (offset_type == 1) c->heif_item[i].is_idat_relative = 1;
Fixes: signed integer overflow: 2314885530818453536 + 9151314442816847872 cannot be represented in type 'long' Fixes: 68359/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-6571950311800832 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mov.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)