Re: [ft-devel] Undefined behavior

[Behdad Esfahbod]

On 14-08-11 10:15 AM, Alexei Podtelezhnikov wrote:> On Fri, Aug 8, 2014 at
4:56 PM, Behdad Esfahbod <address@hidden> wrote:
>> commit 177982e933ed6f2ab96163e14f4267f8abe89efd
>
> Seriously though, my commit does not change much, both FT_MSB and
> ft_highpow2 used to return nonsense on zero input even before this
> commit. I think I am going to apply some patches to protect against
> reaching this code with zero argument, but I am not willing to change
> FT_MSB itself.

Non-sense is not the same as undefined.  FT_MSB used to return 0 for 0.  Now
it's undefined.  See below.



On 14-08-09 02:05 PM, Alexei Podtelezhnikov wrote:>> * src/pfr/pfrobjs.c, do
you think count can be 0 here?
>
> FT_UInt    power       = 1 << FT_MSB( count );
>
> I doubt very highly that undefined behavior can mean a number between
> 0 and 31.

Actually it can.  Please take the time to understand what it means when
something is undefined in C.


> So we'll get power = 0 if count = 0, which is fine too.

Even if count becomes, say, 100, the shift operation 1 << 100 is itself
undefined in C.  In fact, that produces non-zero on existing x86 CPUs.  Here,
I fixed exactly that in glib in 2006:

  https://bugzilla.gnome.org/show_bug.cgi?id=371631


> Every case is safe, or so it seems to me.

You haven't reasoned them as being safe so far.  Plus, I'd rather the macro
API not change into undefined behavior because of an optimization that most
probably is never ever measurable.


On 14-08-09 01:50 PM, Alexei Podtelezhnikov wrote:
> On Fri, Aug 8, 2014 at 4:56 PM, Behdad Esfahbod <address@hidden> wrote:
>>
>>     * src/base/ftcalc.c (FT_MSB): Utilize gcc builtins.
>>
>> introduced undefined behavior, since __builtin_clz and __builtin_ctz have
>> undefined behavior if passed in value is zero.
>>
>> This can actually happen with degenerate glyph outlines.  It might be
>> harmless, but it's something that should be fixed.
> 
> There is just a handful of places where FT_MSB is used.
> * ftoutln.c, FT_MSB is used for scaling and you cannot really
> scale/shift 0 unsafely or undefinedly.

You are missing the point.  It's possible to end up calling FT_MSB(0) from
this call-site, and that has become undefined.  Please fix that.


> * fttrigon.c, the same reasoning applies
> * ftbbox.c, the same reasoning applies
> * src/pfr/pfrobjs.c, do you think count can be 0 here?

Didn't check these ones.


-- 
behdad
http://behdad.org/