[PATCH v2 0/1] MR7042: dwrite/tests: Avoid stack-use-after-scope in test_GetOverhangMetrics. (ASan)
This moves the stack variable to the outer scope, because it gets accessed in the call to `IDWriteTextLayout_Release` a few lines below. ``` ================================================================= ==1792==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7ffffe1fd0c0 at pc 0x6ffff84f74f6 bp 0x7ffffe1fcad0 sp 0x7ffffe1fcb18 READ of size 8 at 0x7ffffe1fd0c0 thread T0 0704:fixme:file:server_get_file_info Unsupported info class e #0 0x6ffff84f74f5 in IDWriteInlineObject_Release .../wine-build/build-asan-pe/64/obj\include\dwrite.h:2815:18 #1 0x6ffff84f74f5 in free_layout_range .../wine/dlls/dwrite/layout.c:2487:13 #2 0x6ffff84f74f5 in free_layout_ranges_list .../wine/dlls/dwrite/layout.c:2514:9 #3 0x6ffff84f74f5 in dwritetextlayout_Release .../wine/dlls/dwrite/layout.c:2949:9 #4 0x0001400ec179 in IDWriteTextLayout_Release .../wine-build/build-asan-pe/64/obj\include\dwrite.h:4211:12 #5 0x0001400ec179 in test_GetOverhangMetrics .../wine/dlls/dwrite/tests/layout.c:6288:5 #6 0x0001400ec179 in func_layout .../wine/dlls/dwrite/tests/layout.c:7118:5 #7 0x00014013a4bc in run_test .../wine/include/wine/test.h:765:5 #8 0x00014013a4bc in main .../wine/include/wine/test.h:884:12 #9 0x00014013c36f in mainCRTStartup .../wine/dlls/msvcrt/crt_main.c:58:11 #10 0x6ffffbdc4808 in BaseThreadInitThunk /usr/src/packages/BUILD\dlls/kernel32\thread.c:61:5 #11 0x6ffffaeffa1a in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x17000fa1a) Address 0x7ffffe1fd0c0 is located in stack of thread T0 at offset 1280 in frame #0 0x0001400a823f in func_layout .../wine/dlls/dwrite/tests/layout.c:7069 This frame has 224 object(s): [32, 392) 'metrics.i6891' (line 6965) [464, 472) 'inlineobj.i6892' (line 6966) [496, 520) 'line.i' (line 6967) [560, 568) 'format.i6893' (line 6968) ... [1136, 1144) 'layout.i5988' (line 6239) [1168, 1184) 'overhang_metrics.i' (line 6254) [1200, 1236) 'metrics.i5989' (line 6256) [1280, 1320) 'obj.i' (line 6257) <== Memory access at offset 1280 is inside this variable [1360, 1368) 'format2.i5721' (line 6136) [1392, 1400) 'layout.i5722' (line 6137) [1424, 1432) 'format.i5723' (line 6138) [1456, 1476) 'spacing.i5724' (line 6176) ... [10320, 10328) 'format.i' (line 1269) [10352, 10360) 'layout1.i' (line 1335) [10384, 10392) 'format1.i' (line 1336) [10416, 10424) 'format117.i' (line 1337) HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp, SEH and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-use-after-scope .../wine-build/build-asan-pe/64/obj\include\dwrite.h:2815:18 in IDWriteInlineObject_Release Shadow bytes around the buggy address: 0x7ffffe1fce00: f2 f2 f8 f2 f2 f2 f8 f2 f8 f2 f2 f2 f8 f2 f2 f2 0x7ffffe1fce80: f8 f2 f2 f2 f8 f8 f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 0x7ffffe1fcf00: f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 0x7ffffe1fcf80: f8 f8 f8 f8 f2 f2 f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 0x7ffffe1fd000: f8 f2 00 f2 f2 f2 00 f2 f2 f2 f8 f8 f2 f2 f8 f8 =>0x7ffffe1fd080: f8 f8 f8 f2 f2 f2 f2 f2[f8]f8 f8 f8 f8 f2 f2 f2 0x7ffffe1fd100: f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f8 0x7ffffe1fd180: f8 f2 f2 f2 f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 0x7ffffe1fd200: f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f8 0x7ffffe1fd280: f8 f8 f2 f2 f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 0x7ffffe1fd300: f8 f8 f8 f2 f2 f2 f2 f2 f8 f2 f2 f2 f8 f8 f8 f8 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==1792==ABORTING make: *** [Makefile:162532: dlls/dwrite/tests/x86_64-windows/layout.ok] Fehler 1 ``` -- v2: dwrite/tests: Allocate test inline objects dynamically. https://gitlab.winehq.org/wine/wine/-/merge_requests/7042
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/dwrite/tests/layout.c | 86 ++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 31 deletions(-) diff --git a/dlls/dwrite/tests/layout.c b/dlls/dwrite/tests/layout.c index f8ca4f250c4..4f5b88d8e8c 100644 --- a/dlls/dwrite/tests/layout.c +++ b/dlls/dwrite/tests/layout.c @@ -1077,6 +1077,20 @@ static const IDWriteTextRendererVtbl testrenderervtbl = { static IDWriteTextRenderer testrenderer = { &testrenderervtbl }; /* test IDWriteInlineObject */ +struct test_inline_obj +{ + IDWriteInlineObject IDWriteInlineObject_iface; + LONG refcount; + + DWRITE_INLINE_OBJECT_METRICS metrics; + DWRITE_OVERHANG_METRICS overhangs; +}; + +static inline struct test_inline_obj *impl_from_IDWriteInlineObject(IDWriteInlineObject *iface) +{ + return CONTAINING_RECORD(iface, struct test_inline_obj, IDWriteInlineObject_iface); +} + static HRESULT WINAPI testinlineobj_QI(IDWriteInlineObject *iface, REFIID riid, void **obj) { if (IsEqualIID(riid, &IID_IDWriteInlineObject) || IsEqualIID(riid, &IID_IUnknown)) { @@ -1091,12 +1105,19 @@ static HRESULT WINAPI testinlineobj_QI(IDWriteInlineObject *iface, REFIID riid, static ULONG WINAPI testinlineobj_AddRef(IDWriteInlineObject *iface) { - return 2; + struct test_inline_obj *obj = impl_from_IDWriteInlineObject(iface); + return InterlockedIncrement(&obj->refcount); } static ULONG WINAPI testinlineobj_Release(IDWriteInlineObject *iface) { - return 1; + struct test_inline_obj *obj = impl_from_IDWriteInlineObject(iface); + ULONG refcount = InterlockedDecrement(&obj->refcount); + + if (!refcount) + free(obj); + + return refcount; } static HRESULT WINAPI testinlineobj_Draw(IDWriteInlineObject *iface, @@ -1133,7 +1154,7 @@ static HRESULT WINAPI testinlineobj2_GetBreakConditions(IDWriteInlineObject *ifa return S_OK; } -static IDWriteInlineObjectVtbl testinlineobjvtbl = { +static const IDWriteInlineObjectVtbl testinlineobjvtbl = { testinlineobj_QI, testinlineobj_AddRef, testinlineobj_Release, @@ -1143,7 +1164,8 @@ static IDWriteInlineObjectVtbl testinlineobjvtbl = { testinlineobj_GetBreakConditions }; -static IDWriteInlineObjectVtbl testinlineobjvtbl2 = { +static const IDWriteInlineObjectVtbl testinlineobjvtbl2 = +{ testinlineobj_QI, testinlineobj_AddRef, testinlineobj_Release, @@ -1153,20 +1175,14 @@ static IDWriteInlineObjectVtbl testinlineobjvtbl2 = { testinlineobj2_GetBreakConditions }; -static IDWriteInlineObject testinlineobj = { &testinlineobjvtbl }; -static IDWriteInlineObject testinlineobj2 = { &testinlineobjvtbl }; -static IDWriteInlineObject testinlineobj3 = { &testinlineobjvtbl2 }; - -struct test_inline_obj +static struct test_inline_obj *create_test_inline_object(const IDWriteInlineObjectVtbl *vtable) { - IDWriteInlineObject IDWriteInlineObject_iface; - DWRITE_INLINE_OBJECT_METRICS metrics; - DWRITE_OVERHANG_METRICS overhangs; -}; + struct test_inline_obj *object = calloc(1, sizeof(*object)); -static inline struct test_inline_obj *impl_from_IDWriteInlineObject(IDWriteInlineObject *iface) -{ - return CONTAINING_RECORD(iface, struct test_inline_obj, IDWriteInlineObject_iface); + object->IDWriteInlineObject_iface.lpVtbl = vtable; + object->refcount = 1; + + return object; } static HRESULT WINAPI testinlineobj3_GetMetrics(IDWriteInlineObject *iface, DWRITE_INLINE_OBJECT_METRICS *metrics) @@ -1194,14 +1210,6 @@ static const IDWriteInlineObjectVtbl testinlineobjvtbl3 = { testinlineobj_GetBreakConditions, }; -static void test_inline_obj_init(struct test_inline_obj *obj, const DWRITE_INLINE_OBJECT_METRICS *metrics, - const DWRITE_OVERHANG_METRICS *overhangs) -{ - obj->IDWriteInlineObject_iface.lpVtbl = &testinlineobjvtbl3; - obj->metrics = *metrics; - obj->overhangs = *overhangs; -} - struct test_effect { IUnknown IUnknown_iface; @@ -2408,6 +2416,8 @@ static void test_GetClusterMetrics(void) 'g',0x0085,'h',0x2028,'i',0x2029,0xad,0xa,0}; static const WCHAR str3W[] = {0x2066,')',')',0x661,'(',0x627,')',0}; static const WCHAR str2W[] = {0x202a,0x202c,'a',0}; + + struct test_inline_obj *testinlineobj, *testinlineobj2, *testinlineobj3; DWRITE_INLINE_OBJECT_METRICS inline_metrics; DWRITE_CLUSTER_METRICS metrics[22]; DWRITE_TEXT_METRICS text_metrics; @@ -2423,6 +2433,13 @@ static void test_GetClusterMetrics(void) FLOAT width; HRESULT hr; + testinlineobj = create_test_inline_object(&testinlineobjvtbl); + ok(!!testinlineobj, "Failed to create test inline object.\n"); + testinlineobj2 = create_test_inline_object(&testinlineobjvtbl); + ok(!!testinlineobj2, "Failed to create test inline object.\n"); + testinlineobj3 = create_test_inline_object(&testinlineobjvtbl2); + ok(!!testinlineobj3, "Failed to create test inline object.\n"); + factory = create_factory(); hr = IDWriteFactory_CreateTextFormat(factory, L"Tahoma", NULL, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, @@ -2579,7 +2596,7 @@ static void test_GetClusterMetrics(void) range.startPosition = 0; range.length = 4; - hr = IDWriteTextLayout_SetInlineObject(layout, &testinlineobj, range); + hr = IDWriteTextLayout_SetInlineObject(layout, &testinlineobj->IDWriteInlineObject_iface, range); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); count = 0; @@ -2600,7 +2617,7 @@ static void test_GetClusterMetrics(void) /* now set two inline object for [0,1] and [2,3], both fail to report break conditions */ range.startPosition = 2; range.length = 2; - hr = IDWriteTextLayout_SetInlineObject(layout, &testinlineobj2, range); + hr = IDWriteTextLayout_SetInlineObject(layout, &testinlineobj2->IDWriteInlineObject_iface, range); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); count = 0; @@ -2658,8 +2675,7 @@ static void test_GetClusterMetrics(void) ok(hr == S_OK, "Failed to create text layout, hr %#lx.\n", hr); range.startPosition = 0; - range.length = ~0u; - hr = IDWriteTextLayout_SetInlineObject(layout, &testinlineobj3, range); + hr = IDWriteTextLayout_SetInlineObject(layout, &testinlineobj3->IDWriteInlineObject_iface, range); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); count = 0; @@ -2862,6 +2878,9 @@ static void test_GetClusterMetrics(void) IDWriteTextLayout_Release(layout); + IDWriteInlineObject_Release(&testinlineobj->IDWriteInlineObject_iface); + IDWriteInlineObject_Release(&testinlineobj2->IDWriteInlineObject_iface); + IDWriteInlineObject_Release(&testinlineobj3->IDWriteInlineObject_iface); IDWriteInlineObject_Release(trimm); IDWriteTextFormat_Release(format); IDWriteFactory_Release(factory); @@ -6234,12 +6253,16 @@ static void test_GetOverhangMetrics(void) { 16.0f, { 10.0f, 50.0f, 20.0f }, { -1.0f, 0.0f, -3.0f, 4.0f }, { -1.0f, 4.0f, -3.0f, 0.0f } }, { 15.0f, { 10.0f, 50.0f, 20.0f }, { -1.0f, 10.0f, 3.0f, -4.0f }, { -1.0f, 15.0f, 3.0f, -9.0f } }, }; + struct test_inline_obj *inline_obj; IDWriteFactory *factory; IDWriteTextFormat *format; IDWriteTextLayout *layout; HRESULT hr; UINT32 i; + inline_obj = create_test_inline_object(&testinlineobjvtbl3); + ok(!!inline_obj, "Failed to create test inline object.\n"); + factory = create_factory(); hr = IDWriteFactory_CreateTextFormat(factory, L"Tahoma", NULL, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, @@ -6254,9 +6277,9 @@ static void test_GetOverhangMetrics(void) DWRITE_OVERHANG_METRICS overhang_metrics; DWRITE_TEXT_RANGE range = { 0, 1 }; DWRITE_TEXT_METRICS metrics; - struct test_inline_obj obj; - test_inline_obj_init(&obj, &test->metrics, &test->overhang_metrics); + inline_obj->metrics = test->metrics; + inline_obj->overhangs = test->overhang_metrics; hr = IDWriteTextLayout_SetLineSpacing(layout, DWRITE_LINE_SPACING_METHOD_UNIFORM, test->metrics.height * 2.0f, test->uniform_baseline); @@ -6265,7 +6288,7 @@ static void test_GetOverhangMetrics(void) hr = IDWriteTextLayout_SetInlineObject(layout, NULL, range); ok(hr == S_OK, "Failed to reset inline object, hr %#lx.\n", hr); - hr = IDWriteTextLayout_SetInlineObject(layout, &obj.IDWriteInlineObject_iface, range); + hr = IDWriteTextLayout_SetInlineObject(layout, &inline_obj->IDWriteInlineObject_iface, range); ok(hr == S_OK, "Failed to set inline object, hr %#lx.\n", hr); hr = IDWriteTextLayout_GetMetrics(layout, &metrics); @@ -6285,6 +6308,7 @@ static void test_GetOverhangMetrics(void) overhang_metrics.right, overhang_metrics.bottom); } + IDWriteInlineObject_Release(&inline_obj->IDWriteInlineObject_iface); IDWriteTextLayout_Release(layout); IDWriteTextFormat_Release(format); IDWriteFactory_Release(factory); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7042
v2: - Replace my version by the better patch from Nikolay Sivov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7042#note_90885
On Thu Dec 19 13:16:29 2024 +0000, Nikolay Sivov wrote:
I suggest switching to allocated objects instead, so that we don't have to think about such issues going forward. Feel free to push this patch. [0001-dwrite-tests-Allocate-test-inline-objects-dynamically.txt](/uploads/3ad1231aae43e4fb33e1f029e1d32b79/0001-dwrite-tests-Allocate-test-inline-objects-dynamically.txt) Thank your for reviewing and creating a better patch. With it ASan also does no longer get triggered. I pushed it as v2.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7042#note_90886
This merge request was approved by Nikolay Sivov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7042
participants (3)
-
Bernhard Übelacker -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)