Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[wm4]

On Mon, 10 Jul 2017 23:10:35 +0200
Michael Niedermayer  wrote:

> On Mon, Jul 10, 2017 at 10:37:46AM +0200, wm4 wrote:
> > On Sun, 9 Jul 2017 22:03:21 +0200
> > Reimar Döffinger  wrote:
> >   
> > > On 09.07.2017, at 16:08, "Ronald S. Bultje"  wrote:  
> > > > Hi,
> > > > 
> > > > On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
> > > > 
> > > > wrote:
> > > > 
> > > >> On 09.07.2017, at 02:52, "Ronald S. Bultje"  
> > > >> wrote:
> > > >>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer
> > > >> 
> > > >>> wrote:
> > > >>> 
> > >  
> > >  Does anyone object to this patch ?
> > >  Or does anyone have a better idea on how to fix this ?
> > >  if not id like to apply it
> > > >>> 
> > > >>> 
> > > >>> I think Rostislav's point is: why fix it, if it can only happen with
> > > >>> corrupt input? The before and after situation is identical: garbage 
> > > >>> in,
> > > >>> garbage out. If the compiler does funny things that makes the garbage
> > > >>> slightly differently bad, is that really so devilishly bad? It's still
> > > >>> garbage. Is anything improved by this?
> > > >> 
> > > >> The way C works, you MUST assume any undefined behaviour can at any 
> > > >> point
> > > >> [..] become exploitable.[..] If you don't like that, C is the wrong
> > > >> language to use.
> > > > 
> > > > 
> > > > I think I've read "the boy who cried wolf" a few too many times to my 
> > > > kids,
> > > > but the form of this discussion is currently too polarizing/political 
> > > > for
> > > > my taste.
> > > 
> > > That is my reading of the C standard, is that political or even just 
> > > controversial?
> > > I mean of course you can ignore standards (see MPEG-4 ASP, and in some 
> > > ways that was actually fairly reasonable thing to do at the time), and I 
> > > don't fix every undefined behaviour case in my code when I can't think of 
> > > any reasonable solution.
> > > So there is a cost-benefit discussion in principle.
> > > I believe the cost of not fixing undefined behaviour, just by virtue of 
> > > going outside what the standard guarantees should be considered fairly 
> > > high.
> > > That is an opinion, but is there any disagreement that undefined 
> > > behaviour is at least an issue of some degree?
> > > If we can agree on that, then the question would only be how much 
> > > effort/code ugliness is reasonable.
> > > There is also the point (which I hope I mentioned in the parts cut out) 
> > > that just making sure that these cases are not already exploitable right 
> > > now with the current compiler can in many cases be quite a pain (and does 
> > > not have tool support), so I think fixing it would often be the 
> > > lowest-effort method.  
> > 
> > The controversial thing is actually the SUINT nonsense. A type is
> > either signed or unsigned, but not both incompletely intransparent  
> 
> > ways. Michael keeps adding them even though many are against it.   
> 
> It is extreemly rude from you to make this claim.
> When in fact i try my best to respect every maitainer and authors
> preferrance and never add it when people object.

Do you deny that you keep sending patches that add SUINT usage? Do you
deny that you know that certain people don't like SUINT?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Michael Niedermayer]

On Mon, Jul 10, 2017 at 10:37:46AM +0200, wm4 wrote:
> On Sun, 9 Jul 2017 22:03:21 +0200
> Reimar Döffinger  wrote:
> 
> > On 09.07.2017, at 16:08, "Ronald S. Bultje"  wrote:
> > > Hi,
> > > 
> > > On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
> > > 
> > > wrote:
> > >   
> > >> On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:  
> > >>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer  
> > >>   
> > >>> wrote:
> > >>>   
> >  
> >  Does anyone object to this patch ?
> >  Or does anyone have a better idea on how to fix this ?
> >  if not id like to apply it  
> > >>> 
> > >>> 
> > >>> I think Rostislav's point is: why fix it, if it can only happen with
> > >>> corrupt input? The before and after situation is identical: garbage in,
> > >>> garbage out. If the compiler does funny things that makes the garbage
> > >>> slightly differently bad, is that really so devilishly bad? It's still
> > >>> garbage. Is anything improved by this?  
> > >> 
> > >> The way C works, you MUST assume any undefined behaviour can at any point
> > >> [..] become exploitable.[..] If you don't like that, C is the wrong
> > >> language to use.  
> > > 
> > > 
> > > I think I've read "the boy who cried wolf" a few too many times to my 
> > > kids,
> > > but the form of this discussion is currently too polarizing/political for
> > > my taste.  
> > 
> > That is my reading of the C standard, is that political or even just 
> > controversial?
> > I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways 
> > that was actually fairly reasonable thing to do at the time), and I don't 
> > fix every undefined behaviour case in my code when I can't think of any 
> > reasonable solution.
> > So there is a cost-benefit discussion in principle.
> > I believe the cost of not fixing undefined behaviour, just by virtue of 
> > going outside what the standard guarantees should be considered fairly high.
> > That is an opinion, but is there any disagreement that undefined behaviour 
> > is at least an issue of some degree?
> > If we can agree on that, then the question would only be how much 
> > effort/code ugliness is reasonable.
> > There is also the point (which I hope I mentioned in the parts cut out) 
> > that just making sure that these cases are not already exploitable right 
> > now with the current compiler can in many cases be quite a pain (and does 
> > not have tool support), so I think fixing it would often be the 
> > lowest-effort method.
> 
> The controversial thing is actually the SUINT nonsense. A type is
> either signed or unsigned, but not both incompletely intransparent

> ways. Michael keeps adding them even though many are against it. 

It is extreemly rude from you to make this claim.
When in fact i try my best to respect every maitainer and authors
preferrance and never add it when people object.

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 


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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[wm4]

On Mon, 10 Jul 2017 09:29:31 -0300
James Almer  wrote:

> On 7/10/2017 5:37 AM, wm4 wrote:
> > On Sun, 9 Jul 2017 22:03:21 +0200
> > Reimar Döffinger  wrote:
> >   
> >> On 09.07.2017, at 16:08, "Ronald S. Bultje"  wrote:  
> >>> Hi,
> >>>
> >>> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
> >>> 
> >>> wrote:
> >>> 
>  On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:  
>    
> > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer
>  
> > wrote:
> > 
> >>
> >> Does anyone object to this patch ?
> >> Or does anyone have a better idea on how to fix this ?
> >> if not id like to apply it
> >
> >
> > I think Rostislav's point is: why fix it, if it can only happen with
> > corrupt input? The before and after situation is identical: garbage in,
> > garbage out. If the compiler does funny things that makes the garbage
> > slightly differently bad, is that really so devilishly bad? It's still
> > garbage. Is anything improved by this?
> 
>  The way C works, you MUST assume any undefined behaviour can at any point
>  [..] become exploitable.[..] If you don't like that, C is the wrong
>  language to use.
> >>>
> >>>
> >>> I think I've read "the boy who cried wolf" a few too many times to my 
> >>> kids,
> >>> but the form of this discussion is currently too polarizing/political for
> >>> my taste.
> >>
> >> That is my reading of the C standard, is that political or even just 
> >> controversial?
> >> I mean of course you can ignore standards (see MPEG-4 ASP, and in some 
> >> ways that was actually fairly reasonable thing to do at the time), and I 
> >> don't fix every undefined behaviour case in my code when I can't think of 
> >> any reasonable solution.
> >> So there is a cost-benefit discussion in principle.
> >> I believe the cost of not fixing undefined behaviour, just by virtue of 
> >> going outside what the standard guarantees should be considered fairly 
> >> high.
> >> That is an opinion, but is there any disagreement that undefined behaviour 
> >> is at least an issue of some degree?
> >> If we can agree on that, then the question would only be how much 
> >> effort/code ugliness is reasonable.
> >> There is also the point (which I hope I mentioned in the parts cut out) 
> >> that just making sure that these cases are not already exploitable right 
> >> now with the current compiler can in many cases be quite a pain (and does 
> >> not have tool support), so I think fixing it would often be the 
> >> lowest-effort method.  
> > 
> > The controversial thing is actually the SUINT nonsense. A type is
> > either signed or unsigned, but not both incompletely intransparent
> > ways. Michael keeps adding them even though many are against it. 
> > 
> > "SUINTFLOAT" just tops this. Is it a signed int? Unsigned int? A float?
> > Unsigned float???  
> 
> SUINTFLOAT is float or an integer (SUINT) depending on how you include
> the template file.
> 
> Blame the template crap for all the int variants of decoders (aac, ac3,
> etc).

Yeah, I understand how it came to be, but maybe it's not a good idea to
combine the various hacks into more combinations of hacks.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[James Almer]

On 7/10/2017 5:37 AM, wm4 wrote:
> On Sun, 9 Jul 2017 22:03:21 +0200
> Reimar Döffinger  wrote:
> 
>> On 09.07.2017, at 16:08, "Ronald S. Bultje"  wrote:
>>> Hi,
>>>
>>> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
>>> wrote:
>>>   
 On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:  
> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer  
   
> wrote:
>   
>>
>> Does anyone object to this patch ?
>> Or does anyone have a better idea on how to fix this ?
>> if not id like to apply it  
>
>
> I think Rostislav's point is: why fix it, if it can only happen with
> corrupt input? The before and after situation is identical: garbage in,
> garbage out. If the compiler does funny things that makes the garbage
> slightly differently bad, is that really so devilishly bad? It's still
> garbage. Is anything improved by this?  

 The way C works, you MUST assume any undefined behaviour can at any point
 [..] become exploitable.[..] If you don't like that, C is the wrong
 language to use.  
>>>
>>>
>>> I think I've read "the boy who cried wolf" a few too many times to my kids,
>>> but the form of this discussion is currently too polarizing/political for
>>> my taste.  
>>
>> That is my reading of the C standard, is that political or even just 
>> controversial?
>> I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways 
>> that was actually fairly reasonable thing to do at the time), and I don't 
>> fix every undefined behaviour case in my code when I can't think of any 
>> reasonable solution.
>> So there is a cost-benefit discussion in principle.
>> I believe the cost of not fixing undefined behaviour, just by virtue of 
>> going outside what the standard guarantees should be considered fairly high.
>> That is an opinion, but is there any disagreement that undefined behaviour 
>> is at least an issue of some degree?
>> If we can agree on that, then the question would only be how much 
>> effort/code ugliness is reasonable.
>> There is also the point (which I hope I mentioned in the parts cut out) that 
>> just making sure that these cases are not already exploitable right now with 
>> the current compiler can in many cases be quite a pain (and does not have 
>> tool support), so I think fixing it would often be the lowest-effort method.
> 
> The controversial thing is actually the SUINT nonsense. A type is
> either signed or unsigned, but not both incompletely intransparent
> ways. Michael keeps adding them even though many are against it. 
> 
> "SUINTFLOAT" just tops this. Is it a signed int? Unsigned int? A float?
> Unsigned float???

SUINTFLOAT is float or an integer (SUINT) depending on how you include
the template file.

Blame the template crap for all the int variants of decoders (aac, ac3,
etc).

> 
> Come on, this is a huge load of bullshit.
> 
>> I'd also like to point out that even ignoring all that, ubsan + fuzzing is a 
>> powerful tool to improve code quality, and I would argue than at least some 
>> amount of code complexity is justified just for having them work well.
>> And it's also that to my mind the patch did not look THAT bad...
> 
> A powerful tool for Michael to churn out patches which make the code
> look worse and more tricky, which without doubt will lead ot more new
> bugs some time in the future. Sure, security is good, but at this
> point I'm even wondering what's the point of this. Realistically,
> you'll have to sandbox ffmpeg anyway if you want some minimal security.
> ___
> 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 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[wm4]

On Sun, 9 Jul 2017 22:03:21 +0200
Reimar Döffinger  wrote:

> On 09.07.2017, at 16:08, "Ronald S. Bultje"  wrote:
> > Hi,
> > 
> > On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
> > wrote:
> >   
> >> On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:  
> >>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer  
> >>   
> >>> wrote:
> >>>   
>  
>  Does anyone object to this patch ?
>  Or does anyone have a better idea on how to fix this ?
>  if not id like to apply it  
> >>> 
> >>> 
> >>> I think Rostislav's point is: why fix it, if it can only happen with
> >>> corrupt input? The before and after situation is identical: garbage in,
> >>> garbage out. If the compiler does funny things that makes the garbage
> >>> slightly differently bad, is that really so devilishly bad? It's still
> >>> garbage. Is anything improved by this?  
> >> 
> >> The way C works, you MUST assume any undefined behaviour can at any point
> >> [..] become exploitable.[..] If you don't like that, C is the wrong
> >> language to use.  
> > 
> > 
> > I think I've read "the boy who cried wolf" a few too many times to my kids,
> > but the form of this discussion is currently too polarizing/political for
> > my taste.  
> 
> That is my reading of the C standard, is that political or even just 
> controversial?
> I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways 
> that was actually fairly reasonable thing to do at the time), and I don't fix 
> every undefined behaviour case in my code when I can't think of any 
> reasonable solution.
> So there is a cost-benefit discussion in principle.
> I believe the cost of not fixing undefined behaviour, just by virtue of going 
> outside what the standard guarantees should be considered fairly high.
> That is an opinion, but is there any disagreement that undefined behaviour is 
> at least an issue of some degree?
> If we can agree on that, then the question would only be how much effort/code 
> ugliness is reasonable.
> There is also the point (which I hope I mentioned in the parts cut out) that 
> just making sure that these cases are not already exploitable right now with 
> the current compiler can in many cases be quite a pain (and does not have 
> tool support), so I think fixing it would often be the lowest-effort method.

The controversial thing is actually the SUINT nonsense. A type is
either signed or unsigned, but not both incompletely intransparent
ways. Michael keeps adding them even though many are against it. 

"SUINTFLOAT" just tops this. Is it a signed int? Unsigned int? A float?
Unsigned float???

Come on, this is a huge load of bullshit.

> I'd also like to point out that even ignoring all that, ubsan + fuzzing is a 
> powerful tool to improve code quality, and I would argue than at least some 
> amount of code complexity is justified just for having them work well.
> And it's also that to my mind the patch did not look THAT bad...

A powerful tool for Michael to churn out patches which make the code
look worse and more tricky, which without doubt will lead ot more new
bugs some time in the future. Sure, security is good, but at this
point I'm even wondering what's the point of this. Realistically,
you'll have to sandbox ffmpeg anyway if you want some minimal security.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Ivan Kalvachev]

On 7/9/17, Ronald S. Bultje  wrote:
> Hi,
>
> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
> wrote:
>
>> On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:
>> > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer
>> 
>> > wrote:
>> >
>> >>
>> >> Does anyone object to this patch ?
>> >> Or does anyone have a better idea on how to fix this ?
>> >> if not id like to apply it
>> >
>> >
>> > I think Rostislav's point is: why fix it, if it can only happen with
>> > corrupt input? The before and after situation is identical: garbage in,
>> > garbage out. If the compiler does funny things that makes the garbage
>> > slightly differently bad, is that really so devilishly bad? It's still
>> > garbage. Is anything improved by this?
>>
>> The way C works, you MUST assume any undefined behaviour can at any point
>> [..] become exploitable.[..] If you don't like that, C is the wrong
>> language to use.
>
>
> I think I've read "the boy who cried wolf" a few too many times to my kids,
> but the form of this discussion is currently too polarizing/political for
> my taste.

So, you stir the pot and then run away.
You ignore technical explanations and you call discussion too political.
And you "imply" that developers are lairs.

Maybe you are not aware what you are going,
but I do assure you, it aint pretty.

I think you owe some people an apology.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Reimar Döffinger]

On 09.07.2017, at 16:08, "Ronald S. Bultje"  wrote:
> Hi,
> 
> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
> wrote:
> 
>> On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:
>>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer
>> 
>>> wrote:
>>> 
 
 Does anyone object to this patch ?
 Or does anyone have a better idea on how to fix this ?
 if not id like to apply it
>>> 
>>> 
>>> I think Rostislav's point is: why fix it, if it can only happen with
>>> corrupt input? The before and after situation is identical: garbage in,
>>> garbage out. If the compiler does funny things that makes the garbage
>>> slightly differently bad, is that really so devilishly bad? It's still
>>> garbage. Is anything improved by this?
>> 
>> The way C works, you MUST assume any undefined behaviour can at any point
>> [..] become exploitable.[..] If you don't like that, C is the wrong
>> language to use.
> 
> 
> I think I've read "the boy who cried wolf" a few too many times to my kids,
> but the form of this discussion is currently too polarizing/political for
> my taste.

That is my reading of the C standard, is that political or even just 
controversial?
I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways 
that was actually fairly reasonable thing to do at the time), and I don't fix 
every undefined behaviour case in my code when I can't think of any reasonable 
solution.
So there is a cost-benefit discussion in principle.
I believe the cost of not fixing undefined behaviour, just by virtue of going 
outside what the standard guarantees should be considered fairly high.
That is an opinion, but is there any disagreement that undefined behaviour is 
at least an issue of some degree?
If we can agree on that, then the question would only be how much effort/code 
ugliness is reasonable.
There is also the point (which I hope I mentioned in the parts cut out) that 
just making sure that these cases are not already exploitable right now with 
the current compiler can in many cases be quite a pain (and does not have tool 
support), so I think fixing it would often be the lowest-effort method.
I'd also like to point out that even ignoring all that, ubsan + fuzzing is a 
powerful tool to improve code quality, and I would argue than at least some 
amount of code complexity is justified just for having them work well.
And it's also that to my mind the patch did not look THAT bad...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Michael Niedermayer]

On Sun, Jul 09, 2017 at 10:08:50AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
> wrote:
> 
> > On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:
> > > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer
> > 
> > > wrote:
> > >
> > >>
> > >> Does anyone object to this patch ?
> > >> Or does anyone have a better idea on how to fix this ?
> > >> if not id like to apply it
> > >
> > >
> > > I think Rostislav's point is: why fix it, if it can only happen with
> > > corrupt input? The before and after situation is identical: garbage in,
> > > garbage out. If the compiler does funny things that makes the garbage
> > > slightly differently bad, is that really so devilishly bad? It's still
> > > garbage. Is anything improved by this?
> >
> > The way C works, you MUST assume any undefined behaviour can at any point
> > [..] become exploitable.[..] If you don't like that, C is the wrong
> > language to use.
> 
> 
> I think I've read "the boy who cried wolf" a few too many times to my kids,
> but the form of this discussion is currently too polarizing/political for
> my taste.

I dont know about polarizing, it quite possibly is but
If anyone belives this is just political or theoretical and its safe to
trigger undefined behavior if the output is not used, here are some
examples found after 30sec of google use where that is not so:
https://blogs.msdn.microsoft.com/oldnewthing/20140627-00/?p=633
https://blog.regehr.org/archives/759

This is intended "for the archives" and not intended to pull
anyone into a discussion they dont want to be in.

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

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope


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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Ronald S. Bultje]

Hi,

On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
wrote:

> On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:
> > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer
> 
> > wrote:
> >
> >>
> >> Does anyone object to this patch ?
> >> Or does anyone have a better idea on how to fix this ?
> >> if not id like to apply it
> >
> >
> > I think Rostislav's point is: why fix it, if it can only happen with
> > corrupt input? The before and after situation is identical: garbage in,
> > garbage out. If the compiler does funny things that makes the garbage
> > slightly differently bad, is that really so devilishly bad? It's still
> > garbage. Is anything improved by this?
>
> The way C works, you MUST assume any undefined behaviour can at any point
> [..] become exploitable.[..] If you don't like that, C is the wrong
> language to use.


I think I've read "the boy who cried wolf" a few too many times to my kids,
but the form of this discussion is currently too polarizing/political for
my taste.

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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Ivan Kalvachev]

On 7/9/17, Ronald S. Bultje  wrote:
> Hi,
>
> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer 
> wrote:
>
>> On Mon, Jul 03, 2017 at 01:37:09AM +0200, Michael Niedermayer wrote:
>> > On Sun, Jul 02, 2017 at 02:24:53PM +0100, Rostislav Pehlivanov wrote:
>> > > On 2 July 2017 at 03:28, Michael Niedermayer 
>> wrote:
>> > >
>> > > > Fixes: runtime error: signed integer overflow: 1965219850 +
>> > > > 995792909
>> > > > cannot be represented in type 'int'
>> > > > Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920
>> > > >
>> > > > Found-by: continuous fuzzing process https://github.com/google/oss-
>> > > > fuzz/tree/master/projects/ffmpeg
>> > > > Signed-off-by
>> > > > > ffmpeg%0ASigned-off-by>:
>> > > > Michael Niedermayer 
>> > > > ---
>> > > >  libavcodec/aacpsdsp_template.c | 3 ++-
>> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_
>> > > > template.c
>> > > > index 9e1a95c1a1..2c0afd4512 100644
>> > > > --- a/libavcodec/aacpsdsp_template.c
>> > > > +++ b/libavcodec/aacpsdsp_template.c
>> > > > @@ -26,9 +26,10 @@
>> > > >  #include "libavutil/attributes.h"
>> > > >  #include "aacpsdsp.h"
>> > > >
>> > > > -static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT
>> (*src)[2], int
>> > > > n)
>> > > > +static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT
>> > > > (*src)[2], int n)
>> > > >  {
>> > > >  int i;
>> > > > +SUINTFLOAT *dst = dst_param;
>> > > >  for (i = 0; i < n; i++)
>> > > >  dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1],
>> src[i][1]);
>> > > >  }
>> > > >
>> > > >
>> > > What's the issue with just _not_ fixing it here? It only occurs on
>> fuzzed
>> > > inputs, doesn't crash on any known platform ever, makes the code
>> uglier and
>> > > why? Because some fuzzer is super pedantic.
>> > > Why not fix the fuzzer? Why not just mark this as a false positive,
>> since
>> > > fixing it is pointless from the standpoint of security (you can't
>> exploit
>> > > overflows in transforms or functions like this), and all developers
>> hate it.
>> >
>> > Iam not sure you understand the problem.
>> > signed integer overflows are undefined behavior, undefined behavior
>> > is not allowed in C.
>> >
>> >
>> > Theres also no option to mark anything as false positive.
>> > If the tool makes a mistake, the issue needs to be reported to its
>> > authors and needs to be fixed.
>> > I belive the tool is correct, if someone thinks its not correct, the
>> > place to report this to is likely where the clang sanitizers are
>> > developed.
>> >
>> > I do understand that this is annoying but this is how C works.
>> >
>> > About "doesn't crash on any known platform ever",
>> > "you can't exploit  overflows in transforms or functions like this"
>> >
>> > undefined behavior has bitten people with unexpected results in the
>> > past. for example "if (a+b < a)" to test for overflow, but because the
>> condition
>> > can only be true in case of overflow and overflow isnt allowed in C
>> > the compiler didnt assemble this the way the human thought.
>> >
>> > In the case of ps_add_squares_c(), dst[i] has to increase if iam
>> > not mistaken. In case of overflow it can decrease but overflow is
>> > not allowed so a compiler can prune anything that implies dst[] to be
>> > negative.
>> > dst[] is used downstream in checks of greater / less and in FFMAX
>> > In this code the compiler can assume that the sign bit is clear,
>> > what happens when  the compilers optimizer has false assumtations
>> > i dont know but i know its not good.
>> >
>> > That said even if no such chain of conditions exist its still invalid
>> > code if theres undefined behavior in it
>>
>> Does anyone object to this patch ?
>> Or does anyone have a better idea on how to fix this ?
>> if not id like to apply it
>
>
> I think Rostislav's point is: why fix it, if it can only happen with
> corrupt input? The before and after situation is identical: garbage in,
> garbage out. If the compiler does funny things that makes the garbage
> slightly differently bad, is that really so devilishly bad? It's still
> garbage. Is anything improved by this?

As I have tried to explain before,
you can harden a program by compiling it
with `gcc -ftrapv` .

In such a program the overflow would trap
and in normal case would lead to termination.

Do you want your browser to die by a small bit error in a random clip?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Reimar Döffinger]

On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:
> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer 
> wrote:
> 
>> 
>> Does anyone object to this patch ?
>> Or does anyone have a better idea on how to fix this ?
>> if not id like to apply it
> 
> 
> I think Rostislav's point is: why fix it, if it can only happen with
> corrupt input? The before and after situation is identical: garbage in,
> garbage out. If the compiler does funny things that makes the garbage
> slightly differently bad, is that really so devilishly bad? It's still
> garbage. Is anything improved by this?

The way C works, you MUST assume any undefined behaviour can at any point 
(different compiler, compiler options, ...) become exploitable.
You can try to justify it with assumptions (but even that is usually very hard, 
is and will the value really never be used in a condition affecting, however 
indirectly, a pointer value for example?), but those are just arbitrary 
assumptions not backed by any standard.
If you don't like that, C is the wrong language to use.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Ronald S. Bultje]

Hi,

On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer 
wrote:

> On Mon, Jul 03, 2017 at 01:37:09AM +0200, Michael Niedermayer wrote:
> > On Sun, Jul 02, 2017 at 02:24:53PM +0100, Rostislav Pehlivanov wrote:
> > > On 2 July 2017 at 03:28, Michael Niedermayer 
> wrote:
> > >
> > > > Fixes: runtime error: signed integer overflow: 1965219850 + 995792909
> > > > cannot be represented in type 'int'
> > > > Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920
> > > >
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-
> > > > fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by
> > > >  ffmpeg%0ASigned-off-by>:
> > > > Michael Niedermayer 
> > > > ---
> > > >  libavcodec/aacpsdsp_template.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_
> > > > template.c
> > > > index 9e1a95c1a1..2c0afd4512 100644
> > > > --- a/libavcodec/aacpsdsp_template.c
> > > > +++ b/libavcodec/aacpsdsp_template.c
> > > > @@ -26,9 +26,10 @@
> > > >  #include "libavutil/attributes.h"
> > > >  #include "aacpsdsp.h"
> > > >
> > > > -static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT
> (*src)[2], int
> > > > n)
> > > > +static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT
> > > > (*src)[2], int n)
> > > >  {
> > > >  int i;
> > > > +SUINTFLOAT *dst = dst_param;
> > > >  for (i = 0; i < n; i++)
> > > >  dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1],
> src[i][1]);
> > > >  }
> > > >
> > > >
> > > What's the issue with just _not_ fixing it here? It only occurs on
> fuzzed
> > > inputs, doesn't crash on any known platform ever, makes the code
> uglier and
> > > why? Because some fuzzer is super pedantic.
> > > Why not fix the fuzzer? Why not just mark this as a false positive,
> since
> > > fixing it is pointless from the standpoint of security (you can't
> exploit
> > > overflows in transforms or functions like this), and all developers
> hate it.
> >
> > Iam not sure you understand the problem.
> > signed integer overflows are undefined behavior, undefined behavior
> > is not allowed in C.
> >
> >
> > Theres also no option to mark anything as false positive.
> > If the tool makes a mistake, the issue needs to be reported to its
> > authors and needs to be fixed.
> > I belive the tool is correct, if someone thinks its not correct, the
> > place to report this to is likely where the clang sanitizers are
> > developed.
> >
> > I do understand that this is annoying but this is how C works.
> >
> > About "doesn't crash on any known platform ever",
> > "you can't exploit  overflows in transforms or functions like this"
> >
> > undefined behavior has bitten people with unexpected results in the
> > past. for example "if (a+b < a)" to test for overflow, but because the
> condition
> > can only be true in case of overflow and overflow isnt allowed in C
> > the compiler didnt assemble this the way the human thought.
> >
> > In the case of ps_add_squares_c(), dst[i] has to increase if iam
> > not mistaken. In case of overflow it can decrease but overflow is
> > not allowed so a compiler can prune anything that implies dst[] to be
> > negative.
> > dst[] is used downstream in checks of greater / less and in FFMAX
> > In this code the compiler can assume that the sign bit is clear,
> > what happens when  the compilers optimizer has false assumtations
> > i dont know but i know its not good.
> >
> > That said even if no such chain of conditions exist its still invalid
> > code if theres undefined behavior in it
>
> Does anyone object to this patch ?
> Or does anyone have a better idea on how to fix this ?
> if not id like to apply it


I think Rostislav's point is: why fix it, if it can only happen with
corrupt input? The before and after situation is identical: garbage in,
garbage out. If the compiler does funny things that makes the garbage
slightly differently bad, is that really so devilishly bad? It's still
garbage. Is anything improved by this?

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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Michael Niedermayer]

On Mon, Jul 03, 2017 at 01:37:09AM +0200, Michael Niedermayer wrote:
> On Sun, Jul 02, 2017 at 02:24:53PM +0100, Rostislav Pehlivanov wrote:
> > On 2 July 2017 at 03:28, Michael Niedermayer  wrote:
> > 
> > > Fixes: runtime error: signed integer overflow: 1965219850 + 995792909
> > > cannot be represented in type 'int'
> > > Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920
> > >
> > > Found-by: continuous fuzzing process https://github.com/google/oss-
> > > fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by
> > > :
> > > Michael Niedermayer 
> > > ---
> > >  libavcodec/aacpsdsp_template.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_
> > > template.c
> > > index 9e1a95c1a1..2c0afd4512 100644
> > > --- a/libavcodec/aacpsdsp_template.c
> > > +++ b/libavcodec/aacpsdsp_template.c
> > > @@ -26,9 +26,10 @@
> > >  #include "libavutil/attributes.h"
> > >  #include "aacpsdsp.h"
> > >
> > > -static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT (*src)[2], int
> > > n)
> > > +static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT
> > > (*src)[2], int n)
> > >  {
> > >  int i;
> > > +SUINTFLOAT *dst = dst_param;
> > >  for (i = 0; i < n; i++)
> > >  dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1], src[i][1]);
> > >  }
> > >
> > >
> > What's the issue with just _not_ fixing it here? It only occurs on fuzzed
> > inputs, doesn't crash on any known platform ever, makes the code uglier and
> > why? Because some fuzzer is super pedantic.
> > Why not fix the fuzzer? Why not just mark this as a false positive, since
> > fixing it is pointless from the standpoint of security (you can't exploit
> > overflows in transforms or functions like this), and all developers hate it.
> 
> Iam not sure you understand the problem.
> signed integer overflows are undefined behavior, undefined behavior
> is not allowed in C.
> 
> 
> Theres also no option to mark anything as false positive.
> If the tool makes a mistake, the issue needs to be reported to its
> authors and needs to be fixed.
> I belive the tool is correct, if someone thinks its not correct, the
> place to report this to is likely where the clang sanitizers are
> developed.
> 
> I do understand that this is annoying but this is how C works.
> 
> About "doesn't crash on any known platform ever",
> "you can't exploit  overflows in transforms or functions like this"
> 
> undefined behavior has bitten people with unexpected results in the
> past. for example "if (a+b < a)" to test for overflow, but because the 
> condition
> can only be true in case of overflow and overflow isnt allowed in C
> the compiler didnt assemble this the way the human thought.
> 
> In the case of ps_add_squares_c(), dst[i] has to increase if iam
> not mistaken. In case of overflow it can decrease but overflow is
> not allowed so a compiler can prune anything that implies dst[] to be
> negative.
> dst[] is used downstream in checks of greater / less and in FFMAX
> In this code the compiler can assume that the sign bit is clear,
> what happens when  the compilers optimizer has false assumtations
> i dont know but i know its not good.
> 
> That said even if no such chain of conditions exist its still invalid
> code if theres undefined behavior in it

Does anyone object to this patch ?
Or does anyone have a better idea on how to fix this ?
if not id like to apply it

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable


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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Michael Niedermayer]

On Sun, Jul 02, 2017 at 02:24:53PM +0100, Rostislav Pehlivanov wrote:
> On 2 July 2017 at 03:28, Michael Niedermayer  wrote:
> 
> > Fixes: runtime error: signed integer overflow: 1965219850 + 995792909
> > cannot be represented in type 'int'
> > Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920
> >
> > Found-by: continuous fuzzing process https://github.com/google/oss-
> > fuzz/tree/master/projects/ffmpeg
> > Signed-off-by
> > :
> > Michael Niedermayer 
> > ---
> >  libavcodec/aacpsdsp_template.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_
> > template.c
> > index 9e1a95c1a1..2c0afd4512 100644
> > --- a/libavcodec/aacpsdsp_template.c
> > +++ b/libavcodec/aacpsdsp_template.c
> > @@ -26,9 +26,10 @@
> >  #include "libavutil/attributes.h"
> >  #include "aacpsdsp.h"
> >
> > -static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT (*src)[2], int
> > n)
> > +static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT
> > (*src)[2], int n)
> >  {
> >  int i;
> > +SUINTFLOAT *dst = dst_param;
> >  for (i = 0; i < n; i++)
> >  dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1], src[i][1]);
> >  }
> >
> >
> What's the issue with just _not_ fixing it here? It only occurs on fuzzed
> inputs, doesn't crash on any known platform ever, makes the code uglier and
> why? Because some fuzzer is super pedantic.
> Why not fix the fuzzer? Why not just mark this as a false positive, since
> fixing it is pointless from the standpoint of security (you can't exploit
> overflows in transforms or functions like this), and all developers hate it.

Iam not sure you understand the problem.
signed integer overflows are undefined behavior, undefined behavior
is not allowed in C.


Theres also no option to mark anything as false positive.
If the tool makes a mistake, the issue needs to be reported to its
authors and needs to be fixed.
I belive the tool is correct, if someone thinks its not correct, the
place to report this to is likely where the clang sanitizers are
developed.

I do understand that this is annoying but this is how C works.

About "doesn't crash on any known platform ever",
"you can't exploit  overflows in transforms or functions like this"

undefined behavior has bitten people with unexpected results in the
past. for example "if (a+b < a)" to test for overflow, but because the condition
can only be true in case of overflow and overflow isnt allowed in C
the compiler didnt assemble this the way the human thought.

In the case of ps_add_squares_c(), dst[i] has to increase if iam
not mistaken. In case of overflow it can decrease but overflow is
not allowed so a compiler can prune anything that implies dst[] to be
negative.
dst[] is used downstream in checks of greater / less and in FFMAX
In this code the compiler can assume that the sign bit is clear,
what happens when  the compilers optimizer has false assumtations
i dont know but i know its not good.

That said even if no such chain of conditions exist its still invalid
code if theres undefined behavior in it

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange


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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Rostislav Pehlivanov]

On 2 July 2017 at 03:28, Michael Niedermayer  wrote:

> Fixes: runtime error: signed integer overflow: 1965219850 + 995792909
> cannot be represented in type 'int'
> Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920
>
> Found-by: continuous fuzzing process https://github.com/google/oss-
> fuzz/tree/master/projects/ffmpeg
> Signed-off-by
> :
> Michael Niedermayer 
> ---
>  libavcodec/aacpsdsp_template.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_
> template.c
> index 9e1a95c1a1..2c0afd4512 100644
> --- a/libavcodec/aacpsdsp_template.c
> +++ b/libavcodec/aacpsdsp_template.c
> @@ -26,9 +26,10 @@
>  #include "libavutil/attributes.h"
>  #include "aacpsdsp.h"
>
> -static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT (*src)[2], int
> n)
> +static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT
> (*src)[2], int n)
>  {
>  int i;
> +SUINTFLOAT *dst = dst_param;
>  for (i = 0; i < n; i++)
>  dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1], src[i][1]);
>  }
>
>
What's the issue with just _not_ fixing it here? It only occurs on fuzzed
inputs, doesn't crash on any known platform ever, makes the code uglier and
why? Because some fuzzer is super pedantic.
Why not fix the fuzzer? Why not just mark this as a false positive, since
fixing it is pointless from the standpoint of security (you can't exploit
overflows in transforms or functions like this), and all developers hate it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Michael Niedermayer]

Fixes: runtime error: signed integer overflow: 1965219850 + 995792909 cannot be 
represented in type 'int'
Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
 libavcodec/aacpsdsp_template.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_template.c
index 9e1a95c1a1..2c0afd4512 100644
--- a/libavcodec/aacpsdsp_template.c
+++ b/libavcodec/aacpsdsp_template.c
@@ -26,9 +26,10 @@
 #include "libavutil/attributes.h"
 #include "aacpsdsp.h"
 
-static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT (*src)[2], int n)
+static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT (*src)[2], 
int n)
 {
 int i;
+SUINTFLOAT *dst = dst_param;
 for (i = 0; i < n; i++)
 dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1], src[i][1]);
 }
-- 
2.13.0

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