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

[Andreas Rheinhardt]

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