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

[Andreas Cadhalpun]

On 16.12.2016 02:29, Rodger Combs wrote: 
>> On Dec 15, 2016, at 19:21, Andreas Cadhalpun 
>>  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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rodger Combs]

> On Dec 15, 2016, at 19:21, Andreas Cadhalpun 
>  wrote:
> 
> On 15.12.2016 14:02, Ronald S. Bultje wrote:
>> On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
>> andreas.cadhal...@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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

On 15.12.2016 14:02, Ronald S. Bultje wrote:
> On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
> andreas.cadhal...@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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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  > 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.cadhal...@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 

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

[Ronald S. Bultje]

Hi,

On Thu, Dec 15, 2016 at 9:28 AM, Michael Niedermayer  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.cadhal...@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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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.cadhal...@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

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

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ronald S. Bultje]

Hi,

On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
andreas.cadhal...@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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

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 
>> ---
>>  libavformat/4xm.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> LGTM

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

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

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

[Michael Niedermayer]

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

LGTM

thx

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ronald S. Bultje]

Hi,

On Tue, Dec 13, 2016 at 8:21 PM, Andreas Cadhalpun <
andreas.cadhal...@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.cadhal...@googlemail.com> wrote:
> >
> >> Signed-off-by: Andreas Cadhalpun 
> >> ---
> >>  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

On 14.12.2016 02:01, Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, Dec 13, 2016 at 7:57 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
> 
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ronald S. Bultje]

Hi,

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

> Signed-off-by: Andreas Cadhalpun 
> ---
>  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun 
---
 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;
+}
+
 /* allocate a new AVStream */
 st = avformat_new_stream(s, NULL);
 if (!st)
@@ -178,7 +184,7 @@ static int parse_strk(AVFormatContext *s,
 st->codecpar->channels  = fourxm->tracks[track].channels;
 st->codecpar->sample_rate   = fourxm->tracks[track].sample_rate;
 st->codecpar->bits_per_coded_sample = fourxm->tracks[track].bits;
-st->codecpar->bit_rate  = st->codecpar->channels *
+st->codecpar->bit_rate  = (int64_t)st->codecpar->channels *
   st->codecpar->sample_rate *
   st->codecpar->bits_per_coded_sample;
 st->codecpar->block_align   = st->codecpar->channels *
-- 
2.10.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel