[PATCH 0/1] MR1055: gdi32: Don't stackoverflow if substitute doesn't change anything
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53787 Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055
From: Fabian Maurer <dark.shadow4(a)web.de> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53787 Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/gdi32/uniscribe/opentype.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/gdi32/uniscribe/opentype.c b/dlls/gdi32/uniscribe/opentype.c index c44639cb993..718bff767f9 100644 --- a/dlls/gdi32/uniscribe/opentype.c +++ b/dlls/gdi32/uniscribe/opentype.c @@ -894,6 +894,8 @@ static INT GSUB_apply_SingleSubst(const OT_LookupTable *look, WORD *glyphs, INT if (GSUB_is_glyph_covered((const BYTE*)ssf1+offset, glyphs[glyph_index]) != -1) { TRACE(" Glyph 0x%x ->",glyphs[glyph_index]); + if (GET_BE_WORD(ssf1->DeltaGlyphID) == 0) + return GSUB_E_NOGLYPH; glyphs[glyph_index] = glyphs[glyph_index] + GET_BE_WORD(ssf1->DeltaGlyphID); TRACE(" 0x%x\n",glyphs[glyph_index]); return glyph_index + write_dir; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1055
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124969 Your paranoid android. === debian11 (32 bit report) === mmdevapi: render.c:1116: Test marked flaky: Position 54144 too far after playing 100ms === debian11 (build log) === Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24848. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24848. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24848.
Nikolay Sivov (@nsivov) commented about dlls/gdi32/uniscribe/opentype.c:
if (GSUB_is_glyph_covered((const BYTE*)ssf1+offset, glyphs[glyph_index]) != -1) { TRACE(" Glyph 0x%x ->",glyphs[glyph_index]); + if (GET_BE_WORD(ssf1->DeltaGlyphID) == 0) + return GSUB_E_NOGLYPH;
This does not lookup right. As I understand GSUB_E_NOGLYPH is used when lookup wasn't applied, but for example harfbuzz (and corresponding dwrite logic) does not check for zero delta, and lookup considered applied there. What exactly happens, and why zero delta is a problem? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_10596
On Thu Oct 13 19:31:48 2022 +0000, Nikolay Sivov wrote:
This does not lookup right. As I understand GSUB_E_NOGLYPH is used when lookup wasn't applied, but for example harfbuzz (and corresponding dwrite logic) does not check for zero delta, and lookup considered applied there. What exactly happens, and why zero delta is a problem? GSUB_apply_feature_all_lookups calls itself infinitely, until stackoverflow.
As I understand it, it's supposed to apply lookups until none is left. But when we add 0 to a glyph, the lookup doesn't change anything, and we keep trying to apply the same lookup forever. You can test with the program linked at https://bugs.winehq.org/show_bug.cgi?id=53787 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_10599
On Thu Oct 13 20:04:53 2022 +0000, Fabian Maurer wrote:
GSUB_apply_feature_all_lookups calls itself infinitely, until stackoverflow. As I understand it, it's supposed to apply lookups until none is left. But when we add 0 to a glyph, the lookup doesn't change anything, and we keep trying to apply the same lookup forever. You can test with the program linked at https://bugs.winehq.org/show_bug.cgi?id=53787 I see it's significantly different to what happens in dwrite currently. Especially recursing part is weird. The point is though that matched coverage even with zero delta is considered "applied" and should skip following subtables.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_10600
On Thu Oct 13 20:16:38 2022 +0000, Nikolay Sivov wrote:
I see it's significantly different to what happens in dwrite currently. Especially recursing part is weird. The point is though that matched coverage even with zero delta is considered "applied" and should skip following subtables. Not sure how to do that though, any pointers?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_10620
On Thu Oct 13 20:40:28 2022 +0000, Fabian Maurer wrote:
Not sure how to do that though, any pointers? I'm not sure either. Best way is to reuse what we have in dwrite now, but it's a lot of work. To patch something quickly right now, I'd remove this recursive call. I don't understand why it's there.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_10678
On Fri Oct 14 13:29:44 2022 +0000, Nikolay Sivov wrote:
I'm not sure either. Best way is to reuse what we have in dwrite now, but it's a lot of work. To patch something quickly right now, I'd remove this recursive call. I don't understand why it's there. I think it is because an applied lookup can lead to another lookup being applied, i.e. restart from 0 again. Not sure though.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_10754
On Fri Oct 14 16:44:58 2022 +0000, Fabian Maurer wrote:
I think it is because an applied lookup can lead to another lookup being applied, i.e. restart from 0 again. Not sure though. The way it works in dwrite right now, which is how it works in harfbuzz I took this from:
``` foreach feature in features foreach lookup in feature.lookups { for first subtable that matches -> apply this subtable and return } ``` So, it shouldn't skip lookups after some lookup was applied, it should skip subtables after first match, but still iterate through all lookups. That's a major difference. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_10758
On Fri Oct 14 17:38:43 2022 +0000, Nikolay Sivov wrote:
The way it works in dwrite right now, which is how it works in harfbuzz I took this from: ``` foreach feature in features foreach lookup in feature.lookups { for first subtable that matches -> apply this subtable and return } ``` So, it shouldn't skip lookups after some lookup was applied, it should skip subtables after first match, but still iterate through all lookups. That's a major difference. Are you sure it works exactly like that in gdi32 as well? We don't seem to have tests for that, and I don't feel comfortable just removing the recursive call. I mean there's probably a reason it was done like this...
Anyways, I just wanted to fix a stackoverflow, didn't expect to rewrite the logic for glyph lookup. Should I really change that with the superficial knowledge I have? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_10850
On Sat Oct 15 16:54:24 2022 +0000, Fabian Maurer wrote:
Are you sure it works exactly like that in gdi32 as well? We don't seem to have tests for that, and I don't feel comfortable just removing the recursive call. I mean there's probably a reason it was done like this... Anyways, I just wanted to fix a stackoverflow, didn't expect to rewrite the logic for glyph lookup. Should I really change that with the superficial knowledge I have? I don't know what you should do, I'm just pointing out why I think this change is conceptually not looking right.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_10858
On Sun Oct 16 13:10:27 2022 +0000, Nikolay Sivov wrote:
I don't know what you should do, I'm just pointing out why I think this change is conceptually not looking right. The recursive behaviour comes from commit 9a6cf4a39135. We'd want to create a test font which would trigger this behaviour to know what we should do.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_11491
On Thu Oct 20 10:23:26 2022 +0000, Huw Davies wrote:
The recursive behaviour comes from commit 9a6cf4a39135. We'd want to create a test font which would trigger this behaviour to know what we should do. It's not trivial to draw such conclusions from a test font. I was referencing this part of harfbuzz logic, that is a top level loop that applies lookups [1]. hb_ot_map_t::apply() iterates through stages (that we don't have in our usp), and applies all lookups for each stage, without skipping. apply_string<Proxy> is what iterates through subtables for a given lookup, skipping after first subtable that covers the glyph [2], there is no check there to see if glyph actually changes, only that current glyph is in the coverage.
But then again, there are other serious discrepancies between how lookups are treated, wrt sorting/duplicates eliminations, and stages that I mentioned. [1] https://github.com/harfbuzz/harfbuzz/blob/970321db7bddbe8c579b73751fc655a924... [2] https://github.com/harfbuzz/harfbuzz/blob/970321db7bddbe8c579b73751fc655a924... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1055#note_11503
participants (5)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Huw Davies (＠huw)
-
Marvin -
Nikolay Sivov (＠nsivov)