Re: [PATCH] dlls/gdi32/fretype.c: Avoid division by zero.
max(a)mtew.isa-geek.net wrote:
+ if ( font->aveWidth && font->potm->otmTextMetrics.tmHeight ) { + if (((font->aveWidth + font->potm->otmTextMetrics.tmHeight - 1) / + font->potm->otmTextMetrics.tmHeight) > 100) { WARN("Ignoring too large font->aveWidth %d\n", font->aveWidth); font->aveWidth = 0; }
In which case font->potm->otmTextMetrics.tmHeight is going to be 0? -- Dmitry.
On 05/04/2013 12:38 AM, Dmitry Timoshkov wrote:
max(a)mtew.isa-geek.net wrote:
+ if ( font->aveWidth && font->potm->otmTextMetrics.tmHeight ) { + if (((font->aveWidth + font->potm->otmTextMetrics.tmHeight - 1) / + font->potm->otmTextMetrics.tmHeight) > 100) { WARN("Ignoring too large font->aveWidth %d\n", font->aveWidth); font->aveWidth = 0; }
In which case font->potm->otmTextMetrics.tmHeight is going to be 0?
I am not sure what you are asking. I have had this particular division throw an exception for some font I have installed, but I have no idea which one at the moment. So if you are implying that font->aveWidth==0 is always true if font->potm->otmTextMetrics.tmHeight==0, that seems not to be the case.
Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
+ if ( font->aveWidth && font->potm->otmTextMetrics.tmHeight ) { + if (((font->aveWidth + font->potm->otmTextMetrics.tmHeight - 1) / + font->potm->otmTextMetrics.tmHeight) > 100) { WARN("Ignoring too large font->aveWidth %d\n", font->aveWidth); font->aveWidth = 0; }
In which case font->potm->otmTextMetrics.tmHeight is going to be 0?
I am not sure what you are asking. I have had this particular division throw an exception for some font I have installed, but I have no idea which one at the moment. So if you are implying that font->aveWidth==0 is always true if font->potm->otmTextMetrics.tmHeight==0, that seems not to be the case.
If font->potm->otmTextMetrics.tmHeight is 0 then the font is invalid and should not be loaded and used at all. There is no point is adding checks for things that shouldn't happen, and may cause various bad things in other places of code as well. -- Dmitry.
On 05/04/2013 01:56 AM, Dmitry Timoshkov wrote:
Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
+ if ( font->aveWidth && font->potm->otmTextMetrics.tmHeight ) { + if (((font->aveWidth + font->potm->otmTextMetrics.tmHeight - 1) / + font->potm->otmTextMetrics.tmHeight) > 100) { WARN("Ignoring too large font->aveWidth %d\n", font->aveWidth); font->aveWidth = 0; }
In which case font->potm->otmTextMetrics.tmHeight is going to be 0?
I am not sure what you are asking. I have had this particular division throw an exception for some font I have installed, but I have no idea which one at the moment. So if you are implying that font->aveWidth==0 is always true if font->potm->otmTextMetrics.tmHeight==0, that seems not to be the case.
If font->potm->otmTextMetrics.tmHeight is 0 then the font is invalid and should not be loaded and used at all. There is no point is adding checks for things that shouldn't happen, and may cause various bad things in other places of code as well.
Sorry, but WRONG. It HAPPENS. Someplace in the collection of fonts I have accumulated from standard sources, there is at least one that has this property but is otherwise useful. You can say all you want about 'it should not be loaded', but it does load. This deals with the problem. Further, this was NOT a problem until the patch that introduced the SCALE_X and SCALE_Y macros. I did a bisection and that was the commit that introduced the divide by zero exception. Before that patch, there was no exception thrown by this code. With that patch and since then it throws a divide by zero consistently. I did NOT change my font mix. The wine test suite simply started throwing a divide by zero exception at that point. I have not gone over that patch in detail, but its intent seems to have been clear. There was just some detail that messed up. This patch corrects the problem. If you REALLY think the font should not load, you should add code to reject the font with an appropriate diagnostic, not have this code throw a divide by zero exception and abort execution. Until you do that, this patch is needed.
Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
If you REALLY think the font should not load, you should add code to reject the font with an appropriate diagnostic, not have this code throw a divide by zero exception and abort execution. Until you do that, this patch is needed.
First, what is needed, is an investigation what is happening exactly, that's what I asked about in the first place. If you belive that tmHeight being 0 is normal and should be accepted, then you need to provide more details and probably a test case. -- Dmitry.
On 05/04/2013 10:50 AM, Dmitry Timoshkov wrote:
Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
If you REALLY think the font should not load, you should add code to reject the font with an appropriate diagnostic, not have this code throw a divide by zero exception and abort execution. Until you do that, this patch is needed.
First, what is needed, is an investigation what is happening exactly, that's what I asked about in the first place. If you belive that tmHeight being 0 is normal and should be accepted, then you need to provide more details and probably a test case.
Sorry, but that is not the case. While an investigation would be useful, the question that it is normal or not is not relevant. The simple fact is that it happens. Since it DOES happen, the patch should be applied, at least until some code elsewhere assures that it can not happen. If you want to throw in a 'FIXME' to the effect that it should not happen and a test is needed to assure that, by all means do so.
Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
If you REALLY think the font should not load, you should add code to reject the font with an appropriate diagnostic, not have this code throw a divide by zero exception and abort execution. Until you do that, this patch is needed.
First, what is needed, is an investigation what is happening exactly, that's what I asked about in the first place. If you belive that tmHeight being 0 is normal and should be accepted, then you need to provide more details and probably a test case.
Sorry, but that is not the case. While an investigation would be useful, the question that it is normal or not is not relevant. The simple fact is that it happens. Since it DOES happen, the patch should be applied, at least until some code elsewhere assures that it can not happen. If you want to throw in a 'FIXME' to the effect that it should not happen and a test is needed to assure that, by all means do so.
I wonder why did you bother with sending a patch at all? You don't want to provide any details or do any investigation. With such an attitude just open a bug report in bugzilla, and let somebody else do real work for you, but even then you will be asked to provide at least some details. -- Dmitry.
On 05/04/2013 11:30 AM, Dmitry Timoshkov wrote:
Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
If you REALLY think the font should not load, you should add code to reject the font with an appropriate diagnostic, not have this code throw a divide by zero exception and abort execution. Until you do that, this patch is needed.
First, what is needed, is an investigation what is happening exactly, that's what I asked about in the first place. If you belive that tmHeight being 0 is normal and should be accepted, then you need to provide more details and probably a test case.
Sorry, but that is not the case. While an investigation could be useful, the question that it is normal or not is not relevant. The simple fact is that it happens. Since it DOES happen, the patch should be applied, at least until some code elsewhere assures that it can not happen. If you want to throw in a 'FIXME' to the effect that it should not happen and a test is needed to assure that, by all means do so.
I wonder why did you bother with sending a patch at all? You don't want to provide any details or do any investigation. With such an attitude just open a bug report in bugzilla, and let somebody else do real work for you, but even then you will be asked to provide at least some details.
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. The patch merely reduces the problem from totally unacceptable to tolerable. The original code snippet is low quality. There should not be any division in the 'sanity' test. As I said, it really does not belong in wine. If you insist on including it in something like its original form, multiply both sides of the comparison by the 'denominator'. Maybe throw in a couple of 'iabs(...)'s. Why send the patch? Because, for me, all versions of wine have been unacceptable since the problem appeared. Wine simply should not throw this exception. An application that uses a font with this problem will blow up. The standard 'make test' suite on my machine has the problem. 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.
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.
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.
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? As Dmitry has said, tmHeight should never be 0, so it's probably valid to assume tmHeight!=0. 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.
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. :)
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)!
--- On Sun, 5/5/13, Max TenEyck Woodbury <max(a)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?
On 05/04/2013 07:50 PM, Hin-Tak Leung wrote:
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?
Actually quite reasonable, particularly the part about identifying the troublesome font. However, I doubt that it would be accepted by the others.
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
OK. Let's summarize: There are some fonts where tmHeight is in fact 0. If Hin-Tak Leang is correct, these may be Open-Source fonts possibly with proprietary equivalents. Since I have hundreds of fonts installed on my system, it is almost certain that I have one or more. Identifying which without support in wine is a large task, not to be undertaken lightly. Dmitry argues that this should not happen. Fine :-(. That's a Coulda, Shoulda, Woulda argument. When it meets reality, it holds no water. It is the reality that has to be dealt with. The code in the 'sanity test' is too low quality. It throws divide by zero exceptions. That is *not* acceptable. The patch I made leaves that code practically unchanged and works around only the exception problem. It is an absolutely minimalist change. It changes only what is absolutely necessary. A better patch is possible, but it hides the direct source of the problem (which makes the designation 'better' debatable): + if ( font->aveWidth ) { + if ((font->aveWidth + font->potm->otmTextMetrics.tmHeight - 1) > + (font->potm->otmTextMetrics.tmHeight) * 100)) { WARN("Ignoring too large font->aveWidth %d\n", font->aveWidth); font->aveWidth = 0; } } Either my original patch or this one *must* be applied to remove the possibility of a divide by zero exception. All discussion about what *SHOULD* be done when tmHeight is 0 is another issue and has *NOTHING* directly to do with this patch. It is, however, a significant question, and does need to be investigated. It probably deserves a bugzilla entry, but it is *ANOTHER ISSUE*. As part of *THAT* issue, I totally think the current 'conformance test' is inadequate and needs expansion, but I am *NOT* the person to do that. *NOR* am I the person who would be best at addressing *THAT* problem.
On 05/04/2013 08:27 PM, Max TenEyck Woodbury wrote:
OK. Let's summarize:
There are some fonts where tmHeight is in fact 0. If Hin-Tak Leang is correct, these may be Open-Source fonts possibly with proprietary equivalents. Since I have hundreds of fonts installed on my system, it is almost certain that I have one or more. Identifying which without support in wine is a large task, not to be undertaken lightly.
Let's try to fully understand this problem and identify a font that causes the issue. Since you already have one installed in your font library, you could just run the tests with the attached patch applied and make quick work of identifying the troublesome font. Maybe (hopefully) it's an easy font to obtain so all of us can get a better look at what's going on. I'm going to *guess* that tmHeight being 0 is the actual problem, but we won't know until we try the same font on Windows and see what Windows does. If Windows also produces tmHeight=0, then this patch makes perfect sense. If Windows gives a non-zero tmHeight under the same circumstances, then we know the problem is in the font loader, and we'll fix that instead, thus making this patch unnecessary because tmHeight will always be nonzero anyway.
On 05/05/2013 12:09 AM, Sam Edwards wrote:
On 05/04/2013 08:27 PM, Max TenEyck Woodbury wrote:
OK. Let's summarize:
There are some fonts where tmHeight is in fact 0. If Hin-Tak Leang is correct, these may be Open-Source fonts possibly with proprietary equivalents. Since I have hundreds of fonts installed on my system, it is almost certain that I have one or more. Identifying which without support in wine is a large task, not to be undertaken lightly.
Let's try to fully understand this problem and identify a font that causes the issue. Since you already have one installed in your font library, you could just run the tests with the attached patch applied and make quick work of identifying the troublesome font. Maybe (hopefully) it's an easy font to obtain so all of us can get a better look at what's going on.
I'm going to *guess* that tmHeight being 0 is the actual problem, but we won't know until we try the same font on Windows and see what Windows does. If Windows also produces tmHeight=0, then this patch makes perfect sense.
If Windows gives a non-zero tmHeight under the same circumstances, then we know the problem is in the font loader, and we'll fix that instead, thus making this patch unnecessary because tmHeight will always be nonzero anyway.
$ grep ' has tmHeight=0, aveWidth=' 201305055-make-test-native.log err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'RiordonFancy' has tmHeight=0, aveWidth=0!
I am very interested in this whole situation because I have run into a similar font issue. Many Mac fonts have an AveCharWidth of a negative value. These fonts do not work well in Wine because of this. Doing some investigation I have found that the given fonts will not install on windows 7. Windows 7 reports the font as corrupted. So Dmitry is right, the font is not proper for windows. However in all honestly WINE is never running on windows. We are running on other OS (OS/X Linux) and it seems like a logical thing to allow a user to use the fonts native on their systems. I would like to think we could find a way to load and handle these guest fonts that the user will expect to have be function. Even if the font would not be a normal windows font. -aric On 5/4/13 11:38 PM, Max TenEyck Woodbury wrote:
On 05/05/2013 12:09 AM, Sam Edwards wrote:
On 05/04/2013 08:27 PM, Max TenEyck Woodbury wrote:
OK. Let's summarize:
There are some fonts where tmHeight is in fact 0. If Hin-Tak Leang is correct, these may be Open-Source fonts possibly with proprietary equivalents. Since I have hundreds of fonts installed on my system, it is almost certain that I have one or more. Identifying which without support in wine is a large task, not to be undertaken lightly.
Let's try to fully understand this problem and identify a font that causes the issue. Since you already have one installed in your font library, you could just run the tests with the attached patch applied and make quick work of identifying the troublesome font. Maybe (hopefully) it's an easy font to obtain so all of us can get a better look at what's going on.
I'm going to *guess* that tmHeight being 0 is the actual problem, but we won't know until we try the same font on Windows and see what Windows does. If Windows also produces tmHeight=0, then this patch makes perfect sense.
If Windows gives a non-zero tmHeight under the same circumstances, then we know the problem is in the font loader, and we'll fix that instead, thus making this patch unnecessary because tmHeight will always be nonzero anyway.
$ grep ' has tmHeight=0, aveWidth=' 201305055-make-test-native.log err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'RiordonFancy' has tmHeight=0, aveWidth=0!
On 05/04/2013 10:38 PM, Max TenEyck Woodbury wrote:
$ grep ' has tmHeight=0, aveWidth=' 201305055-make-test-native.log err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'Emmentaler-Brace' has tmHeight=0, aveWidth=3! err:font:get_text_metrics Font named 'RiordonFancy' has tmHeight=0, aveWidth=0!
The Emmentaler-Brace font is part of the LilyPond music engraving program, so I was able to track it down without too much effort. On both Windows (XP) and Wine, the .otf loads without any trouble. When calling CreateFontIndirect with lfHeight of 1-9, the tm{Height,Ascent,Descent} are, in fact, 0 on both platforms. lfHeight values of 10 or greater produce nonzero values for tm{Height,Ascent,Descent}. So, Wine's font loader seems to be working appropriately and thus the sane thing to do is to make sure tmHeight=0 is handled gracefully. As for RiordonFancy, I don't know, but aveWidth=0 in that case, so that doesn't really affect us here.
On 05/05/2013 06:27 PM, Sam Edwards wrote:
So, Wine's font loader seems to be working appropriately and thus the sane thing to do is to make sure tmHeight=0 is handled gracefully.
Just to make this clear, the most recent version of this patch is such a graceful handling, right?
On Sat, 04 May 2013 19:13:07 -0400, Max TenEyck Woodbury wrote:
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)!
Hi, I'm an author of the SCALE_Y patch. Could you try my patch? It can be obtianed from http://bugs.winehq.org/show_bug.cgi?id=33424#c4 . As far as my investigation, fabs() in freetype_SelectFont() makes the font height (lfHeight) to -2147483648 (=0x80000000). SCALE_Y patch doesn't check ppem value, just left shifts it. So the issue occurs. I'm currently investing why fabs returns so huge value. But the issue seems to affect another applications, and conformance tests show that there is upper bound for lfHeight in Windows. Therefore I implement upperbounds test in advance. Regards, Akihiro Sagawa
participants (6)
-
Akihiro Sagawa -
Aric Stewart -
Dmitry Timoshkov -
Hin-Tak Leung -
Max TenEyck Woodbury -
Sam Edwards