[FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()
jamrial at gmail.com
Mon Jul 10 15:29:31 EEST 2017
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:
>>> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger at gmx.de>
>>>> 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>
>>>>>> 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,
> 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
More information about the ffmpeg-devel