[PATCH v2 0/1] MR6791: msvcrt: Do not create in newer msvcrt versions a separate heap. (ASan)
A llvm-mingw built winver.exe with ASan enabled shows below error when exiting the application. This seems to be caused by libc++ using register_onexit_function before ASan has all function hooks in place, therefore allocates memory with plain calloc function. On process exit ASan uses in asan_allocator.cpp/Deallocate the function HeapValidate, which fails if msvcrt does not use the heap returned by GetProcessHeap(). ``` wine64 winver_asan.exe ================================================================= ==292==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x7ffffea05a10 in thread T0 0130:fixme:file:server_get_file_info Unsupported info class e #0 0x6ffffa809fe1 in free .../llvm-mingw/llvm-project/compiler-rt\lib/asan/asan_malloc_win.cpp:71:3 #1 0x6ffffeb048c5 in execute_onexit_table /home/bernhard/wine/dlls/msvcrt\exit.c:141:5 #2 0x6ffffbfb109e in _CRT_INIT .../llvm-mingw/mingw-w64/mingw-w64-crt/build-x86_64\../crt\crtdll.c:130:11 #3 0x6ffffbfb1316 in __DllMainCRTStartup .../llvm-mingw/mingw-w64/mingw-w64-crt/build-x86_64\../crt\crtdll.c:196:6 #4 0x6fffffc6d223 in call_dll_entry_point (C:\windows\system32\ntdll.dll+0x17002d223) #5 0x6fffffc71fdd in MODULE_InitDLL /home/bernhard/wine/dlls/ntdll\loader.c:1720:16 #6 0x6fffffc72601 in process_detach /home/bernhard/wine/dlls/ntdll\loader.c:1866:13 #7 0x6fffffc726f5 in RtlExitUserProcess /home/bernhard/wine/dlls/ntdll\loader.c:3893:5 #8 0x6fffffa8b899 in ExitProcess /home/bernhard/wine/dlls/kernel32\process.c:207:5 #9 0x6ffffeb054a7 in exit /home/bernhard/wine/dlls/msvcrt\exit.c:383:3 #10 0x000140001338 in mainCRTStartup /home/bernhard/wine/dlls/msvcrt\crt_main.c:60:5 #11 0x6fffffa98d58 in BaseThreadInitThunk /home/bernhard/wine/dlls/kernel32\thread.c:61:5 #12 0x6fffffc95afa in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x170055afa) Address 0x7ffffea05a10 is a wild pointer inside of access range of size 0x000000000001. SUMMARY: AddressSanitizer: bad-free /home/bernhard/wine/dlls/msvcrt\exit.c:141:5 in execute_onexit_table ==292==ABORTING ``` Please provide some guidance if this needs to be separated commits per msvcrt version, and if they are all needed (or more)? -- v2: msvcrt: Do not create in newer msvcrt versions a separate heap. https://gitlab.winehq.org/wine/wine/-/merge_requests/6791
From: Bernhard Übelacker <bernhardu(a)mailbox.org> A llvm-mingw built winver.exe with ASan enabled shows below error when exiting the application. This seems to be caused by libc++ using register_onexit_function before ASan has all function hooks in place, therefore allocates memory with plain calloc function. On process exit ASan uses in asan_allocator.cpp/Deallocate the function HeapValidate, which fails if msvcrt does not use the heap returned by GetProcessHeap(). --- dlls/msvcr100/tests/msvcr100.c | 8 ++++++++ dlls/msvcr110/tests/msvcr110.c | 8 ++++++++ dlls/msvcrt/heap.c | 6 ++++++ dlls/msvcrt/tests/heap.c | 8 ++++++++ dlls/ucrtbase/tests/misc.c | 8 ++++++++ 5 files changed, 38 insertions(+) diff --git a/dlls/msvcr100/tests/msvcr100.c b/dlls/msvcr100/tests/msvcr100.c index 7bde5c7935d..09aa8dcdbb0 100644 --- a/dlls/msvcr100/tests/msvcr100.c +++ b/dlls/msvcr100/tests/msvcr100.c @@ -239,6 +239,7 @@ static size_t (__cdecl *p___strncnt)(const char*, size_t); static int (__cdecl *p_strcmp)(const char *, const char *); static int (__cdecl *p_strncmp)(const char *, const char *, size_t); +static void* (__cdecl *p__get_heap_handle)(void); /* make sure we use the correct errno */ #undef errno @@ -282,6 +283,7 @@ static BOOL init(void) SET(p_strcmp, "strcmp"); SET(p_strncmp, "strncmp"); + SET(p__get_heap_handle, "_get_heap_handle"); if(sizeof(void*) == 8) { /* 64-bit initialization */ SET(pSpinWait_ctor_yield, "??0?$_SpinWait@$00(a)details@Concurrency@@QEAA(a)P6AXXZ@Z"); @@ -1152,6 +1154,11 @@ static void test_strcmp(void) ok( ret == 0, "wrong ret %d\n", ret ); } +static void test__get_heap_handle(void) +{ + ok(p__get_heap_handle() != GetProcessHeap(), "Expected _get_heap_handle() not to return GetProcessHeap()\n"); +} + START_TEST(msvcr100) { if (!init()) @@ -1173,4 +1180,5 @@ START_TEST(msvcr100) test_setlocale(); test___strncnt(); test_strcmp(); + test__get_heap_handle(); } diff --git a/dlls/msvcr110/tests/msvcr110.c b/dlls/msvcr110/tests/msvcr110.c index 35ba370bb49..a3e5b8cbdb7 100644 --- a/dlls/msvcr110/tests/msvcr110.c +++ b/dlls/msvcr110/tests/msvcr110.c @@ -54,6 +54,7 @@ static _Context* (__cdecl *p__Context__CurrentContext)(_Context*); static int (__cdecl *p_strcmp)(const char *, const char *); static int (__cdecl *p_strncmp)(const char *, const char *, size_t); +static void* (__cdecl *p__get_heap_handle)(void); #define SETNOFAIL(x,y) x = (void*)GetProcAddress(module,y) #define SET(x,y) do { SETNOFAIL(x,y); ok(x != NULL, "Export '%s' not found\n", y); } while(0) @@ -89,6 +90,7 @@ static BOOL init(void) SET(p_strcmp, "strcmp"); SET(p_strncmp, "strncmp"); + SET(p__get_heap_handle, "_get_heap_handle"); return TRUE; } @@ -307,6 +309,11 @@ static void test_strcmp(void) ok( ret == 0, "wrong ret %d\n", ret ); } +static void test__get_heap_handle(void) +{ + ok(p__get_heap_handle() == GetProcessHeap(), "Expected _get_heap_handle() to return GetProcessHeap()\n"); +} + START_TEST(msvcr110) { if (!init()) return; @@ -315,4 +322,5 @@ START_TEST(msvcr110) test___strncnt(); test_CurrentContext(); test_strcmp(); + test__get_heap_handle(); } diff --git a/dlls/msvcrt/heap.c b/dlls/msvcrt/heap.c index bf06c37e2c5..70e4ecb815c 100644 --- a/dlls/msvcrt/heap.c +++ b/dlls/msvcrt/heap.c @@ -829,13 +829,19 @@ int CDECL wmemcpy_s(wchar_t *dest, size_t numberOfElements, BOOL msvcrt_init_heap(void) { +#if _MSVCR_VER <= 100 heap = HeapCreate(0, 0, 0); +#else + heap = GetProcessHeap(); +#endif return heap != NULL; } void msvcrt_destroy_heap(void) { +#if _MSVCR_VER <= 100 HeapDestroy(heap); +#endif if(sb_heap) HeapDestroy(sb_heap); } diff --git a/dlls/msvcrt/tests/heap.c b/dlls/msvcrt/tests/heap.c index e6de728f2b7..ed4d501d1b5 100644 --- a/dlls/msvcrt/tests/heap.c +++ b/dlls/msvcrt/tests/heap.c @@ -523,10 +523,18 @@ static void test_calloc(void) free(ptr); } +static void test__get_heap_handle(void) +{ + void* (__cdecl *p__get_heap_handle)(void); + p__get_heap_handle = (void *)GetProcAddress( GetModuleHandleA("msvcrt.dll"), "_get_heap_handle"); + ok(p__get_heap_handle() != GetProcessHeap(), "Expected _get_heap_handle() not to return GetProcessHeap()\n"); +} + START_TEST(heap) { test_aligned(); test_sbheap(); test_malloc(); test_calloc(); + test__get_heap_handle(); } diff --git a/dlls/ucrtbase/tests/misc.c b/dlls/ucrtbase/tests/misc.c index ff1ed20f2f8..24d23c2be69 100644 --- a/dlls/ucrtbase/tests/misc.c +++ b/dlls/ucrtbase/tests/misc.c @@ -1706,6 +1706,13 @@ static void test_gmtime64(void) tm.tm_year, tm.tm_hour, tm.tm_min, tm.tm_sec); } +static void test__get_heap_handle(void) +{ + void* (__cdecl *p__get_heap_handle)(void); + p__get_heap_handle = (void *)GetProcAddress( GetModuleHandleA("ucrtbase.dll"), "_get_heap_handle"); + ok(p__get_heap_handle() == GetProcessHeap(), "Expected _get_heap_handle() to return GetProcessHeap()\n"); +} + START_TEST(misc) { int arg_c; @@ -1752,4 +1759,5 @@ START_TEST(misc) test_rewind_i386_abi(); #endif test_gmtime64(); + test__get_heap_handle(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6791
v2: - Fixed #if condition in msvcrt_destroy_heap. - Use `_get_heap_handle` instead of the indirect detection via `HeapValidate(GetProcessHeap(), ...)` For convenience this are the links into llvm-project, where this `HeapValidate(GetProcessHeap(), ...)` is called: [IsSystemHeapAddress](https://github.com/llvm/llvm-project/blob/71f82bba35c48eaf98c50aeeb4d2675156...) and [Deallocate](https://github.com/llvm/llvm-project/blob/6ce44266fc2d06dfcbefd8146279473cca...). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6791#note_86915
On Fri Nov 8 11:46:37 2024 +0000, Piotr Caban wrote:
Please fix #if condition. Sorry for this one, thanks for spotting.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6791#note_86916
On Fri Nov 8 17:22:02 2024 +0000, Bernhard Übelacker wrote:
Yes, this is a with a local llvm-mingw with this [llvm-project-patch](https://github.com/llvm/llvm-project/pull/113085), to allow ASan to hotpatch more instructions. And a wine tree with at least [wine-patch-1](https://gitlab.winehq.org/bernhardu/wine/-/commit/31295ffc453bc17011b58c28e0...) and [wine-patch-2](https://gitlab.winehq.org/bernhardu/wine/-/commit/8ec62b18c5d35eb1f5775784fa...). Configured with this options: ``` PKG_CONFIG_PATH=/usr/lib/x86_64-linux-gnu/pkgconfig \ CFLAGS='-g -O2' \ x86_64_CC='ccache /home/bernhard/llvm-mingw_inst/bin/x86_64-w64-mingw32-gcc' \ CROSSCFLAGS='-g -O2 -fsanitize=address -Wno-pragma-pack' \ x86_64_LDFLAGS='-lasan' \ /home/bernhard/wine/configure \ --prefix=/usr \ --with-mingw \ --without-oss \ --enable-win64 ``` Then putting `libc++.dll`, `libclang_rt.asan_dynamic-x86_64.dll` and `libunwind.dll` into PATH or copying them into a new directory. And calling this ASan-enabled executables like this, either with a renamed executable: ``` wine64 C:\\x86_64\\notepad-asan.exe ``` or with this additional [wine-patch-3](https://gitlab.winehq.org/bernhardu/wine/-/commit/1cbadc736b59128ee3eb28e1d2...) ``` WINEDLLOVERRIDES="notepad.exe=n" wine64 C:\\x86_64\\notepad.exe ``` I guess the wine-patch-1 is also something which should be handled in llvm-mingw, as I assume `x86_64-w64-mingw32-gcc` should behave the same as the mingw-w64 counterpart, which does not need the extra `-c`. And wine-patch-2 might be achieved by some other configure argument? Forgot to mention there is also a llvm-symbolizer.exe needed to get nice stack traces.
I had installed following file from [llvm-project](https://github.com/llvm/llvm-project/releases/tag/llvmorg-19.1.3) installed: `LLVM-19.1.3-win64.exe` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6791#note_86922
Piotr Caban (@piotr) commented about dlls/ucrtbase/tests/misc.c:
tm.tm_year, tm.tm_hour, tm.tm_min, tm.tm_sec); }
+static void test__get_heap_handle(void) +{ + void* (__cdecl *p__get_heap_handle)(void); + p__get_heap_handle = (void *)GetProcAddress( GetModuleHandleA("ucrtbase.dll"), "_get_heap_handle"); + ok(p__get_heap_handle() == GetProcessHeap(), "Expected _get_heap_handle() to return GetProcessHeap()\n");
Is there any reason to load `_get_heap_handle` dynamically in msvcrt and ucrtbase tests? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6791#note_86923
On Fri Nov 8 18:12:32 2024 +0000, Piotr Caban wrote:
Is there any reason to load `_get_heap_handle` dynamically in msvcrt and ucrtbase tests? No reason, except to have the four tests look kind of equal. Should I change these two tests to direct linking like this?
```diff diff --git a/dlls/msvcrt/tests/heap.c b/dlls/msvcrt/tests/heap.c index e6de728f2b7..e93f909be8d 100644 --- a/dlls/msvcrt/tests/heap.c +++ b/dlls/msvcrt/tests/heap.c @@ -524,4 +524,11 @@ static void test_calloc(void) } +void* __cdecl _get_heap_handle(void); + +static void test__get_heap_handle(void) +{ + ok(_get_heap_handle() != GetProcessHeap(), "Expected _get_heap_handle() not to return GetProcessHeap()\n"); +} + START_TEST(heap) { @@ -530,3 +537,4 @@ START_TEST(heap) test_malloc(); test_calloc(); + test__get_heap_handle(); } diff --git a/dlls/ucrtbase/tests/misc.c b/dlls/ucrtbase/tests/misc.c index ff1ed20f2f8..33fb326316a 100644 --- a/dlls/ucrtbase/tests/misc.c +++ b/dlls/ucrtbase/tests/misc.c @@ -1707,4 +1707,11 @@ static void test_gmtime64(void) } +void* __cdecl _get_heap_handle(void); + +static void test__get_heap_handle(void) +{ + ok(_get_heap_handle() == GetProcessHeap(), "Expected _get_heap_handle() to return GetProcessHeap()\n"); +} + START_TEST(misc) { @@ -1753,3 +1760,4 @@ START_TEST(misc) #endif test_gmtime64(); + test__get_heap_handle(); } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6791#note_86934
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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149556 Your paranoid android. === debian11 (32 bit report) === ucrtbase: misc.c:1713: Test failed: Expected _get_heap_handle() to return GetProcessHeap() === debian11 (32 bit ar:MA report) === ucrtbase: misc.c:1713: Test failed: Expected _get_heap_handle() to return GetProcessHeap() === debian11 (32 bit de report) === ucrtbase: misc.c:1713: Test failed: Expected _get_heap_handle() to return GetProcessHeap() === debian11 (32 bit fr report) === ucrtbase: misc.c:1713: Test failed: Expected _get_heap_handle() to return GetProcessHeap() === debian11 (32 bit he:IL report) === ucrtbase: misc.c:1713: Test failed: Expected _get_heap_handle() to return GetProcessHeap() === debian11 (32 bit hi:IN report) === ucrtbase: misc.c:1713: Test failed: Expected _get_heap_handle() to return GetProcessHeap() === debian11 (32 bit ja:JP report) === ucrtbase: misc.c:1713: Test failed: Expected _get_heap_handle() to return GetProcessHeap() === debian11 (32 bit zh:CN report) === ucrtbase: misc.c:1713: Test failed: Expected _get_heap_handle() to return GetProcessHeap() === debian11b (32 bit WoW report) === ucrtbase: misc.c:1713: Test failed: Expected _get_heap_handle() to return GetProcessHeap() === debian11b (64 bit WoW report) === ucrtbase: misc.c:1713: Test failed: Expected _get_heap_handle() to return GetProcessHeap()
On Fri Nov 8 11:46:37 2024 +0000, Piotr Caban wrote:
Please use `_get_heap_handle()` to check what heap is used. Thanks, I have pushed v2 using `_get_heap_handle()`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6791#note_86935
On Fri Nov 8 21:21:26 2024 +0000, Bernhard Übelacker wrote:
No reason, except to have the four tests look kind of equal. Should I change these two tests to direct linking like this? ```diff diff --git a/dlls/msvcrt/tests/heap.c b/dlls/msvcrt/tests/heap.c index e6de728f2b7..e93f909be8d 100644 --- a/dlls/msvcrt/tests/heap.c +++ b/dlls/msvcrt/tests/heap.c @@ -524,4 +524,11 @@ static void test_calloc(void) }
+void* __cdecl _get_heap_handle(void); + +static void test__get_heap_handle(void) +{ + ok(_get_heap_handle() != GetProcessHeap(), "Expected _get_heap_handle() not to return GetProcessHeap()\n"); +} + START_TEST(heap) { @@ -530,3 +537,4 @@ START_TEST(heap) test_malloc(); test_calloc(); + test__get_heap_handle(); } diff --git a/dlls/ucrtbase/tests/misc.c b/dlls/ucrtbase/tests/misc.c index ff1ed20f2f8..33fb326316a 100644 --- a/dlls/ucrtbase/tests/misc.c +++ b/dlls/ucrtbase/tests/misc.c @@ -1707,4 +1707,11 @@ static void test_gmtime64(void) }
+void* __cdecl _get_heap_handle(void); + +static void test__get_heap_handle(void) +{ + ok(_get_heap_handle() == GetProcessHeap(), "Expected _get_heap_handle() to return GetProcessHeap()\n"); +} + START_TEST(misc) { @@ -1753,3 +1760,4 @@ START_TEST(misc) #endif test_gmtime64(); + test__get_heap_handle(); } ``` Yes, please change the tests. Instead of defining function prototype in tests please add it to `malloc.h` header.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6791#note_86960
participants (3)
-
Bernhard Übelacker -
Marvin -
Piotr Caban (＠piotr)