[FFmpeg-devel] [PATCH 2/3] avcodec/utils: Fix several integer overflows.

[James Almer]

On 6/3/2017 9:55 PM, Michael Niedermayer wrote:
> On Sat, Jun 03, 2017 at 09:47:32PM -0300, James Almer wrote:
>> On 6/3/2017 9:25 PM, Michael Niedermayer wrote:
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>>  libavcodec/utils.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>> index cde5849a41..feee7556ac 100644
>>> --- a/libavcodec/utils.c
>>> +++ b/libavcodec/utils.c
>>> @@ -2278,6 +2278,9 @@ void avcodec_parameters_free(AVCodecParameters **ppar)
>>>  
>>>  int avcodec_parameters_copy(AVCodecParameters *dst, const AVCodecParameters *src)
>>>  {
>>> +    if (src->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>>> +        return AVERROR(EINVAL);
>>> +

IMO, move this inside the extradata check right before the av_mallocz
call, like in the two functions below.

>>>      codec_parameters_reset(dst);
>>>      memcpy(dst, src, sizeof(*dst));
>>>  
>>> @@ -2341,6 +2344,8 @@ int avcodec_parameters_from_context(AVCodecParameters *par,
>>>      }
>>>  
>>>      if (codec->extradata) {
>>> +        if (codec->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>>> +            return AVERROR(EINVAL);
>>>          par->extradata = av_mallocz(codec->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>          if (!par->extradata)
>>>              return AVERROR(ENOMEM);
>>> @@ -2397,6 +2402,8 @@ int avcodec_parameters_to_context(AVCodecContext *codec,
>>>      }
>>>  
>>>      if (par->extradata) {
>>> +        if (par->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>>> +            return AVERROR(EINVAL);
>>>          av_freep(&codec->extradata);
>>>          codec->extradata = av_mallocz(par->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>          if (!codec->extradata)
>>>
>>
>> ERANGE? Or ENOMEM, the only error these functions are currently
>> returning. EINVAL for this situation with no way to log the reason of
>> the error is not very intuitive. The function is meant to copy the
>> fields from one struct to the other, not really validate said fields.
>>
>> I see EINVAL more fit as an error for example if src or dst are NULL,
>> something that would actually be an invalid argument.
>> We don't currently check that, for that matter. Maybe we should...
>>
> 
> do you prefer ERANGE or ENOMEM ?

Eh, go with ERANGE i guess.

> 
> 
>> Also, unless the user calls av_max_alloc to set a value higher than
>> INT_MAX, shouldn't av_mallocz refuse to alloc these?
> 
> the size is a int, the addition would overflow and be a undefined
> operation. I dont have a testcase that triggers this though
> 
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>