[FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior

[Ganesh Ajjanagadde]

On Sun, Nov 1, 2015 at 7:59 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Sun, Nov 1, 2015 at 7:21 PM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>
>> On Sun, Nov 1, 2015 at 7:15 PM, Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>> > On Sun, Nov 01, 2015 at 06:51:19PM -0500, Ganesh Ajjanagadde wrote:
>> >> On Sun, Nov 1, 2015 at 6:25 PM, Nicolas George <george at nsup.org> wrote:
>> >> > Le primidi 11 brumaire, an CCXXIV, Ronald S. Bultje a écrit :
>> >> >> So, is this a bug in llrint, or is this a failure to use llrint, or
>> is this
>> >> >> different from llrint? It sounds to me that llrint should be used,
>> not our
>> >> >> own alternative.
>> >> >
>> >> > Is there a sized version of the function? int64rint? Otherwise, these
>> >> > functions for the native types are as useless as the native types
>> >> > themselves.
>> >>
>> >> No, not in ISO C, or even libc AFAIK. ISO C tends to favor the
>> >> "non-sized" types in their signatures. The reason (I believe) stems
>> >> from the fact that an implementation is free to not even define the
>> >> sized types:
>> >> 7.20.1.1, point 3 - "These types are optional. However, if an
>> >> implementation provides integer types with widths of 8, 16, 32, or 64
>> >> bits, no padding bits, and (for the signed types) that have a two’s
>> >> complement representation [all platforms supported by the project], it
>> >> shall define the corresponding typedef names." -
>> >> Thus they want to limit their scope to mostly (or perharps only?)
>> stdint.h.
>> >>
>> >> Even otherwise, these functions are somewhat useless due to the
>> >> undefined behavior outside the range. All they really do is get
>> >> rounding done correctly according to the current FPU environment and
>> >> associated rounding modes, which can be manipulated in C99/C11.
>> >
>> > quite some of the undefined behavior also makes more optimizations
>> > possible for advanced compilers
>> > random silly example
>> >
>> > min = 0;
>> > for(i=0; i<N; i++) {
>> >     float c = whatever()
>> >     min = fmin(min, c);
>> >     out[i] = llrint(c);
>> > }
>> >
>> > here the compiler can remove any and all handling of NaN and +-Inf
>> > from fmin() because llrint(c) implies c is within the range
>> > represetable of the integer types
>> >
>> > with a llrint() equivalent that is defined for all cases this is not
>> > possible anymore
>>
>> Yes, which is why I have mentioned that we need to use a safe version
>> only when we need to guarantee safety, and are dealing with arbitrary
>> doubles. But such cases are there, such as the ffserver_config patch I
>> posted.
>
>
> OK, so let me sum up my current thoughts:
> - I don't really care about ffserver, so I have no opinion on whether the
> patch is appropriate. It may well be. Let's assume it should.

I don't care about ffserver either. Gave this as the simplest
illustration (and where I originally found the issue). There is at
least one example in libavfilter: af_adelay:148 (not a vulnerability
due to the check in practice, still undefined behavior as delay is
untrusted), cmdutils.c, and possibly many more. I think all agree by
now that this is a real issue in parts of the codebase, and we need a
function for this. How many places/where all, I don't know.

> - I think the name of this function is confusing. The av_clip* namespace
> seems reserved for int to int clips, so using it for a float to int
> conversion with clip is kind of weird. I would use a different namespace
> for it.

Mathematically, it is the same as clipping, albeit "lossy", but then
again all clipping is by nature "lossy" on inputs outside the clipping
range, hence the choice of name. Any ideas for a namespace for this?

> - I'm not sure the code should live in this header. av_clip* is internal
> API (afaiu), and ffserver should not use internal api. I know it does, but
> it shouldn't, so adding new api specifically so ffserver can use it is ...
> all upside-down.

It is NOT specifically for ffserver. If it was, I would not have
bothered at all with the patch.

> Maybe the code should (for now) live in ffserver, or in an
> installed header, so it's ok for apps (like ffserver) to use it, if that's
> the intention.

Not just for apps, also for libavfilter, likely libavcodec as well.
Hence went in libavutil.

>
> So, finally, maybe I'm being pedantic now, but it seems one of these usages
> overflows if we go over 2^64 = 16000 petabytes. Is that right? Do we really
> care? I mean, we're talking ffserver here, ffserver has much bigger issues
> than this.

You are right, gave it as an illustration of API usage and an issue,
albeit not a terribly important one :) - user's config is an untrusted
file.

>
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel