[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.
From: Bernhard Übelacker bernhardu@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@details@Concurrency@@QEAA@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(); }
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...).
On Fri Nov 8 11:46:37 2024 +0000, Piotr Caban wrote:
Please fix #if condition.
Sorry for this one, thanks for spotting.
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-win64Then 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.exeor with this additional [wine-patch-3](https://gitlab.winehq.org/bernhardu/wine/-/commit/1cbadc736b59128ee3eb28e1d2...)
WINEDLLOVERRIDES="notepad.exe=n" wine64 C:\\x86_64\\notepad.exeI 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`
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?
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(); } ```
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()`.
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 --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.
-
Bernhard Übelacker -
Bernhard Übelacker (＠bernhardu)
-
Marvin -
Piotr Caban (＠piotr)