[PATCH] dlls/gdi32/freetype.c: A better divide by zero fix, report bad fonts.

[Max TenEyck Woodbury]

On 05/07/2013 08:15 AM, Sam Edwards wrote:
> On 05/06/2013 03:05 PM, Max TenEyck Woodbury wrote:
>> Just to make this clear, the most recent version of this patch is such
>> a graceful handling, right?
>
> I haven't worked on gdi32/freetype.c much, so I wouldn't be the one to
> say for sure (you should probably talk to Alexandre Julliard, Dmitry
> Timoshkov, or Huw Davies), but I'll give you my thoughts anyway:
>
> On 05/05/2013 12:48 PM, max at mtew.isa-geek.net wrote:
>> +       /* HACKHACK: If a font has tmHeight=0, let us know. */
>> +       if (!font->potm->otmTextMetrics.tmHeight) {
>> +           ERR("Font named '%s' has tmHeight=0, aveWidth=%d!\n",
>> font->ft_face->family_name, font->aveWidth);
>> +       }
>> +
>
> Since we've learned that tmHeight=0 is not actually an error, it doesn't
> make sense to log it using ERR(...). I would suggest a TRACE(...) or
> (preferably) nothing at all. Also, when I wrote the ERR message patch
> for you, it was intended only to help you locate the problematic font
> and wasn't meant to be included in upstream Wine. (Which is why I marked
> it with HACKHACK and didn't bother to use debugstr_a.)
>
> As for the actual check, it makes sense to consider what the math is
> trying to do: If aveWidth is extremely large (relative to tmHeight),
> then assume aveWidth is wrong and set it to 0. If tmHeight is 0, then it
> seems reasonable to assume that aveWidth isn't valid either.
>
> I'm not sure why the sanity check cares to do this:
> (aveWidth+tmHeight-1) / tmHeight > 100
> When it can just do this:
> (aveWidth+tmHeight-1) / tmHeight > 100
> = (aveWidth-1)/tmHeight + 1 > 100
> = (aveWidth-1)/tmHeight > 99
> = (aveWidth-1) > tmHeight*99
> = aveWidth > tmHeight*99 + 1
> ≈ aveWidth > tmHeight*100
>
> This allows us to write out tmHeight just once, and is also much easier
> to understand at a glance.
>
>
>
> So, I would just simplify the whole sanity check to something like:
>
> /* Make sure that the font has sane width/height ratio */
> if (font->aveWidth > font->potm->otmTextMetrics.tmHeight * 100)
> {
> WARN("Ignoring too large font->aveWidth %d\n", font->aveWidth);
> font->aveWidth = 0;
> }
>
> But even then, the sanity check itself sounds like a workaround for a
> much deeper bug. Maybe add a "FIXME: Investigate why this is necessary"
> comment while you're at it?
>
> Best,
> Sam
>
OK.  Could you please put in a patch that removes the divide by zero
crash?  You obviously have a much better understanding of the situation
than I do, but the crash still is a Bad Thing and needs to go away.