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

[James Almer]

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);
> +
>      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...

Also, unless the user calls av_max_alloc to set a value higher than
INT_MAX, shouldn't av_mallocz refuse to alloc these?