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

[Sam Edwards]

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, [e-mail] 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

[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, [e-mail] 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.