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

Submitted by Michael Niedermayer on June 4, 2017, 12:25 a.m.

Details

Message ID 20170604002546.5707-2-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer June 4, 2017, 12:25 a.m.
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/utils.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

James Almer June 4, 2017, 12:47 a.m.
On 6/3/2017 9:25 PM, Michael Niedermayer wrote:
> Signed-off-by: Michael Niedermayer <michael@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?
Michael Niedermayer June 4, 2017, 12:55 a.m.
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@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


[...]
James Almer June 4, 2017, 2:11 a.m.
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@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
>
Nicolas George June 4, 2017, 10:32 a.m.
Le quintidi 15 prairial, an CCXXV, James Almer a écrit :
> On 6/3/2017 9:25 PM, Michael Niedermayer wrote:
> 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...

This kind of case, I call "obviously invalid use of the API", and I
think in these cases, an immediate crash is way better than an error
code. The crash is already what happens here, since par and codec are
immediately dereferenced. An av_assert0() would make it clearer.

The same goes for every cases where passing NULL makes no sense, passing
an invalid value where a small set of enumerated values / flags is
expected (42 instead of SEEK_SET is an obvious example), or calling a
amath function outside a simple interval.

We may want to introduce something like av_assert_api() to have the
failure message tell explicitly that the crash is caused by a bug in the
application rather than the library.

Regards,
Ronald S. Bultje June 4, 2017, 11:15 a.m.
Hi,

On Sun, Jun 4, 2017 at 6:32 AM, Nicolas George <george@nsup.org> wrote:

> Le quintidi 15 prairial, an CCXXV, James Almer a écrit :
> > On 6/3/2017 9:25 PM, Michael Niedermayer wrote:
> > 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...
>
> This kind of case, I call "obviously invalid use of the API", and I
> think in these cases, an immediate crash is way better than an error
> code. The crash is already what happens here, since par and codec are
> immediately dereferenced. An av_assert0() would make it clearer.
>
> The same goes for every cases where passing NULL makes no sense, passing
> an invalid value where a small set of enumerated values / flags is
> expected (42 instead of SEEK_SET is an obvious example), or calling a
> amath function outside a simple interval.
>
> We may want to introduce something like av_assert_api() to have the
> failure message tell explicitly that the crash is caused by a bug in the
> application rather than the library.


This reminds me of g_return_val_if_fail in glib, and in some situations
it's quite sensible and helpful.

Ronald
Nicolas George June 5, 2017, 5:20 p.m.
Le sextidi 16 prairial, an CCXXV, Ronald S. Bultje a écrit :
> This reminds me of g_return_val_if_fail in glib, and in some situations
> it's quite sensible and helpful.

I think so too.

But g_return_val_if_fail() has a flaw: it prints a warning and returns,
at least with default builds of the library. A lot of applications
developers ignore these warnings when they have no obvious adverse
drawbacks. A crash is better IMHO.

Note that I am only advocating a crash for invalid uses of the API that
are obvious and easy to check, like:

	* @param foo  pointer to return value, must not be NULL

because it is easy for the application to wrap it and make the test:

	int frobnicate_or_bail(type *foo) {
	    if (foo == NULL) {
		warn("I'm afraid I can't do that");
		return ERROR_INVALID;
	    }
	    return frobnicate(foo);
	}

My main argument is that it is almost always unnecessary: when the
argument to a function must not be NULL, it is usually because the
natural way of calling the function yields a non-NULL pointer. For
example, in this particular case, the natural way of calling the
function is probably "ret = frobnicate(&value);".

The same goes for seek: when you call "seek(fd, pos, SEEK_SET);", you do
not need to check that SEEK_SET is a valid value: it is. I would even
argue that fd should get the same treatment, since an application doing
things on a fd without knowing if it is valid is broken and may
overwrite precious files (crashing will prevent that). On the other
hand, the application cannot easily check if fd is seekable or if pos is
valid: these conditions must cause an error return code, not a crash.

Regards,

Patch hide | download patch | download mbox

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)