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 <mich...@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
