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

[Michael Niedermayer]

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


> 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


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170604/b0bfe7a8/attachment.sig>