Page MenuHomePhabricator

[compiler-rt][asan] Add support for desallocation of unhandled pointers
ClosedPublic

Authored by etienneb on Oct 25 2016, 9:03 AM.

Details

Summary

On windows 10, the ucrt DLL is performing allocations before the function hooking and there are multiple allocations not handled by Asan. When a free occur at the end of the process, asan is reporting desallocations not malloc-ed.

Event Timeline

etienneb updated this revision to Diff 75715.Oct 25 2016, 9:03 AM
etienneb retitled this revision from to [compiler-rt][asan] Add support for desallocation of unhandled pointers.
etienneb updated this object.
etienneb added a reviewer: rnk.
etienneb added subscribers: chrisha, llvm-commits.
rnk added inline comments.Oct 25 2016, 9:33 AM
lib/asan/asan_allocator.cc
560

IMO it's a bug if we get here and !PointerIsMine. The primary allocator check is pretty fast, but checking the secondary allocator is a linear search I think.

We can probably move this check to the HeapFree interceptor in asan_malloc_win.cc if we need it, though. That's probably only called by the CRT, which isn't usually hot. It also keeps Windows heap interception concerns out of the main allocator logic.

etienneb added inline comments.Oct 25 2016, 9:51 AM
lib/asan/asan_allocator.cc
560

Do we consider full coverage (catching every allocations) or not.

If not, then this make sense and we may need to figure out efficient way to run this test.

If we assume full coverage, then we should at least add a DCHECK here.
This is really error prone.

On windows, it's pretty hard to guarantee full coverage. Dynamic loading of DLL and the operating system may play in your back often.
I''m not convince which way is the best.

At least, we should make some benchmark on this.

kcc added a subscriber: kcc.Oct 25 2016, 9:52 AM

Yep. Whatever the actual problem is this patch is unlikely to be the right fix.

etienneb updated this revision to Diff 88007.Feb 10 2017, 8:52 AM

better validation, windows only

rnk@, is that what we talked about?

rnk added inline comments.Feb 13 2017, 1:02 PM
lib/asan/asan_allocator.cc
557

Reworded a little better:

// On Windows, uninstrumented DLLs may allocate memory before ASan hooks malloc. Don't report an invalid free in this case.
lib/asan/asan_internal.h
67

There are many ways an address can be "valid". Maybe IsSystemHeapAddress would be a better name?

lib/asan/asan_win.cc
282

This is too broad. This will suppress invalid free checks on any pointer from VirtualAlloc. Why not use HeapValidate? It appears to support checking a single block efficiently: https://msdn.microsoft.com/en-us/library/windows/desktop/aa366708(v=vs.85).aspx

test/asan/TestCases/Windows/virtual_memory.cc
1 ↗(On Diff #88007)

Use a test that looks more like this, it more closely matches the actual dbghelp.dll scenario where a DLL initializes before ASan and uses ucrtbase.dll.

// foo.cpp:
#include <memory>
std::unique_ptr<int> __declspec(dllexport) myglobal(new int(42));

// t.cpp:
#include <memory>
extern std::unique_ptr<int> __declspec(dllimport) myglobal;
int main() { printf("*myglobal: %d\n", *myglobal); }

$ clang-cl -EHsc foo.cpp -MD -LD -nologo
foo.cpp
   Creating library foo.lib and object foo.exp

$ clang-cl -fsanitize=address t.cpp foo.lib -MT
   Creating library t.lib and object t.exp

$ ./t.exe
...
etienneb added inline comments.Feb 14 2017, 8:36 AM
lib/asan/asan_win.cc
282

Are we assuming?

  1. no custom memory management (direct access to virtual-alloc/free)?
  2. no other heap (only the default process heap

And I'm not sure |lpMem| can be any arbitrary pointer within the memory chunk. I suspect it must be the begining of the block.

For this given snippet:

#include <windows.h>
#include <iostream>

int main(int argc, char* argv[]) {
  char *p1 = (char*)HeapAlloc(GetProcessHeap(), 0, 12);
  char *p2 = p1 + 7;

  BOOL b1 = HeapValidate(GetProcessHeap(), 0, NULL);
  BOOL b2 = HeapValidate(GetProcessHeap(), 0, p1);
  BOOL b3 = HeapValidate(GetProcessHeap(), 0, p2);
  std::cout << "result: " << b1 << " " << b2 << " " << b3 << std::endl;
}

The result is:

result: 1 1 0

HeapValidate is triggering a breakpoint with an invalid address:

_RtlpBreakPointHeap@4:
77CA06F9  mov         edi,edi  
77CA06FB  push        ebp  
77CA06FC  mov         ebp,esp  
77CA06FE  mov         eax,dword ptr fs:[00000018h]  
77CA0704  mov         eax,dword ptr [eax+30h]  
77CA0707  cmp         byte ptr [eax+2],0  
77CA070B  je          _RtlpBreakPointHeap@4+2Bh (77CA0724h)  
77CA070D  mov         eax,dword ptr [ebp+8]  
77CA0710  mov         byte ptr ds:[77CD92A5h],1  
77CA0717  mov         dword ptr ds:[77CD92A0h],eax  
77CA071C  int         3                                  <<---- debugger is breaking here
77CA071D  mov         byte ptr ds:[77CD92A5h],0  
77CA0724  pop         ebp  
77CA0725  ret         4
rnk added inline comments.Feb 14 2017, 10:12 AM
lib/asan/asan_win.cc
282

We only come here when someone calls free in the CRT. That means they must be using the system heap. ASan assumes it's the only code hotpatching the system heap. We only want to suppress invalid free reports on pointers to blocks of memory allocated on the system heap from before ASan initialized. Any other pointer should generate an invalid free report.

rnk edited edge metadata.Feb 14 2017, 10:16 AM

For this given snippet:
... snip

Looks right to me.

HeapValidate is triggering a breakpoint with an invalid address:
.. snip

That's annoying, but it only happens when you attach a debugger, and I still think this is the right thing to do. The universal CRT actually uses HeapValidate internally in some builds to select between different CRT heaps. Take a look at select_heap in WindowsSDK\10\Source\10.0.14393\ucrt\heap\free_base.cpp.

etienneb added inline comments.Feb 14 2017, 10:18 AM
lib/asan/asan_win.cc
282

You're right, this is for Free(...).
I'm gonna add a comment to |IsSystemHeapAddress |.

etienneb updated this revision to Diff 88538.Feb 15 2017, 7:56 AM
etienneb marked 6 inline comments as done.

rnk comments

PTAL.

Without the fix, the unittest is failing with:

% "D:\src\llvm\ninja\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Windows\Output\dll_heap_allocation.cc.tmp"
myglobal: 42
=================================================================
==17584==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x00add6f8 in thread T0
    #0 0x39e7e4 in __asan_wrap_HeapFree d:\src\llvm\llvm\projects\compiler-rt\lib\asan\asan_malloc_win.cc:181
    #1 0xf5e99fe in _stdio_common_vswprintf+0x12e (C:\Windows\system32\ucrtbase.DLL+0x100299fe)
    #2 0x77c094c4 in RtlIsCurrentThreadAttachExempt+0x5e (C:\Windows\SysWOW64\ntdll.dll+0x7dea94c4)
    #3 0x77c28ebd in LdrShutdownProcess+0x96 (C:\Windows\SysWOW64\ntdll.dll+0x7dec8ebd)
    #4 0x77c28e09 in RtlExitUserProcess+0x73 (C:\Windows\SysWOW64\ntdll.dll+0x7dec8e09)
    #5 0x775879c4 in ExitProcess+0x14 (C:\Windows\syswow64\kernel32.dll+0x7dd779c4)
    #6 0x3d28e8 in exit_or_terminate_process d:\th\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp:129
    #7 0x3d287e in common_exit d:\th\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp:269
    #8 0x3d2a63 in exit d:\th\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp:282
    #9 0x3b1159 in __scrt_common_main_seh f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:260
    #10 0x77583369 in BaseThreadInitThunk+0x11 (C:\Windows\syswow64\kernel32.dll+0x7dd73369)
    #11 0x77c09901 in RtlInitializeExceptionChain+0x62 (C:\Windows\SysWOW64\ntdll.dll+0x7dea9901)
    #12 0x77c098d4 in RtlInitializeExceptionChain+0x35 (C:\Windows\SysWOW64\ntdll.dll+0x7dea98d4)
rnk accepted this revision.Feb 15 2017, 11:22 AM

lgtm

@kcc, does this look like what we discussed?

This revision is now accepted and ready to land.Feb 15 2017, 11:22 AM
kcc accepted this revision.Feb 17 2017, 2:53 PM

LGTM with a comment nit.

lib/asan/asan_internal.h
67

Returns whether 'addr' is a valid allocated system heap block.
'addr' must point to the beginning of the block.

etienneb updated this revision to Diff 89218.Feb 21 2017, 8:10 AM
etienneb marked an inline comment as done.

fix kcc comments

etienneb closed this revision.Feb 21 2017, 8:21 AM