diff mbox series

[FFmpeg-devel,04/11] avformat/utils: Move +1 to avoid overflow

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
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Michael Niedermayer Oct. 20, 2020, 8:56 p.m. UTC
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(-)

Comments

Andreas Rheinhardt Oct. 21, 2020, 6:29 a.m. UTC | #1
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 Oct. 21, 2020, 6:47 a.m. UTC | #2
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
Michael Niedermayer Oct. 30, 2020, 8:24 p.m. UTC | #3
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

[...]
Michael Niedermayer Nov. 28, 2020, 8:51 p.m. UTC | #4
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

[...]
diff mbox series

Patch

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;
         }