[PATCH] dlls/gdi32/fretype.c: Avoid division by zero.

[Sam Edwards]

On 05/04/2013 05:13 PM, Max TenEyck Woodbury wrote:
>
> Having wine throw an exception where it did not do so before is another
> kind of problem indicator.  One that hardly needs a special conformance
> test. 
> Hmm.  As I suspected, that is a single point pass only test.  It does
> not explore any of the possible fail conditions.  Thus, it is also
> definitely a possibility that Windows makes no such sanity check.
>
I suggested the conformance test for removing the sanity check, not for 
the crash. We need to know for sure whether Windows makes no such sanity 
check before we act on that possibility.

> Almost certainly.  Without this patch, the 'make test' suit throws a
> divide by zero exception and brings up a dialog box.  With this patch,
> it does not.  I believe that is sufficient to convict the unpatched
> code of having something wrong with it.
The flaw in your logic is that you're assuming the code which generates 
the exception is necessarily the code that's written incorrectly. By the 
same logic, you could patch out the crash handler and say "Look, no more 
crash dialog! It's fixed!"

I agree that the unpatched code is definitely dividing by zero in this 
case; but according to Dmitry, tmHeight is never supposed to be 0. If 
he's correct, it means that this code is correct (given the assumption), 
but something else is bugged (thereby violating that "contract of 
behavior") and that's what should be fixed instead.

>
>>  As Dmitry has said,
>> tmHeight should never be 0, so it's probably valid to assume
>> tmHeight!=0.
>
> But that assumption can be checked.  Currently there is no such check.
> 'Should' in this context is a very bad word and has no place at the
> foundation of an argument about what actually happens.
I agree that this assumption can/should be checked, but the appropriate 
place to do that is in unit tests. This also verifies that the 
assumption holds on Windows as well, ensuring that Wine's behavior 
doesn't diverge from that of Windows.

And yeah, sorry for using "should" - it is kind of a weak word, in 
retrospect... I think "must" would have been a better choice, but I'm 
not certain enough for a word so strong: We need to see what tmHeight 
does on Windows (that means tests!) before we can argue it one way or 
another. :)

>
>>  The bug may instead be in allowing the font to load with
>> tmHeight=0, and this is merely the first crash that the problem causes
>> for you. But what about apps that divide by tmHeight under the same
>> assumption? We can't change those, so it's better to fix tmHeight.
>
> If the APP does the division, that is the APP's problem.  Wine, on the
> other hand should not throw an exception, and it did NOT do so until
> recently.  Whoever wrote the new code (Dmitry?) should have put in the
> check or made the test work without doing a division.  The fact that it
> fails is the problem being addressed.
If the app, which was made for Windows, works on Windows, but not on 
Wine, then it is our problem, not the app's. The crash that you're 
seeing is a clear indicator of *something* being wrong, but we don't yet 
know specifically what.

>
> I have no objection to someone writing an alternative patch and backing
> this one out when that patch goes in, but until then, this patch, or
> something like it, needs to be applied.  With wine throwing the
> exception, some Apps are going to fail that would not fail otherwise.
> That is definitely a 'Bad Thing' and should be fixed ASAP (like right
> now)!
Crashes are definitely a Bad Thing(TM), but this patch may not fix all 
of them (e.g. with apps that assume tmHeight!=0) without actually fixing 
the underlying problem. In fact, this patch itself could be a Bad 
Thing(TM) -- if something is wrong with tmHeight calculation/scaling, 
the patch might only mask the real issue, which would make a proper fix 
take even longer.

-----

I would like to reiterate: I simply don't know what tmHeight is supposed 
to do. According to Dmitry, the real bug is having tmHeight=0. If 
Windows allows tmHeight=0 under some circumstance, then your patch is 
the right thing to do, because we must prevent a division by zero in 
that case. If Windows absolutely disallows tmHeight=0, then the bug is 
actually that Wine allows tmHeight=0, and so we should be focusing on 
that instead.

Hope this helps,
Sam