[PATCH 0/1] MR10612: gdi32/uniscribe: Fix heap buffer overflow in ScriptStringAnalyse().

[समीरसिंह Sameer Singh (＠ss141309)]

Hi,

I came across this bug while solving another issue.

Steps to reproduce:

1. Open wine notepad
2. Start spamming any character that requires mark_invalid_combinations() in the notepad, such as:

   َ       (0x064e Arabic Fatha)
3. At nearly 41 inserted Fatha, notepad will crash

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10612

[PATCH 1/1] gdi32/uniscribe: Fix heap buffer overflow in ScriptStringAnalyse().

[समीर सिंह Sameer Singh]

From: समीर सिंह Sameer Singh <[e-mail]>

Increase the glyph buffer allocation to 2 * cChar + 16 to accommodate the
worst case scenario in mark_invalid_combinations() where every character in
a run requires an inserted dotted circle.
---
 dlls/gdi32/uniscribe/usp10.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/gdi32/uniscribe/usp10.c b/dlls/gdi32/uniscribe/usp10.c
index 43afbaac125..3aea03b81f0 100644
--- a/dlls/gdi32/uniscribe/usp10.c
+++ b/dlls/gdi32/uniscribe/usp10.c
@@ -1994,7 +1994,7 @@ HRESULT WINAPI ScriptStringAnalyse(HDC hdc, const void *pString, int cString,
         {
             SCRIPT_CACHE *sc = (SCRIPT_CACHE*)&analysis->glyphs[i].sc;
             int cChar = analysis->pItem[i+1].iCharPos - analysis->pItem[i].iCharPos;
-            int numGlyphs = 1.5 * cChar + 16;
+            int numGlyphs = 2 * cChar + 16;
             WORD *glyphs = calloc(numGlyphs, sizeof(*glyphs));
             WORD *pwLogClust = calloc(cChar, sizeof(*pwLogClust));
             int *piAdvance = calloc(numGlyphs, sizeof(*piAdvance));
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/10612

[Aric Stewart (＠aricstewart)]

In general this looks good.  The issue is that we are guessing how many glyphs will result from the string and we where guessing a maximum of 1.5 glyphs per code point and you bump it to 2.  This will break if we find a substitution rule that results in more than 2 glyphs per code point and you generate a string of those unicode characters long enough.  A quick web search does not find a lot of examples of 3+ glyphs per code point so we are likely safe with this.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10612#note_135829

[PATCH 0/1] MR10612: gdi32/uniscribe: Fix heap buffer overflow in ScriptStringAnalyse(). - approved

[Aric Stewart (＠aricstewart)]

This merge request was approved by Aric Stewart.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10612

[Nikolay Sivov (＠nsivov)]

That's not how this estimated size works, I believe. It's the size of results glyph array, not necessarily a maximum length during intermediate substitution steps. In any case, since array sizes are passed to shaping logic, it should check for bounds. But I agree that "likely safe" approach is unfortunately better in this case, there is no quick fix and I'd rather reuse shaping logic from newer text API here, at some point.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10612#note_135847

[Huw Davies (＠huw)]

Why can't we check for `ScriptShape()` returning `E_OUTOFMEMORY` and grow the buffers if needed?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10612#note_137206

[समीरसिंह Sameer Singh (＠ss141309)]

We could do that, but wouldn't that approach be slower? We would have to wait for the function to return an `E_OUTOFMEMORY` and then allocate some new memory. The current approach does not have to do this.

BTW my initial way of solving the problem was exactly this, but I remember some slowdowns when spamming the offending character in the notepad.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10612#note_137321

[Huw Davies (＠huw)]

You'd start with a size large enough to work in most cases (even using your increased value would be fine).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10612#note_137322