--- On Sun, 5/5/13, Max TenEyck Woodbury max@mtew.isa-geek.net wrote:
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)!
I'd like to mention two things:
- there were(are?) overflows/underflows within Freetype itself, up to and including 2.4.11 - the fixes went into trunk, but AFAIK 2.4.12 isn't release yet. That's specifically affect 32-bit platform, and emulated styles (i.e. where the application requires a font style which cannot be provided by any one font, but needed to be "synthesized" by fontconfig and freetype).
- there a few well-known "open-source" fonts which microsoft's gdiplus does not like and crash on them, but nonetheless, windows users never encounter the problems, because they typically have the proprietary MS equivalent, and therefore do not need those fonts at all.
I suggest - (1) check out freetype trunk to see if it helps, or at least, patch your freetype with those fixes from end of January; (2) modify the "Avoid division by zero" patch to emit the font's typeface name whenever the condition occur, and just run it on the affected system to see which typeface wine doesn't like?
Does this sound reasonable?