Skip to content
Snippets Groups Projects

msvcrt: Do not create in newer msvcrt versions a separate heap. (ASan)

Merged Bernhard Übelacker requested to merge bernhardu/wine:asan-msvcrt-heap into master
2 unresolved threads

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)?

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Bernhard Übelacker changed title from msvcrt: Do not create in newer msvcrt versions a separate heap. to msvcrt: Do not create in newer msvcrt versions a separate heap. (ASan)

    changed title from msvcrt: Do not create in newer msvcrt versions a separate heap. to msvcrt: Do not create in newer msvcrt versions a separate heap. (ASan)

  • **** requested review from @piotr

    requested review from @piotr

    • How did you use ASan in Wine? Did you recompile PE parts of Wine with llvm-mingw and the -fsanitize=address option? I've heard that recompiling the Unix parts with ASan enabled causes some preloader issues

    • Yes, this is a with a local llvm-mingw with this llvm-project-patch, to allow ASan to hotpatch more instructions.

      And a wine tree with at least wine-patch-1 and wine-patch-2.

      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

      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?

      Edited by Bernhard Übelacker
    • Forgot to mention there is also a llvm-symbolizer.exe needed to get nice stack traces.

      I had installed following file from llvm-project installed: LLVM-19.1.3-win64.exe

    • Another small note, I have the llvm-mingw added to the PATH before configure and build, without it looks like it does not call x86_64-w64-mingw32-gcc with full path in my terminal, therefore using the system installed real mingw-w64 binary.

    • Maybe this would be a better configure line, at least with this ccache is used later:

          export PATH=/home/bernhard/llvm-mingw_inst/bin:$PATH
      
          PKG_CONFIG_PATH=/usr/lib/x86_64-linux-gnu/pkgconfig \
          /home/bernhard/data/entwicklung/2024/wine/wine/configure \
              CC='ccache gcc' \
              CFLAGS='-g -O2' \
              CROSSCFLAGS='-g -O2 -fsanitize=address -Wno-pragma-pack -gdwarf-4' \
              x86_64_CC='ccache x86_64-w64-mingw32-gcc' \
              --prefix=/usr \
              --with-mingw \
              --without-oss \
              --enable-win64 \
              --disable-tests \

      Yet another update after this conversation in llvm-mingw.

      Latest ASan related wine patches collected in this branch.

      Was more or less still just able to use renamed ASan instrumented exe files inside a regular built Wineprefix.

      Edited by Bernhard Übelacker
    • Please register or sign in to reply
  • Piotr Caban
  • Piotr Caban
  • added 1 commit

    • 97cbdb71 - msvcrt: Do not create in newer msvcrt versions a separate heap.

    Compare with previous version

  • 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 and Deallocate.

  • Piotr Caban
    Piotr Caban @piotr started a thread on an outdated change in commit 97cbdb71
  • 1706 1706 tm.tm_year, tm.tm_hour, tm.tm_min, tm.tm_sec);
    1707 1707 }
    1708 1708
    1709 static void test__get_heap_handle(void)
    1710 {
    1711 void* (__cdecl *p__get_heap_handle)(void);
    1712 p__get_heap_handle = (void *)GetProcAddress( GetModuleHandleA("ucrtbase.dll"), "_get_heap_handle");
    1713 ok(p__get_heap_handle() == GetProcessHeap(), "Expected _get_heap_handle() to return GetProcessHeap()\n");
    • Comment on lines +1711 to +1713

      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 --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 changed this line in version 3 of the diff

      changed this line in version 3 of the diff

    • Thanks, I have pushed v3.

      Edited by Bernhard Übelacker
    • Please register or sign in to reply
  • added 1 commit

    • 4f6718d5 - msvcrt: Do not create in newer msvcrt versions a separate heap.

    Compare with previous version

  • v3:

    • Add prototype _get_heap_handle to malloc.h.
    • No loading of _get_heap_handle via GetProcAddress in msvcrt and ucrtbase tests.
  • added 1 commit

    • 0c703994 - msvcrt: Do not create in newer msvcrt versions a separate heap.

    Compare with previous version

  • v4:

    • Avoid warning "error: comparison between pointer and integer" by casting to HANDLE type.
  • Piotr Caban approved this merge request

    approved this merge request

  • added 15 commits

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Bernhard Übelacker mentioned in merge request !7041 (merged)

    mentioned in merge request !7041 (merged)

  • Please register or sign in to reply
    Loading