[FFmpeg-devel] [PATCH] avcodec/fft_template: Fix multiple runtime error: signed integer overflow: -1943918714 - 1935113003 cannot be represented in type 'int'

[Ivan Kalvachev]

On 6/10/17, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Thu, Jun 8, 2017 at 8:57 PM, Michael Niedermayer <michael at niedermayer.cc>
> wrote:
>
>> On Thu, Jun 08, 2017 at 06:35:07PM -0400, Ronald S. Bultje wrote:
>> > Hi,
>> >
>> > On Thu, Jun 8, 2017 at 6:10 PM, Michael Niedermayer
>> <michael at niedermayer.cc>
>> > wrote:
>> >
>> > > Signed value in
>> > > Unsigned
>> > > INTeger type
>> >
>> > [..]
>> > > Both SUINT and unsigned should produce identical binaries
>> >
>> > This seems to go against the rule that code should be as simple as
>> possible.
>> >
>> > Unsigned is simpler than SUINT if the outcome is the same.
>>
>> You can simply add the part of my mail here as awnser that you snipped
>> away:
>>
>> "But it makes the code hard to understand and maintain because these
>>  values are not positive integers but signed integers. Which for
>>  C standard compliance need to be stored in a unsigned type."
>>
>> A type that avoids the undefinedness of signed but is semantically
>> signed is correct, unsigned is not.
>>
>> If understandable code and maintainable code has no value to you,
>> you would favour using single letter variables exclusivly and would
>> never use typedef.
>> But you do not do that.
>>
>> I fail to understand why you insist on using unsigned in place of a
>> more specific type, it is not the correct nor clean thing to do.
>
>
> It's not just me, it appears to be most of us. Can't you just step back at
> some point and be like "ok, I'll let the majority have their way"?

There was actually a technical discussion undergoing,
until your regular "majority" group came with strong words,
opinions, and comments that clearly indicate that you don't
understand the issue at hand.

I'll try to explain the issue one more time.

---

You all know that singed overflow is undefined.
In gcc there are two options to define/control the behavior of it:
-fwrapv - defines signed overflow as wrapping around, just like unsigned.
-ftrapv - causes a trap exception, could lead to termination of the program.

Now, as the article
  http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html
explains in detail, undefined behavior allows certain
optimizations, like loop optimizations, pointer arithmetic etc..
Using -fwrapv globally might lead to a significant slow down.

On the other side, some of these optimization might lead to security
exploits e.g. if they optimize-out checks in the code, that are there to
prevent overflows.

So there is a possible scenario, where some entity that want to secure the
video playback of their browser, would like to enable a bunch of
runtime checking functionality, like -fstack-protector and -fwrapv.

Now, we get to the FFT.

As Rostislav (atomnuker) said, it is not big issue if
overflow happens in fft calculation and produces wrong result.

However if -ftrapv is enabled, it may crash the whole program (!!)
(instead of producing a short noise).

To prevent that Michael changes the code so a signed variable is
converted to unsigned for the operations that could overflow
and converted back to signed for operations that need sign extension.

He is using a new typedef,
so the developers(!) could know that this type contains signed value,
while the type for the compiler(!) is actually unsigned.

Now...
I would be happy if you all could think of a better way to get the same result,
that doesn't involve changing types back and forth.

All I can think of is:
1. Moving the functions to a special file and compiling it with
-fwrapv -fno-trapv.
In the fft case this might be extra hard, as the file is actually a template...
2. Asking gcc for attribute that defines the behavior, for a single
function or code block.
3. Asking gcc for attribute that defines the behavior, for a variable or type.
4. Defining that behavior by the standard committee in next C
standard. Maybe with new standard type. Or making "int32_t" wrap,
while keeping "int" undefined.


Michael solution may look ugly, make the code
harder to understand, but it:
1. Works Now.
2. Does work on all compilers.
3. Doesn't make the code slower.
4. Doesn't complicate the build system.


Of course, as FFmpeg developer, it is your right to initiate a vote
that would prevent Michael from trying to make FFmpeg more secure.
He has always complied with official decisions.

However this might turn into publicity nightmare,
as security community is known to overreact
on certain topics.