[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 <Reimar.Doeffinger at gmx.de> wrote:
> 
>> On 09.07.2017, at 16:08, "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
>>> Hi,
>>>
>>> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger at gmx.de>
>>> wrote:
>>>   
>>>> On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje at gmail.com> wrote:  
>>>>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer  
>>>> <michael at niedermayer.cc>  
>>>>> 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>