| Message ID | 20201020205619.7939-4-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/11] avcodec/notchlc: Check uncompressed size against input for LZ4 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
Michael Niedermayer: > Fixes: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long' > Fixes: Timeout > Fixes: 26434/clusterfuzz-testcase-minimized-ffmpeg_dem_MV_fuzzer-5752845451919360 > Fixes: 26444/clusterfuzz-testcase-minimized-ffmpeg_dem_BINK_fuzzer-4697773380993024 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/utils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index e8335a601f..59d65a8092 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -253,7 +253,7 @@ int ffio_limit(AVIOContext *s, int size) > remaining= FFMAX(remaining, 0); > } > > - if (s->maxsize>= 0 && remaining+1 < size) { > + if (s->maxsize>= 0 && remaining < size - (int64_t)1) { > av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); > size = remaining+1; > } > The +1 here seems very weird. If remaining + 1 == size, the packet will not be truncated, despite there not being enough data left. The only reason for its existence I can think of is that it is designed to counter the potential -1 from s->maxsize = newsize - !newsize a few lines above. Yet this makes no sense: If newsize == 0, s->maxsize has just been set to -1 (indicating that future calls to ffio_limit() should not try again to get the filesize) and the second check above will not be reached at all. So how about just removing the +1 in all three lines above? - Andreas PS: And while just at it: You can also add a space to "s->maxsize>= 0".
Andreas Rheinhardt: > Michael Niedermayer: >> Fixes: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long' >> Fixes: Timeout >> Fixes: 26434/clusterfuzz-testcase-minimized-ffmpeg_dem_MV_fuzzer-5752845451919360 >> Fixes: 26444/clusterfuzz-testcase-minimized-ffmpeg_dem_BINK_fuzzer-4697773380993024 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavformat/utils.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/utils.c b/libavformat/utils.c >> index e8335a601f..59d65a8092 100644 >> --- a/libavformat/utils.c >> +++ b/libavformat/utils.c >> @@ -253,7 +253,7 @@ int ffio_limit(AVIOContext *s, int size) >> remaining= FFMAX(remaining, 0); >> } >> >> - if (s->maxsize>= 0 && remaining+1 < size) { >> + if (s->maxsize>= 0 && remaining < size - (int64_t)1) { >> av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); >> size = remaining+1; >> } >> > The +1 here seems very weird. If remaining + 1 == size, the packet will > not be truncated, despite there not being enough data left. > > The only reason for its existence I can think of is that it is designed > to counter the potential -1 from s->maxsize = newsize - !newsize a few > lines above. Yet this makes no sense: If newsize == 0, s->maxsize has > just been set to -1 (indicating that future calls to ffio_limit() should > not try again to get the filesize) and the second check above will not > be reached at all. > > So how about just removing the +1 in all three lines above? > > - Andreas > > PS: And while just at it: You can also add a space to "s->maxsize>= 0". > On second look (seeing "remaining ?") this seems to be done to make sure not to truncate to zero in order to always read something at all. So how about this here (untested): diff --git a/libavformat/utils.c b/libavformat/utils.c index e8335a601f..dd1b9e41c1 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -253,9 +253,11 @@ int ffio_limit(AVIOContext *s, int size) remaining= FFMAX(remaining, 0); } - if (s->maxsize>= 0 && remaining+1 < size) { - av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); - size = remaining+1; + if (s->maxsize >= 0 && remaining < size && size > 1) { + av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, + "Truncating packet of size %d to %"PRId64"\n", + size, remaining + !remaining); + size = remaining + !remaining; } } return size; - Andreas
On Wed, Oct 21, 2020 at 08:47:22AM +0200, Andreas Rheinhardt wrote: > Andreas Rheinhardt: > > Michael Niedermayer: > >> Fixes: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long' > >> Fixes: Timeout > >> Fixes: 26434/clusterfuzz-testcase-minimized-ffmpeg_dem_MV_fuzzer-5752845451919360 > >> Fixes: 26444/clusterfuzz-testcase-minimized-ffmpeg_dem_BINK_fuzzer-4697773380993024 > >> > >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> --- > >> libavformat/utils.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/libavformat/utils.c b/libavformat/utils.c > >> index e8335a601f..59d65a8092 100644 > >> --- a/libavformat/utils.c > >> +++ b/libavformat/utils.c > >> @@ -253,7 +253,7 @@ int ffio_limit(AVIOContext *s, int size) > >> remaining= FFMAX(remaining, 0); > >> } > >> > >> - if (s->maxsize>= 0 && remaining+1 < size) { > >> + if (s->maxsize>= 0 && remaining < size - (int64_t)1) { > >> av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); > >> size = remaining+1; > >> } > >> > > The +1 here seems very weird. If remaining + 1 == size, the packet will > > not be truncated, despite there not being enough data left. > > > > The only reason for its existence I can think of is that it is designed > > to counter the potential -1 from s->maxsize = newsize - !newsize a few > > lines above. Yet this makes no sense: If newsize == 0, s->maxsize has > > just been set to -1 (indicating that future calls to ffio_limit() should > > not try again to get the filesize) and the second check above will not > > be reached at all. > > > > So how about just removing the +1 in all three lines above? > > > > - Andreas > > > > PS: And while just at it: You can also add a space to "s->maxsize>= 0". > > > On second look (seeing "remaining ?") this seems to be done to make sure > not to truncate to zero in order to always read something at all. So > how about this here (untested): > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index e8335a601f..dd1b9e41c1 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -253,9 +253,11 @@ int ffio_limit(AVIOContext *s, int size) > remaining= FFMAX(remaining, 0); > } > > - if (s->maxsize>= 0 && remaining+1 < size) { > - av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, > "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); > - size = remaining+1; > + if (s->maxsize >= 0 && remaining < size && size > 1) { > + av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, > + "Truncating packet of size %d to %"PRId64"\n", > + size, remaining + !remaining); > + size = remaining + !remaining; > } > } > return size; LGTM, please apply thx [...]
On Fri, Oct 30, 2020 at 09:24:15PM +0100, Michael Niedermayer wrote: > On Wed, Oct 21, 2020 at 08:47:22AM +0200, Andreas Rheinhardt wrote: > > Andreas Rheinhardt: > > > Michael Niedermayer: > > >> Fixes: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long' > > >> Fixes: Timeout > > >> Fixes: 26434/clusterfuzz-testcase-minimized-ffmpeg_dem_MV_fuzzer-5752845451919360 > > >> Fixes: 26444/clusterfuzz-testcase-minimized-ffmpeg_dem_BINK_fuzzer-4697773380993024 > > >> > > >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > >> --- > > >> libavformat/utils.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/libavformat/utils.c b/libavformat/utils.c > > >> index e8335a601f..59d65a8092 100644 > > >> --- a/libavformat/utils.c > > >> +++ b/libavformat/utils.c > > >> @@ -253,7 +253,7 @@ int ffio_limit(AVIOContext *s, int size) > > >> remaining= FFMAX(remaining, 0); > > >> } > > >> > > >> - if (s->maxsize>= 0 && remaining+1 < size) { > > >> + if (s->maxsize>= 0 && remaining < size - (int64_t)1) { > > >> av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); > > >> size = remaining+1; > > >> } > > >> > > > The +1 here seems very weird. If remaining + 1 == size, the packet will > > > not be truncated, despite there not being enough data left. > > > > > > The only reason for its existence I can think of is that it is designed > > > to counter the potential -1 from s->maxsize = newsize - !newsize a few > > > lines above. Yet this makes no sense: If newsize == 0, s->maxsize has > > > just been set to -1 (indicating that future calls to ffio_limit() should > > > not try again to get the filesize) and the second check above will not > > > be reached at all. > > > > > > So how about just removing the +1 in all three lines above? > > > > > > - Andreas > > > > > > PS: And while just at it: You can also add a space to "s->maxsize>= 0". > > > > > On second look (seeing "remaining ?") this seems to be done to make sure > > not to truncate to zero in order to always read something at all. So > > how about this here (untested): > > > > diff --git a/libavformat/utils.c b/libavformat/utils.c > > index e8335a601f..dd1b9e41c1 100644 > > --- a/libavformat/utils.c > > +++ b/libavformat/utils.c > > @@ -253,9 +253,11 @@ int ffio_limit(AVIOContext *s, int size) > > remaining= FFMAX(remaining, 0); > > } > > > > - if (s->maxsize>= 0 && remaining+1 < size) { > > - av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, > > "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); > > - size = remaining+1; > > + if (s->maxsize >= 0 && remaining < size && size > 1) { > > + av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, > > + "Truncating packet of size %d to %"PRId64"\n", > > + size, remaining + !remaining); > > + size = remaining + !remaining; > > } > > } > > return size; > > LGTM, please apply ping [...]
On Sat, Nov 28, 2020 at 09:51:44PM +0100, Michael Niedermayer wrote: > On Fri, Oct 30, 2020 at 09:24:15PM +0100, Michael Niedermayer wrote: > > On Wed, Oct 21, 2020 at 08:47:22AM +0200, Andreas Rheinhardt wrote: > > > Andreas Rheinhardt: > > > > Michael Niedermayer: > > > >> Fixes: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long' > > > >> Fixes: Timeout > > > >> Fixes: 26434/clusterfuzz-testcase-minimized-ffmpeg_dem_MV_fuzzer-5752845451919360 > > > >> Fixes: 26444/clusterfuzz-testcase-minimized-ffmpeg_dem_BINK_fuzzer-4697773380993024 > > > >> > > > >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > >> --- > > > >> libavformat/utils.c | 2 +- > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >> > > > >> diff --git a/libavformat/utils.c b/libavformat/utils.c > > > >> index e8335a601f..59d65a8092 100644 > > > >> --- a/libavformat/utils.c > > > >> +++ b/libavformat/utils.c > > > >> @@ -253,7 +253,7 @@ int ffio_limit(AVIOContext *s, int size) > > > >> remaining= FFMAX(remaining, 0); > > > >> } > > > >> > > > >> - if (s->maxsize>= 0 && remaining+1 < size) { > > > >> + if (s->maxsize>= 0 && remaining < size - (int64_t)1) { > > > >> av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); > > > >> size = remaining+1; > > > >> } > > > >> > > > > The +1 here seems very weird. If remaining + 1 == size, the packet will > > > > not be truncated, despite there not being enough data left. > > > > > > > > The only reason for its existence I can think of is that it is designed > > > > to counter the potential -1 from s->maxsize = newsize - !newsize a few > > > > lines above. Yet this makes no sense: If newsize == 0, s->maxsize has > > > > just been set to -1 (indicating that future calls to ffio_limit() should > > > > not try again to get the filesize) and the second check above will not > > > > be reached at all. > > > > > > > > So how about just removing the +1 in all three lines above? > > > > > > > > - Andreas > > > > > > > > PS: And while just at it: You can also add a space to "s->maxsize>= 0". > > > > > > > On second look (seeing "remaining ?") this seems to be done to make sure > > > not to truncate to zero in order to always read something at all. So > > > how about this here (untested): > > > > > > diff --git a/libavformat/utils.c b/libavformat/utils.c > > > index e8335a601f..dd1b9e41c1 100644 > > > --- a/libavformat/utils.c > > > +++ b/libavformat/utils.c > > > @@ -253,9 +253,11 @@ int ffio_limit(AVIOContext *s, int size) > > > remaining= FFMAX(remaining, 0); > > > } > > > > > > - if (s->maxsize>= 0 && remaining+1 < size) { > > > - av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, > > > "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); > > > - size = remaining+1; > > > + if (s->maxsize >= 0 && remaining < size && size > 1) { > > > + av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, > > > + "Truncating packet of size %d to %"PRId64"\n", > > > + size, remaining + !remaining); > > > + size = remaining + !remaining; > > > } > > > } > > > return size; > > > > LGTM, please apply > > ping This affects quite a few files, so i may apply this or something else myself if you dont apply anything and dont comment thx [...]
Michael Niedermayer: > On Sat, Nov 28, 2020 at 09:51:44PM +0100, Michael Niedermayer wrote: >> On Fri, Oct 30, 2020 at 09:24:15PM +0100, Michael Niedermayer wrote: >>> On Wed, Oct 21, 2020 at 08:47:22AM +0200, Andreas Rheinhardt wrote: >>>> Andreas Rheinhardt: >>>>> Michael Niedermayer: >>>>>> Fixes: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long' >>>>>> Fixes: Timeout >>>>>> Fixes: 26434/clusterfuzz-testcase-minimized-ffmpeg_dem_MV_fuzzer-5752845451919360 >>>>>> Fixes: 26444/clusterfuzz-testcase-minimized-ffmpeg_dem_BINK_fuzzer-4697773380993024 >>>>>> >>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>>>> --- >>>>>> libavformat/utils.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c >>>>>> index e8335a601f..59d65a8092 100644 >>>>>> --- a/libavformat/utils.c >>>>>> +++ b/libavformat/utils.c >>>>>> @@ -253,7 +253,7 @@ int ffio_limit(AVIOContext *s, int size) >>>>>> remaining= FFMAX(remaining, 0); >>>>>> } >>>>>> >>>>>> - if (s->maxsize>= 0 && remaining+1 < size) { >>>>>> + if (s->maxsize>= 0 && remaining < size - (int64_t)1) { >>>>>> av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); >>>>>> size = remaining+1; >>>>>> } >>>>>> >>>>> The +1 here seems very weird. If remaining + 1 == size, the packet will >>>>> not be truncated, despite there not being enough data left. >>>>> >>>>> The only reason for its existence I can think of is that it is designed >>>>> to counter the potential -1 from s->maxsize = newsize - !newsize a few >>>>> lines above. Yet this makes no sense: If newsize == 0, s->maxsize has >>>>> just been set to -1 (indicating that future calls to ffio_limit() should >>>>> not try again to get the filesize) and the second check above will not >>>>> be reached at all. >>>>> >>>>> So how about just removing the +1 in all three lines above? >>>>> >>>>> - Andreas >>>>> >>>>> PS: And while just at it: You can also add a space to "s->maxsize>= 0". >>>>> >>>> On second look (seeing "remaining ?") this seems to be done to make sure >>>> not to truncate to zero in order to always read something at all. So >>>> how about this here (untested): >>>> >>>> diff --git a/libavformat/utils.c b/libavformat/utils.c >>>> index e8335a601f..dd1b9e41c1 100644 >>>> --- a/libavformat/utils.c >>>> +++ b/libavformat/utils.c >>>> @@ -253,9 +253,11 @@ int ffio_limit(AVIOContext *s, int size) >>>> remaining= FFMAX(remaining, 0); >>>> } >>>> >>>> - if (s->maxsize>= 0 && remaining+1 < size) { >>>> - av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, >>>> "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); >>>> - size = remaining+1; >>>> + if (s->maxsize >= 0 && remaining < size && size > 1) { >>>> + av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, >>>> + "Truncating packet of size %d to %"PRId64"\n", >>>> + size, remaining + !remaining); >>>> + size = remaining + !remaining; >>>> } >>>> } >>>> return size; >>> >>> LGTM, please apply >> >> ping > > This affects quite a few files, so i may apply this or something else myself > if you dont apply anything and dont comment > > thx > > [...] > Applied the above version. Sorry for the delay. - Andreas
diff --git a/libavformat/utils.c b/libavformat/utils.c index e8335a601f..59d65a8092 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -253,7 +253,7 @@ int ffio_limit(AVIOContext *s, int size) remaining= FFMAX(remaining, 0); } - if (s->maxsize>= 0 && remaining+1 < size) { + if (s->maxsize>= 0 && remaining < size - (int64_t)1) { av_log(NULL, remaining ? AV_LOG_ERROR : AV_LOG_DEBUG, "Truncating packet of size %d to %"PRId64"\n", size, remaining+1); size = remaining+1; }
Fixes: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long' Fixes: Timeout Fixes: 26434/clusterfuzz-testcase-minimized-ffmpeg_dem_MV_fuzzer-5752845451919360 Fixes: 26444/clusterfuzz-testcase-minimized-ffmpeg_dem_BINK_fuzzer-4697773380993024 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)