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

[Andreas Rheinhardt]

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 at 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