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

[Max TenEyck Woodbury]

On 05/04/2013 05:37 PM, Sam Edwards wrote:
> On 05/04/2013 12:59 PM, Max TenEyck Woodbury wrote:
>>
>>You are trying to make this about me.  It is not. Windows fairly
>> obviously does not do this 'sanity' test.  Wine is supposed to imitate
>> windows.  To do this absolutely correctly, the whole 'sanity' test
>> should go away.
>
> This sounds like an excellent reason to write a conformance test. A test
> that succeeds on Windows but fails under Wine is a great way to convince
> everyone that the patch is necessary. It's also gives us a closer look
> at Windows's behavior under the same circumstances, so we can see
> whether Windows does this sanity check or not, and if not, how it reacts
> when aveWidth is wrong.

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.
>
> I should note, the commit that introduces the sanity check
> (21589993826cdb1cb2f87ceb94c8a188bd4a3090) also includes a test
> (dlls/gdi32/tests/font.c:3908 as of this writing) that passes under
> Windows, which could mean that Windows actually does include this sanity
> check for the width vs. the height.
>
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 isolated the problem, and came up with a fix.  Bug reports are for
>> cases where you can not yourself identify the bad code.  That this code
>> is bad is obvious when you know that it can throw an exception. The
>> only investigation absolutely needed is to report the occurrence of the
>> exception.  It happens in at least some circumstances.  Anything
>> additional is simply an invitation to delay.
>
> Are we sure that *this* code is the problem?

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.

>  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.

>  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.

> Are delays necessarily a bad thing? This bug doesn't have any security
> implications, and we aren't hurrying to catch the Wine 1.6 release
> window. We can afford to take the extra time to ensure the quality of
> the patch. :)

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)!