[PATCH 0/1] MR1192: win32u: Prevent division by zero in scale_font_metrics().
The function `scale_font_metrics()` in `win32u/font.c` was generating a division by zero exception in one of the `comctl32` tests (`status.ok`). This patch just short-circuits the ratio check in the function in case `tm->tmHeight == 0`. This was enough to allow the tests to pass. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1192
If metrics are scaled to 0 on Windows as well, then sure, we'll need to account for that when we divide by it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1192#note_12718
Ok. I'll look for some more possible side-effects of that, then I'll update the MR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1192#note_12751
Ok, I'll check it out. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1192#note_12502
From: Tarcísio Ladeia de Oliveira<wyrquill(a)gmail.com> --- dlls/win32u/font.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/win32u/font.c b/dlls/win32u/font.c index 75189f8ec5a..ca0e52be07f 100644 --- a/dlls/win32u/font.c +++ b/dlls/win32u/font.c @@ -4317,7 +4317,7 @@ static void scale_font_metrics( struct gdi_font *font, TEXTMETRICW *tm ) double scale_x, scale_y; /* Make sure that the font has sane width/height ratio */ - if (font->aveWidth && (font->aveWidth + tm->tmHeight - 1) / tm->tmHeight > 100) + if (font->aveWidth && tm->tmHeight && (font->aveWidth + tm->tmHeight - 1) / tm->tmHeight > 100) { WARN( "Ignoring too large font->aveWidth %d\n", font->aveWidth ); font->aveWidth = 0; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1192
This patch just short-circuits the ratio check in the function in case `tm->tmHeight == 0`
Hi, thanks for the patch. I'm not commenting on patch contents, but rather on symptoms you're seeing. Could you try to figure out which font triggers this in status tests? There we go through all available fonts. It's possible that our tmHeight estimation is incorrect somewhere. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1192#note_12501
I investigated `check_height_font_enumproc()` and it seems that the height calculations are correct, but a single font I have installed (`Emmentaler-Brace`) is returning `tmHeight` as 0 in `font_GetTextMetrics()`. Here is the backtrace: [emmentaler-backtrace.txt](/uploads/65d3cee47465cd151e63f4dbc50fe070/emmentaler-backtrace.txt). All other fonts are working. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1192#note_12510
Thanks, I was able to reproduce with some version of this font that I found. The issue is in calc_ppem_for_height() I believe: ``` 1932 if(pOS2->usWinAscent + windescent == 0) 1933 units = pHori->Ascender - pHori->Descender; 1934 else 1935 units = pOS2->usWinAscent + windescent; 1936 ppem = pFT_MulDiv(ft_face->units_per_EM, height, units); ``` Here 'units' is about 9000 * 2, units_per_EM for this font from 'head' is 1000. For a height of 6 this produces ppem == 0. Later in freetype_set_outline_text_metrics() this gives em_scale == 0. That scales tmAscent/tmDescent and others to 0. So, it's a bigger issue than just one metric getting reset. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1192#note_12530
Oh, got it. I hadn't dived into font.c too much so I got a bit lost. I'll try this on Windows later today and see how it handles it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1192#note_12543
Right, interesting part is to see what metrics it gets on Windows, for the same height. Note that sometimes builtin font installer application on Windows rejects fonts that are still usable, you might have to install it manually in that case - registry entry will be important. Or maybe use memory resource API is easier. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1192#note_12544
So, I edited the test to make it spill the entire contents of the TEXTMETRICA instance and ran it on Windows. It did not complain of a division by zero, but printed this out: ``` status.c:274: dpi=96 (min height: 24 or 20) SM_CYSIZE: 22 (...) Emmentaler-26 6 6 3 3 4 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 7 6 3 3 4 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 8 9 4 5 6 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 9 9 4 5 6 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 10 9 4 5 6 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 11 12 6 6 8 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 12 12 6 6 8 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 13 12 6 6 8 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 15 15 7 8 10 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 16 15 7 8 10 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 20 21 10 11 14 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 22 21 10 11 14 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 28 29 13 16 19 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 36 35 16 19 23 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 48 47 22 25 31 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 72 71 33 38 47 0 9 41 500 0 1001 1001 32 255 32 32 0 0 0 59 0 Emmentaler-Brace 6 0 0 0 0 0 0 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 7 0 0 0 0 0 0 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 8 0 0 0 0 0 0 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 9 0 0 0 0 0 0 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 10 19 9 10 18 0 0 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 11 19 9 10 18 0 0 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 12 19 9 10 18 0 0 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 13 19 9 10 18 0 0 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 15 19 9 10 18 0 0 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 16 19 9 10 18 0 0 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 20 19 9 10 18 0 16 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 22 19 9 10 18 0 16 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 28 19 9 10 18 0 16 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 36 39 20 19 37 0 16 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 48 39 20 19 37 0 16 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 72 77 38 39 73 0 16 0 400 0 1001 1001 32 255 32 32 0 0 0 59 0 (...) ``` (I'm putting Emmentaler-26 along it as a bit of a control) The first column is the font size (from the `sizes` array in the test), and the others follow this order: ``` tmHeight, tmAscent, tmDescent, tmInternalLeading, tmExternalLeading, tmAveCharWidth, tmMaxCharWidth, tmWeight, tmOverhang, tmDigitizedAspectX, tmDigitizedAspectY, tmFirstChar, tmLastChar, tmDefaultChar, tmBreakChar, tmItalic, tmUnderlined, tmStruckOut, tmPitchAndFamily, tmCharSet ``` For comparison, I added the fix in the merge request and ran it on Linux via Wine, and got this: ``` status.c:275: dpi=96 (min height: 20 or 20) SM_CYSIZE: 18 (...) Emmentaler-26 6 6 3 3 4 0 9 36 500 0 96 96 32 255 31 32 0 0 0 11 0 7 6 3 3 4 0 9 36 500 0 96 96 32 255 31 32 0 0 0 11 0 8 6 3 3 4 0 9 36 500 0 96 96 32 255 31 32 0 0 0 11 0 9 9 4 5 6 0 9 45 500 0 96 96 32 255 31 32 0 0 0 11 0 10 9 4 5 6 0 9 45 500 0 96 96 32 255 31 32 0 0 0 11 0 11 9 4 5 6 0 9 45 500 0 96 96 32 255 31 32 0 0 0 11 0 12 12 6 6 8 0 9 63 500 0 96 96 32 255 31 32 0 0 0 11 0 13 12 6 6 8 0 9 63 500 0 96 96 32 255 31 32 0 0 0 11 0 15 15 7 8 10 0 9 81 500 0 96 96 32 255 31 32 0 0 0 11 0 16 15 7 8 10 0 9 81 500 0 96 96 32 255 31 32 0 0 0 11 0 20 17 8 9 12 1 9 50 500 0 96 96 32 255 31 32 0 0 0 11 0 22 21 10 11 14 1 9 54 500 0 96 96 32 255 31 32 0 0 0 11 0 28 26 12 14 17 1 9 48 500 0 96 96 32 255 31 32 0 0 0 11 0 36 36 17 19 23 1 9 63 500 0 96 96 32 255 31 32 0 0 0 11 0 48 47 22 25 31 1 9 63 500 0 96 96 32 255 31 32 0 0 0 11 0 72 71 33 38 47 2 9 55 500 0 96 96 32 255 31 32 0 0 0 11 0 Emmentaler-Brace 6 0 0 0 0 0 16 0 400 0 96 96 255 255 31 32 0 0 0 11 0 7 0 0 0 0 0 16 0 400 0 96 96 255 255 31 32 0 0 0 11 0 8 0 0 0 0 0 16 0 400 0 96 96 255 255 31 32 0 0 0 11 0 9 0 0 0 0 0 16 0 400 0 96 96 255 255 31 32 0 0 0 11 0 10 20 10 10 18 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 11 20 10 10 18 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 12 20 10 10 18 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 13 20 10 10 18 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 15 20 10 10 18 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 16 20 10 10 18 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 20 20 10 10 18 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 22 20 10 10 18 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 28 20 10 10 18 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 36 20 10 10 18 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 48 38 19 19 37 0 16 32 400 0 96 96 255 255 31 32 0 0 0 11 0 72 58 29 29 55 0 16 16 400 0 96 96 255 255 31 32 0 0 0 11 0 (...) ``` Some things stood out: 1. The numbers in columns 2 to 4 are just slightly different. This may result from the different min height and SM_CYSIZE, but I'm not sure 2. tmAveCharWidth = 16 and tmMaxCharWidth = 16 instead of 0 in some places 3. tmDigitizedAspect[X/Y] = 96 instead of 1001 4. tmFirstChar = 255 instead of 32 5. tmDefaultChar = 31 instead of 32 6. tmPitchAndFamily = 11 instead of 59 It seems that Windows does check somewhere for tmHeight == 0 and handles it gracefully, even though that might mean the font will effectively not be printed. I looked into freetype.c and found what appears to be the source for differences 2. to 6. in freetype_set_outline_text_metrics(). I don't know much about fonts, so I'll be guessing a lot of stuff here. For 2., it seems be the following part: ``` 3746 TM.tmAveCharWidth = SCALE_X(pOS2->xAvgCharWidth); 3747 if (TM.tmAveCharWidth == 0) { 3748 TM.tmAveCharWidth = 1; 3749 } 3750 TM.tmMaxCharWidth = SCALE_X(ft_face->bbox.xMax - ft_face->bbox.xMin); ``` Removing the check in lines 3747-3749 does make it zero (as well as tmMaxCharWidth), but then some operation down the line causes it to become equal to -2147483648 (I couldn't find where). For 3. it's: ``` 3765 TM.tmDigitizedAspectX = 96; /* FIXME */ 3766 TM.tmDigitizedAspectY = 96; /* FIXME */ ``` So not really much I can do. For 4., it's: ``` 3792 TM.tmFirstChar = pOS2->usFirstCharIndex; /* Should be the first char in the cmap */ ``` This one seems to be more correct than in Windows (although I don't know how it would affect compatibility). There is only a single character in the entire font (the left brace), and it's in the font's private use area, so it makes sense to default to 0xff as to say "there's effectively nothing here". I guess 0x20 is just the default in case pOS2->usFirstCharIndex == pOS2->usLastCharIndex. For 5., it's: ``` 3795 if(pOS2->usFirstCharIndex <= 1) 3796 TM.tmBreakChar = pOS2->usFirstCharIndex + 2; 3797 else if (pOS2->usFirstCharIndex > 0xff) 3798 TM.tmBreakChar = 0x20; 3799 else 3800 TM.tmBreakChar = pOS2->usFirstCharIndex; 3801 TM.tmDefaultChar = TM.tmBreakChar - 1; ``` This one seems to be more consistent than in Windows. The fonts don't have complete charsets, so being off by one probably doesn't change much. I guess 0x20 is more likely to be supported in exotic fonts than 0x1f? For 6., it's probably that: ``` WINE: tmPitchAndFamily = FF_DONTCARE | TMPF_DEVICE | TMPF_VECTOR | TMPF_FIXED_PITCH WINDOWS: tmPitchAndFamily = FF_MODERN | TMPF_DEVICE | TMPF_VECTOR | TMPF_FIXED_PITCH ``` I guess FF_MODERN is also a default? So, it seems that checking for tmHeight == 0 might be necessary in places where a division might happen (to account for those weird fonts, at least in tests), and the calculation of tmAveCharWidth and tmMaxCharWidth *might* need some changes too. I guess we could also add an ERR call to catch fonts that might cause problems because of tmHeight == 0 or ppem == 0. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1192#note_12607
participants (2)
-
Nikolay Sivov (＠nsivov) -
Tarcísio Ladeia de Oliveira