[FFmpeg-devel,1/3] 4xm: prevent overflow during bit rate calculation

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavformat/4xm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Hi,

On Tue, Dec 13, 2016 at 7:57 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:

> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavformat/4xm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
> index 8a50778..2758b69 100644
> --- a/libavformat/4xm.c
> +++ b/libavformat/4xm.c
> @@ -163,6 +163,12 @@ static int parse_strk(AVFormatContext *s,
>          return AVERROR_INVALIDDATA;
>      }
>
> +    if (fourxm->tracks[track].sample_rate > INT64_MAX /
> fourxm->tracks[track].bits / fourxm->tracks[track].channels) {
> +        av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation %d
> * %d * %d\n",
> +               fourxm->tracks[track].sample_rate,
> fourxm->tracks[track].bits, fourxm->tracks[track].channels);
> +        return AVERROR_INVALIDDATA;
> +    }


What is the functional effect of the overflow? Does it crash? Or is there
some other security issue?

Ronald

On 14.12.2016 02:01, Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, Dec 13, 2016 at 7:57 PM, Andreas Cadhalpun <
> andreas.cadhalpun@googlemail.com> wrote:
> 
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavformat/4xm.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
>> index 8a50778..2758b69 100644
>> --- a/libavformat/4xm.c
>> +++ b/libavformat/4xm.c
>> @@ -163,6 +163,12 @@ static int parse_strk(AVFormatContext *s,
>>          return AVERROR_INVALIDDATA;
>>      }
>>
>> +    if (fourxm->tracks[track].sample_rate > INT64_MAX /
>> fourxm->tracks[track].bits / fourxm->tracks[track].channels) {
>> +        av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation %d
>> * %d * %d\n",
>> +               fourxm->tracks[track].sample_rate,
>> fourxm->tracks[track].bits, fourxm->tracks[track].channels);
>> +        return AVERROR_INVALIDDATA;
>> +    }
> 
> 
> What is the functional effect of the overflow?

It is undefined behavior.

> Does it crash? Or is there some other security issue?

The most likely behavior is that bit_rate will contain a negative value,
which might cause problems later on, but I'm not aware of specific
security issues caused by this.

Best regards,
Andreas

Hi,

On Tue, Dec 13, 2016 at 8:21 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:

> On 14.12.2016 02:01, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Tue, Dec 13, 2016 at 7:57 PM, Andreas Cadhalpun <
> > andreas.cadhalpun@googlemail.com> wrote:
> >
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> >> ---
> >>  libavformat/4xm.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
> >> index 8a50778..2758b69 100644
> >> --- a/libavformat/4xm.c
> >> +++ b/libavformat/4xm.c
> >> @@ -163,6 +163,12 @@ static int parse_strk(AVFormatContext *s,
> >>          return AVERROR_INVALIDDATA;
> >>      }
> >>
> >> +    if (fourxm->tracks[track].sample_rate > INT64_MAX /
> >> fourxm->tracks[track].bits / fourxm->tracks[track].channels) {
> >> +        av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation
> %d
> >> * %d * %d\n",
> >> +               fourxm->tracks[track].sample_rate,
> >> fourxm->tracks[track].bits, fourxm->tracks[track].channels);
> >> +        return AVERROR_INVALIDDATA;
> >> +    }
> >
> >
> > What is the functional effect of the overflow?
>
> It is undefined behavior.
>
> > Does it crash? Or is there some other security issue?
>
> The most likely behavior is that bit_rate will contain a negative value,
> which might cause problems later on, but I'm not aware of specific
> security issues caused by this.


Not wanting to discourage you, but I wonder if there's really a point to
this...? I don't see how the user experience changes.

This isn't specifically intended at this patch, but rather at the sort of
rabbit hole this change might lead to, which would cause the code to be
uber-full of such checks, none of which really have any significance. But
maybe others disagree...

Ronald

On Wed, Dec 14, 2016 at 01:57:54AM +0100, Andreas Cadhalpun wrote:
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavformat/4xm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

LGTM

thx

[...]

On 14.12.2016 02:46, Ronald S. Bultje wrote:
> Not wanting to discourage you, but I wonder if there's really a point to
> this...?

These patches are prerequisites for enforcing the validity of codec
parameters [1].

> I don't see how the user experience changes.

Users won't see ffmpeg claiming nonsense bit rates like -1184293205235990 kb/s
anymore.

> This isn't specifically intended at this patch, but rather at the sort of
> rabbit hole this change might lead to,

I have a pretty good map of this rabbit hole (i.e. lots of samples triggering
UBSan errors) and one day I might try to dig it up, but for now I'm limiting
myself to the codec parameters.

> which would cause the code to be uber-full of such checks, none of which
> really have any significance. But maybe others disagree...

Not relying on undefined behavior is a significant improvement. And doing
these checks consequently where the values are set makes it possible
for other code to rely on their validity without further checks.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161026/c0151e90/attachment.patch

On 14.12.2016 03:54, Michael Niedermayer wrote:
> On Wed, Dec 14, 2016 at 01:57:54AM +0100, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavformat/4xm.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> LGTM

Pushed.

Best regards,
Andreas

Hi,

On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:

> On 14.12.2016 02:46, Ronald S. Bultje wrote:
> > Not wanting to discourage you, but I wonder if there's really a point to
> > this...?
>
> These patches are prerequisites for enforcing the validity of codec
> parameters [1].
>
> > I don't see how the user experience changes.
>
> Users won't see ffmpeg claiming nonsense bit rates like -1184293205235990
> kb/s
> anymore.


I don't think you understand my question.

- does this belong in 4xm.c? (Or in generic code? Or in the app?)
- should it return an error? (Or just clip parameters? Or ignore the
invalid value?)

> This isn't specifically intended at this patch, but rather at the sort of
> > rabbit hole this change might lead to,
>
> I have a pretty good map of this rabbit hole (i.e. lots of samples
> triggering
> UBSan errors) and one day I might try to dig it up, but for now I'm
> limiting
> myself to the codec parameters.


I'm not saying mplayer was great, but one of the principles I believe we
always copied from them was to try to play files to the best of our
abilities and not error out at the first convenience. This isn't just a
theoretical thing, a lot of people credited mplayer with playing utterly
broken AVI files that probably even ffmpeg rejects. What ffmpeg added on
top of that is to make a project maintainable by not being an utter
crapshoot.

This isn't 4xm.c-specific, this is a general philosophical question:
- should we error out?
- should this be in generic code if we're likely to repeat such checks all
over the place?

> which would cause the code to be uber-full of such checks, none of which
> > really have any significance. But maybe others disagree...
>
> Not relying on undefined behavior is a significant improvement. And doing
> these checks consequently where the values are set makes it possible
> for other code to rely on their validity without further checks.


Unless "UB" leads to actual bad behaviour, I personally don't necessarily
care. I'm very scared that when you go beyond codec parameters, and you
want to check for overflows all over the place, we'll never see the end of
it...

I'd appreciate if others could chime in here, I don't want to carry this
argument all by myself if nobody cares.

Ronald

On Thu, Dec 15, 2016 at 08:02:52AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
> andreas.cadhalpun@googlemail.com> wrote:
> 
> > On 14.12.2016 02:46, Ronald S. Bultje wrote:
> > > Not wanting to discourage you, but I wonder if there's really a point to
> > > this...?
> >
> > These patches are prerequisites for enforcing the validity of codec
> > parameters [1].
> >
> > > I don't see how the user experience changes.
> >
> > Users won't see ffmpeg claiming nonsense bit rates like -1184293205235990
> > kb/s
> > anymore.
> 
> 
> I don't think you understand my question.
> 
> - does this belong in 4xm.c? (Or in generic code? Or in the app?)
> - should it return an error? (Or just clip parameters? Or ignore the
> invalid value?)
> 
> > This isn't specifically intended at this patch, but rather at the sort of
> > > rabbit hole this change might lead to,
> >
> > I have a pretty good map of this rabbit hole (i.e. lots of samples
> > triggering
> > UBSan errors) and one day I might try to dig it up, but for now I'm
> > limiting
> > myself to the codec parameters.
> 
> 
> I'm not saying mplayer was great, but one of the principles I believe we
> always copied from them was to try to play files to the best of our
> abilities and not error out at the first convenience. This isn't just a
> theoretical thing, a lot of people credited mplayer with playing utterly
> broken AVI files that probably even ffmpeg rejects. What ffmpeg added on
> top of that is to make a project maintainable by not being an utter
> crapshoot.
> 
> This isn't 4xm.c-specific, this is a general philosophical question:
> - should we error out?
> - should this be in generic code if we're likely to repeat such checks all
> over the place?
> 
> > which would cause the code to be uber-full of such checks, none of which
> > > really have any significance. But maybe others disagree...
> >
> > Not relying on undefined behavior is a significant improvement. And doing
> > these checks consequently where the values are set makes it possible
> > for other code to rely on their validity without further checks.
> 
> 
> Unless "UB" leads to actual bad behaviour, I personally don't necessarily
> care. I'm very scared that when you go beyond codec parameters, and you
> want to check for overflows all over the place, we'll never see the end of
> it...
> 

> I'd appreciate if others could chime in here, I don't want to carry this
> argument all by myself if nobody cares.

as you are asking for others oppinion
valid C code must not trigger undefined behavior

undefined behavior means anything can happen and this is not guranteed
to be limited to the value a operation produces.
if you write a*b the compiler can assume that a and b are small
enough for this not to overflow, it can use this to simplify checks
before and after the operation.
A silly example would be a*b/b != a  (with signed a and b) which any
good compiler should replace by 0

We have seen bugs from undefined behavior like strict aliassing
violations were the compiler assumed that things wouldnt be accessed
as they had a different type ...

I think we should fix all instances of UB.
If we find cases that we cannot fix without slowdown or without
making the code hard to maintain by the people who activly work on it
then iam not against making exceptions. But in my oppinion in
general UB should be fixed

[...]

Hi,

On Thu, Dec 15, 2016 at 9:28 AM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> On Thu, Dec 15, 2016 at 08:02:52AM -0500, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
> > andreas.cadhalpun@googlemail.com> wrote:
> >
> > > On 14.12.2016 02:46, Ronald S. Bultje wrote:
> > > > Not wanting to discourage you, but I wonder if there's really a
> point to
> > > > this...?
> > >
> > > These patches are prerequisites for enforcing the validity of codec
> > > parameters [1].
> > >
> > > > I don't see how the user experience changes.
> > >
> > > Users won't see ffmpeg claiming nonsense bit rates like
> -1184293205235990
> > > kb/s
> > > anymore.
> >
> >
> > I don't think you understand my question.
> >
> > - does this belong in 4xm.c? (Or in generic code? Or in the app?)
> > - should it return an error? (Or just clip parameters? Or ignore the
> > invalid value?)
> >
> > > This isn't specifically intended at this patch, but rather at the sort
> of
> > > > rabbit hole this change might lead to,
> > >
> > > I have a pretty good map of this rabbit hole (i.e. lots of samples
> > > triggering
> > > UBSan errors) and one day I might try to dig it up, but for now I'm
> > > limiting
> > > myself to the codec parameters.
> >
> >
> > I'm not saying mplayer was great, but one of the principles I believe we
> > always copied from them was to try to play files to the best of our
> > abilities and not error out at the first convenience. This isn't just a
> > theoretical thing, a lot of people credited mplayer with playing utterly
> > broken AVI files that probably even ffmpeg rejects. What ffmpeg added on
> > top of that is to make a project maintainable by not being an utter
> > crapshoot.
> >
> > This isn't 4xm.c-specific, this is a general philosophical question:
> > - should we error out?
> > - should this be in generic code if we're likely to repeat such checks
> all
> > over the place?
> >
> > > which would cause the code to be uber-full of such checks, none of
> which
> > > > really have any significance. But maybe others disagree...
> > >
> > > Not relying on undefined behavior is a significant improvement. And
> doing
> > > these checks consequently where the values are set makes it possible
> > > for other code to rely on their validity without further checks.
> >
> >
> > Unless "UB" leads to actual bad behaviour, I personally don't necessarily
> > care. I'm very scared that when you go beyond codec parameters, and you
> > want to check for overflows all over the place, we'll never see the end
> of
> > it...
> >
>
> > I'd appreciate if others could chime in here, I don't want to carry this
> > argument all by myself if nobody cares.
>
> as you are asking for others oppinion
> valid C code must not trigger undefined behavior


So, I asked on IRC, here's 3 suggestions from Roger Combs:

- in case we want to be pedantic (I personally don't care, but I understand
other people are), it might make sense to just make these members
(channels, block_align, bitrate) unsigned. That removes the UB of the
overflow, and it fixes the negative number reporting in client apps for
bitrate, but you can still have positive crazy numbers and it doesn't
return an error.
- if for whatever reason some things cannot be done in generic code or by
changing the type (this should really cover most cases), and we want
specific overflow checks, then maybe we want to have some generic helper
macros that make them one-liners in decoders. This would return an error
along with fixing the UB.
- have overflow-safe multiplication functions instead of checking each
argument before doing a multiply, like __builtin_mul_overflow, and then
return INT64_MAX if it would overflow inside that function. This fixes
display of numbers in client applications and the UB, but without returning
an error.

What I want most - and this comment applies to all patches of this sort -
is to have something that we can all agree is OK for pretty much any
decoder out there without significant overhead in code (source - not
binary) size. This way, you have a template to work from and fix specific
instances without us having to argue over every single time you do a next
run with ubsan. I personally like suggestion (1), unsigned is a pretty good
type for things that shouldn't have negative values. WDYT?

Ronald

On Thu, Dec 15, 2016 at 03:57:57PM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Thu, Dec 15, 2016 at 9:28 AM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:
> 
> > On Thu, Dec 15, 2016 at 08:02:52AM -0500, Ronald S. Bultje wrote:
> > > Hi,
> > >
> > > On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
> > > andreas.cadhalpun@googlemail.com> wrote:
> > >
> > > > On 14.12.2016 02:46, Ronald S. Bultje wrote:
> > > > > Not wanting to discourage you, but I wonder if there's really a
> > point to
> > > > > this...?
> > > >
> > > > These patches are prerequisites for enforcing the validity of codec
> > > > parameters [1].
> > > >
> > > > > I don't see how the user experience changes.
> > > >
> > > > Users won't see ffmpeg claiming nonsense bit rates like
> > -1184293205235990
> > > > kb/s
> > > > anymore.
> > >
> > >
> > > I don't think you understand my question.
> > >
> > > - does this belong in 4xm.c? (Or in generic code? Or in the app?)
> > > - should it return an error? (Or just clip parameters? Or ignore the
> > > invalid value?)
> > >
> > > > This isn't specifically intended at this patch, but rather at the sort
> > of
> > > > > rabbit hole this change might lead to,
> > > >
> > > > I have a pretty good map of this rabbit hole (i.e. lots of samples
> > > > triggering
> > > > UBSan errors) and one day I might try to dig it up, but for now I'm
> > > > limiting
> > > > myself to the codec parameters.
> > >
> > >
> > > I'm not saying mplayer was great, but one of the principles I believe we
> > > always copied from them was to try to play files to the best of our
> > > abilities and not error out at the first convenience. This isn't just a
> > > theoretical thing, a lot of people credited mplayer with playing utterly
> > > broken AVI files that probably even ffmpeg rejects. What ffmpeg added on
> > > top of that is to make a project maintainable by not being an utter
> > > crapshoot.
> > >
> > > This isn't 4xm.c-specific, this is a general philosophical question:
> > > - should we error out?
> > > - should this be in generic code if we're likely to repeat such checks
> > all
> > > over the place?
> > >
> > > > which would cause the code to be uber-full of such checks, none of
> > which
> > > > > really have any significance. But maybe others disagree...
> > > >
> > > > Not relying on undefined behavior is a significant improvement. And
> > doing
> > > > these checks consequently where the values are set makes it possible
> > > > for other code to rely on their validity without further checks.
> > >
> > >
> > > Unless "UB" leads to actual bad behaviour, I personally don't necessarily
> > > care. I'm very scared that when you go beyond codec parameters, and you
> > > want to check for overflows all over the place, we'll never see the end
> > of
> > > it...
> > >
> >
> > > I'd appreciate if others could chime in here, I don't want to carry this
> > > argument all by myself if nobody cares.
> >
> > as you are asking for others oppinion
> > valid C code must not trigger undefined behavior
> 
> 
> So, I asked on IRC, here's 3 suggestions from Roger Combs:
> 
> - in case we want to be pedantic (I personally don't care, but I understand
> other people are), it might make sense to just make these members
> (channels, block_align, bitrate) unsigned. That removes the UB of the
> overflow, and it fixes the negative number reporting in client apps for
> bitrate, but you can still have positive crazy numbers and it doesn't
> return an error.
> - if for whatever reason some things cannot be done in generic code or by
> changing the type (this should really cover most cases), and we want
> specific overflow checks, then maybe we want to have some generic helper
> macros that make them one-liners in decoders. This would return an error
> along with fixing the UB.
> - have overflow-safe multiplication functions instead of checking each
> argument before doing a multiply, like __builtin_mul_overflow, and then
> return INT64_MAX if it would overflow inside that function. This fixes
> display of numbers in client applications and the UB, but without returning
> an error.
> 
> What I want most - and this comment applies to all patches of this sort -
> is to have something that we can all agree is OK for pretty much any
> decoder out there without significant overhead in code (source - not
> binary) size. This way, you have a template to work from and fix specific
> instances without us having to argue over every single time you do a next
> run with ubsan. I personally like suggestion (1), unsigned is a pretty good
> type for things that shouldn't have negative values. WDYT?

unsigned is not unproblematic.
If you use unsigned values, all computations touched are unsigned (
or a larger data type). If unsigned they cannot be negative. This
requires great care buffer end, index and start computations can become
invalid very easily by changing the signedness of a type involved.

also unsigned makes detecting overflows impossible with existing
tools i know of. So while now if a decoder simply gives bad output with
no clear hints as to why, one can use ubsan, asan, valgrind, ...
and it will show integer overflows if thats the cause. But with
unsigned values nothing would be shown in a case that a affected
computation overflow was involved.

IMO if you compute the bitrate out of channels, bits per sample and
sample rate.
its correct to check that the result is representable in the bitrate
field if prior checks on the inputs are not sufficient.

If computations involve for example array indexing or division
checks cant be skiped as they would crash. Should we treat integer
overflow differntly ?
IMO the professional thing to do is to set the bitrate correctly or
if it is not possible to (not set it / set it to "unknown" / set it
by other means)
setting it to some overflowed value even if it doesnt matter for
the functioning of the demuxer is at least ugly

Anyway, what i think is important really is to understand the
extend of this. Are we dealing with 100, 1000, 10000 individual issues?
i think if its 100 or rather 100 checks would fix all issues then
simply adding checks should not really be a problem.

Considering that iam fixing these kind of issues since a while and
people do not report astronomic numbers of such issues to me it may
be that this whole problem is not of a magnitude that requires
extreem meassures.
But i may be wrong i have not myself tried to analyze how many such
issues exist.

What i do know though is there are some of these integer overflows
in  DSP code with fuzzed samples, and i do not know of a solution
for the DSP case that i like. Its almost as if leaving the DSP code
as is with the overflows is least bad.

[...]

On 15.12.2016 14:02, Ronald S. Bultje wrote:
> On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
> andreas.cadhalpun@googlemail.com> wrote:
>> On 14.12.2016 02:46, Ronald S. Bultje wrote:
>>> Not wanting to discourage you, but I wonder if there's really a point to
>>> this...?
>>
>> These patches are prerequisites for enforcing the validity of codec
>> parameters [1].
>>
>>> I don't see how the user experience changes.
>>
>> Users won't see ffmpeg claiming nonsense bit rates like -1184293205235990
>> kb/s
>> anymore.
> 
> 
> I don't think you understand my question.

Maybe you just didn't understand my answer.

> - does this belong in 4xm.c? (Or in generic code? Or in the app?)

I think it belongs both in 4xm.c and generic code, as I have explained
in detail [1] in the discussion back then.

> - should it return an error? (Or just clip parameters? Or ignore the
> invalid value?)

This was also already discussed [2].

On 15.12.2016 21:57, Ronald S. Bultje wrote:
> So, I asked on IRC, here's 3 suggestions from Roger Combs:
> 
> - in case we want to be pedantic (I personally don't care, but I understand
> other people are), it might make sense to just make these members
> (channels, block_align, bitrate) unsigned. That removes the UB of the
> overflow, and it fixes the negative number reporting in client apps for
> bitrate, but you can still have positive crazy numbers and it doesn't
> return an error.

These are public fields, so changing them is not easy and more importantly
changing them from signed to unsigned is a very bad idea, as it will most
probably cause all kinds of subtle bugs for API users, e.g. when comparing,
subtracting, etc. and not expecting unsigned behavior.

> - if for whatever reason some things cannot be done in generic code or by
> changing the type (this should really cover most cases), and we want
> specific overflow checks, then maybe we want to have some generic helper
> macros that make them one-liners in decoders. This would return an error
> along with fixing the UB.

I don't think the number of overflow checks added justifies the additional
complexity of factoring things out. These checks are also subtly different,
so it's not easy to write a generic helper for that.
However, I plan to do this for the actually common cases when validating
codec parameters, like checking that a parameter is not negative.

> - have overflow-safe multiplication functions instead of checking each
> argument before doing a multiply, like __builtin_mul_overflow, and then
> return INT64_MAX if it would overflow inside that function. This fixes
> display of numbers in client applications and the UB, but without returning
> an error.

I think that would be over-engineered.

> What I want most - and this comment applies to all patches of this sort -
> is to have something that we can all agree is OK for pretty much any
> decoder out there without significant overhead in code (source - not
> binary) size. This way, you have a template to work from and fix specific
> instances without us having to argue over every single time you do a next
> run with ubsan.

UBSan is not only about overflows, undefined shifts are also quite common.
But I haven't really looked into these cases yet, so I don't know what kind
of template, if any, would make sense for them.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-October/201742.html
2: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/203257.html

> On Dec 15, 2016, at 19:21, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote:
> 
> On 15.12.2016 14:02, Ronald S. Bultje wrote:
>> On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
>> andreas.cadhalpun@googlemail.com> wrote:
>>> On 14.12.2016 02:46, Ronald S. Bultje wrote:
>>>> Not wanting to discourage you, but I wonder if there's really a point to
>>>> this...?
>>> 
>>> These patches are prerequisites for enforcing the validity of codec
>>> parameters [1].
>>> 
>>>> I don't see how the user experience changes.
>>> 
>>> Users won't see ffmpeg claiming nonsense bit rates like -1184293205235990
>>> kb/s
>>> anymore.
>> 
>> 
>> I don't think you understand my question.
> 
> Maybe you just didn't understand my answer.
> 
>> - does this belong in 4xm.c? (Or in generic code? Or in the app?)
> 
> I think it belongs both in 4xm.c and generic code, as I have explained
> in detail [1] in the discussion back then.
> 
>> - should it return an error? (Or just clip parameters? Or ignore the
>> invalid value?)
> 
> This was also already discussed [2].
> 
> On 15.12.2016 21:57, Ronald S. Bultje wrote:
>> So, I asked on IRC, here's 3 suggestions from Roger Combs:
>> 
>> - in case we want to be pedantic (I personally don't care, but I understand
>> other people are), it might make sense to just make these members
>> (channels, block_align, bitrate) unsigned. That removes the UB of the
>> overflow, and it fixes the negative number reporting in client apps for
>> bitrate, but you can still have positive crazy numbers and it doesn't
>> return an error.
> 
> These are public fields, so changing them is not easy and more importantly
> changing them from signed to unsigned is a very bad idea, as it will most
> probably cause all kinds of subtle bugs for API users, e.g. when comparing,
> subtracting, etc. and not expecting unsigned behavior.
> 

Fair enough.

>> - if for whatever reason some things cannot be done in generic code or by
>> changing the type (this should really cover most cases), and we want
>> specific overflow checks, then maybe we want to have some generic helper
>> macros that make them one-liners in decoders. This would return an error
>> along with fixing the UB.
> 
> I don't think the number of overflow checks added justifies the additional
> complexity of factoring things out. These checks are also subtly different,
> so it's not easy to write a generic helper for that.
> However, I plan to do this for the actually common cases when validating
> codec parameters, like checking that a parameter is not negative.
> 

My proposal was for something like:
#define BAIL_ON_OVERFLOW(x) if (x) {av_log(avctx, AV_LOG_ERROR, "Overflow check failed: " #x); return AVERROR_INVALIDDATA;}
Which basically reduces the code overhead down to a simple one-liner. It's hard to get detailed error prints out of this, but if we're saying these cases are so unlikely (fuzzer-only?) that we're comfortable outright failing on them, the level of precision in the message probably doesn't matter much?

>> - have overflow-safe multiplication functions instead of checking each
>> argument before doing a multiply, like __builtin_mul_overflow, and then
>> return INT64_MAX if it would overflow inside that function. This fixes
>> display of numbers in client applications and the UB, but without returning
>> an error.
> 
> I think that would be over-engineered.
> 
>> What I want most - and this comment applies to all patches of this sort -
>> is to have something that we can all agree is OK for pretty much any
>> decoder out there without significant overhead in code (source - not
>> binary) size. This way, you have a template to work from and fix specific
>> instances without us having to argue over every single time you do a next
>> run with ubsan.
> 
> UBSan is not only about overflows, undefined shifts are also quite common.
> But I haven't really looked into these cases yet, so I don't know what kind
> of template, if any, would make sense for them.
> 
> Best regards,
> Andreas
> 
> 
> 1: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-October/201742.html
> 2: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/203257.html
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

On 16.12.2016 02:29, Rodger Combs wrote: 
>> On Dec 15, 2016, at 19:21, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote:
>> On 15.12.2016 14:02, Ronald S. Bultje wrote:
>>> - if for whatever reason some things cannot be done in generic code or by
>>> changing the type (this should really cover most cases), and we want
>>> specific overflow checks, then maybe we want to have some generic helper
>>> macros that make them one-liners in decoders. This would return an error
>>> along with fixing the UB.
>>
>> I don't think the number of overflow checks added justifies the additional
>> complexity of factoring things out. These checks are also subtly different,
>> so it's not easy to write a generic helper for that.
>> However, I plan to do this for the actually common cases when validating
>> codec parameters, like checking that a parameter is not negative.
>>
> 
> My proposal was for something like:
> #define BAIL_ON_OVERFLOW(x) if (x) {av_log(avctx, AV_LOG_ERROR, "Overflow check failed: " #x); return AVERROR_INVALIDDATA;}
> Which basically reduces the code overhead down to a simple one-liner.

Yeah, that's similar to how I plan to handle the more common cases.

> It's hard to get detailed error prints out of this, but if we're saying these
> cases are so unlikely (fuzzer-only?) that we're comfortable outright failing
> on them, the level of precision in the message probably doesn't matter much?

Agreed, so I've updated the patch series using this approach.

Best regards,
Andreas