[win/asan] Improve SharedReAlloc with HEAP_REALLOC_IN_PLACE_ONLY. #132558
Conversation
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- compiler-rt/test/asan/TestCases/Windows/rtlallocateheap_realloc_in_place.cpp compiler-rt/lib/asan/asan_allocator.cpp compiler-rt/lib/asan/asan_allocator.h compiler-rt/lib/asan/asan_malloc_win.cppView the diff from clang-format here.diff --git a/compiler-rt/lib/asan/asan_allocator.cpp b/compiler-rt/lib/asan/asan_allocator.cpp
index b4e7940bf..5f99c8ae8 100644
--- a/compiler-rt/lib/asan/asan_allocator.cpp
+++ b/compiler-rt/lib/asan/asan_allocator.cpp
@@ -527,10 +527,12 @@ struct Allocator {
bool UpdateDeallocationStack(uptr addr, BufferedStackTrace *stack) {
AsanChunk *m = GetAsanChunkByAddr(addr);
- if (!m) return false;
+ if (!m)
+ return false;
if (atomic_load(&m->chunk_state, memory_order_acquire) != CHUNK_ALLOCATED)
return false;
- if (m->Beg() != addr) return false;
+ if (m->Beg() != addr)
+ return false;
AsanThread *t = GetCurrentThread();
m->SetFreeContext(t ? t->tid() : kMainTid, StackDepotPut(*stack));
return true;
@@ -952,8 +954,8 @@ uptr AsanChunkView::AllocTid() const {
}
uptr AsanChunkView::FreeTid() const {
- //if (!IsQuarantined())
- // return kInvalidTid;
+ // if (!IsQuarantined())
+ // return kInvalidTid;
u32 tid = 0;
u32 stack = 0;
chunk_->GetFreeContext(tid, stack);
@@ -972,8 +974,8 @@ u32 AsanChunkView::GetAllocStackId() const {
}
u32 AsanChunkView::GetFreeStackId() const {
- //if (!IsQuarantined())
- // return 0;
+ // if (!IsQuarantined())
+ // return 0;
u32 tid = 0;
u32 stack = 0;
chunk_->GetFreeContext(tid, stack);
@@ -1130,7 +1132,7 @@ void asan_mz_force_unlock() SANITIZER_NO_THREAD_SAFETY_ANALYSIS {
instance.ForceUnlock();
}
-int asan_update_deallocation_context(void* addr, BufferedStackTrace *stack) {
+int asan_update_deallocation_context(void *addr, BufferedStackTrace *stack) {
return instance.UpdateDeallocationStack((uptr)addr, stack);
}
diff --git a/compiler-rt/lib/asan/asan_allocator.h b/compiler-rt/lib/asan/asan_allocator.h
index 7236543e3..f6cdb053f 100644
--- a/compiler-rt/lib/asan/asan_allocator.h
+++ b/compiler-rt/lib/asan/asan_allocator.h
@@ -292,7 +292,7 @@ uptr asan_mz_size(const void *ptr);
void asan_mz_force_lock();
void asan_mz_force_unlock();
-int asan_update_deallocation_context(void* addr, BufferedStackTrace *stack);
+int asan_update_deallocation_context(void *addr, BufferedStackTrace *stack);
void PrintInternalAllocatorStats();
void AsanSoftRssLimitExceededCallback(bool exceeded);
diff --git a/compiler-rt/lib/asan/asan_malloc_win.cpp b/compiler-rt/lib/asan/asan_malloc_win.cpp
index dbef1943a..3181bf213 100644
--- a/compiler-rt/lib/asan/asan_malloc_win.cpp
+++ b/compiler-rt/lib/asan/asan_malloc_win.cpp
@@ -14,13 +14,14 @@
#include "sanitizer_common/sanitizer_allocator_interface.h"
#include "sanitizer_common/sanitizer_platform.h"
#if SANITIZER_WINDOWS
-#include "asan_allocator.h"
-#include "asan_interceptors.h"
-#include "asan_internal.h"
-#include "asan_poisoning.h"
-#include "asan_stack.h"
-#include "interception/interception.h"
-#include <stddef.h>
+# include <stddef.h>
+
+# include "asan_allocator.h"
+# include "asan_interceptors.h"
+# include "asan_internal.h"
+# include "asan_poisoning.h"
+# include "asan_stack.h"
+# include "interception/interception.h"
// Intentionally not including windows.h here, to avoid the risk of
// pulling in conflicting declarations of these functions. (With mingw-w64,
@@ -324,7 +325,6 @@ void *SharedReAlloc(ReAllocFunction reallocFunc, SizeFunction heapSizeFunc,
}
if (ownershipState == ASAN && !only_asan_supported_flags) {
-
if (dwFlags & HEAP_REALLOC_IN_PLACE_ONLY) {
size_t old_usable_size = asan_malloc_usable_size(lpMem, pc, bp);
if (dwBytes == old_usable_size) {
@@ -335,10 +335,12 @@ void *SharedReAlloc(ReAllocFunction reallocFunc, SizeFunction heapSizeFunc,
return nullptr;
} else {
// poison just the "freed" part
- uptr aligned_ptr = RoundUpTo((uptr)((char *)lpMem + dwBytes), ASAN_SHADOW_GRANULARITY);
+ uptr aligned_ptr = RoundUpTo((uptr)((char *)lpMem + dwBytes),
+ ASAN_SHADOW_GRANULARITY);
uptr aligned_size = RoundUpTo(dwBytes, ASAN_SHADOW_GRANULARITY);
PoisonShadow(aligned_ptr, aligned_size, kAsanHeapFreeMagic);
- // unpoison the lower part, it could be poisoned by a previous realloc in place
+ // unpoison the lower part, it could be poisoned by a previous realloc
+ // in place
__asan_unpoison_memory_region((char *)lpMem, dwBytes);
__asan::asan_update_deallocation_context(lpMem, &stack);
return lpMem;
diff --git a/compiler-rt/test/asan/TestCases/Windows/rtlallocateheap_realloc_in_place.cpp b/compiler-rt/test/asan/TestCases/Windows/rtlallocateheap_realloc_in_place.cpp
index 5773bca4b..1f1e3ccf6 100644
--- a/compiler-rt/test/asan/TestCases/Windows/rtlallocateheap_realloc_in_place.cpp
+++ b/compiler-rt/test/asan/TestCases/Windows/rtlallocateheap_realloc_in_place.cpp
@@ -41,7 +41,7 @@ int main() {
buffer = (char *)RtlAllocateHeap_ptr(GetProcessHeap(), 0, 23),
ret = RtlReAllocateHeap_ptr(GetProcessHeap(), HEAP_REALLOC_IN_PLACE_ONLY,
- buffer, 7);
+ buffer, 7);
if (!ret) {
puts("returned nullptr");
}
@@ -51,7 +51,7 @@ int main() {
// CHECK: Okay 6
ret = RtlReAllocateHeap_ptr(GetProcessHeap(), HEAP_REALLOC_IN_PLACE_ONLY,
- buffer, 15);
+ buffer, 15);
if (!ret) {
puts("returned nullptr");
}
|
|
Hello, I wonder if such a patch would be acceptable? Also I am not sure which configurations this should be tested before final submission? I found also most heap(re)alloc or rtlallocateheap tests contain an |
bernhardu
force-pushed
the
HeapReAlloc_1
branch
from
213f036 to
bfcb81b
Compare
|
Just corrected the clang-format. Another point I forgot to mention, this currently creates an issue when running with ASAN_OPTIONS containing |
|
To the question where this should be tested before submission, I wondered why the "Windows Premerge Check" did not run the tests. But then I found about Is there any other way to submit a test to some buildbot or similar? |
I'm not aware of any such setup unfortunately... But I use a custom github actions setup for test running various things on the public github actions runners - see mstorsjo@gha-mingw-compiler-rt. I pushed a combination of that with this test branch, which should produce results at https://github.com/mstorsjo/llvm-project/actions/runs/14078778580. It should probably be possible to do a similar setup with clang-cl as well; it's mainly a question if it can be built with a recent enough separate build of clang (so one can build just compiler-rt), or if it requires a full build of clang+compiler-rt at the same time (which takes a fair bit of time on the github actions runners). |
|
Hello everyone, |
llvmbot
added
compiler-rt
compiler-rt:asan
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: None (bernhardu) ChangesThis patch allows reallocations in place if the size is below or equal to the initial allocated size. Currently it prints only a "use-after-poison" message, not a proper "heap-buffer-overflow" with a hint to a reallocation. Full diff: https://github.com/llvm/llvm-project/pull/132558.diff 2 Files Affected:
diff --git a/compiler-rt/lib/asan/asan_malloc_win.cpp b/compiler-rt/lib/asan/asan_malloc_win.cpp
index 3278f07219876..4a8eb0acb174e 100644
--- a/compiler-rt/lib/asan/asan_malloc_win.cpp
+++ b/compiler-rt/lib/asan/asan_malloc_win.cpp
@@ -323,12 +323,24 @@ void *SharedReAlloc(ReAllocFunction reallocFunc, SizeFunction heapSizeFunc,
}
if (ownershipState == ASAN && !only_asan_supported_flags) {
+ size_t old_usable_size = asan_malloc_usable_size(lpMem, pc, bp);
+ if (dwFlags & HEAP_REALLOC_IN_PLACE_ONLY) {
+ if (dwBytes >= 1 && dwBytes <= old_usable_size) {
+ if (dwBytes < old_usable_size) {
+ __asan_poison_memory_region((char *)lpMem + dwBytes,
+ old_usable_size - dwBytes);
+ }
+ __asan_unpoison_memory_region((char *)lpMem, dwBytes);
+ return lpMem;
+ } else {
+ return nullptr;
+ }
+ }
+
// Conversion to unsupported flags allocation,
// transfer this allocation back to the original allocator.
void *replacement_alloc = allocFunc(hHeap, dwFlags, dwBytes);
- size_t old_usable_size = 0;
if (replacement_alloc) {
- old_usable_size = asan_malloc_usable_size(lpMem, pc, bp);
REAL(memcpy)(replacement_alloc, lpMem,
Min<size_t>(dwBytes, old_usable_size));
asan_free(lpMem, &stack, FROM_MALLOC);
@@ -348,6 +360,7 @@ void *SharedReAlloc(ReAllocFunction reallocFunc, SizeFunction heapSizeFunc,
}
// asan_realloc will never reallocate in place, so for now this flag is
// unsupported until we figure out a way to fake this.
+ // Small exception when shrinking or staying below the inital size, see above.
if (dwFlags & HEAP_REALLOC_IN_PLACE_ONLY)
return nullptr;
diff --git a/compiler-rt/test/asan/TestCases/Windows/rtlallocateheap_realloc_in_place.cpp b/compiler-rt/test/asan/TestCases/Windows/rtlallocateheap_realloc_in_place.cpp
new file mode 100644
index 0000000000000..ffe62b7c30723
--- /dev/null
+++ b/compiler-rt/test/asan/TestCases/Windows/rtlallocateheap_realloc_in_place.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cl_asan %Od %s %Fe%t %MD
+// RUN: %env_asan_opts=windows_hook_rtl_allocators=true:halt_on_error=false not %run %t 2>&1 | FileCheck %s
+
+#include <stdio.h>
+#include <windows.h>
+
+using AllocateFunctionPtr = PVOID(__stdcall *)(PVOID, ULONG, SIZE_T);
+using ReAllocateFunctionPtr = PVOID(__stdcall *)(PVOID, ULONG, PVOID, SIZE_T);
+using FreeFunctionPtr = PVOID(__stdcall *)(PVOID, ULONG, PVOID);
+
+int main() {
+ HMODULE NtDllHandle = GetModuleHandle("ntdll.dll");
+ if (!NtDllHandle) {
+ puts("Couldn't load ntdll??");
+ return -1;
+ }
+
+ auto RtlAllocateHeap_ptr =
+ (AllocateFunctionPtr)GetProcAddress(NtDllHandle, "RtlAllocateHeap");
+ if (RtlAllocateHeap_ptr == 0) {
+ puts("Couldn't RtlAllocateHeap");
+ return -1;
+ }
+
+ auto RtlReAllocateHeap_ptr =
+ (ReAllocateFunctionPtr)GetProcAddress(NtDllHandle, "RtlReAllocateHeap");
+ if (RtlReAllocateHeap_ptr == 0) {
+ puts("Couldn't find RtlReAllocateHeap");
+ return -1;
+ }
+
+ auto RtlFreeHeap_ptr =
+ (FreeFunctionPtr)GetProcAddress(NtDllHandle, "RtlFreeHeap");
+ if (RtlFreeHeap_ptr == 0) {
+ puts("Couldn't RtlFreeHeap");
+ return -1;
+ }
+
+ char *buffer;
+ buffer = (char *)RtlAllocateHeap_ptr(GetProcessHeap(), 0, 48),
+
+ RtlReAllocateHeap_ptr(GetProcessHeap(), HEAP_REALLOC_IN_PLACE_ONLY, buffer,
+ 16);
+ buffer[15] = 'a';
+ puts("Okay 15");
+ fflush(stdout);
+ // CHECK: Okay 15
+
+ RtlReAllocateHeap_ptr(GetProcessHeap(), HEAP_REALLOC_IN_PLACE_ONLY, buffer,
+ 32);
+ buffer[31] = 'a';
+ puts("Okay 31");
+ fflush(stdout);
+ // CHECK: Okay 31
+
+ buffer[32] = 'a';
+ // CHECK: AddressSanitizer: use-after-poison on address [[ADDR:0x[0-9a-f]+]]
+ // CHECK: WRITE of size 1 at [[ADDR]] thread T0
+
+ RtlFreeHeap_ptr(GetProcessHeap(), 0, buffer);
+}
|
Nice! (Btw I see that your actions jobs still are named |
|
Hello everyone, any opinions on the patch? |
|
Would this approach in general make bug detection worse? The existing behavior of realloc always returning a new pointer (with the old memory marked inaccessible) can catch erroneous code that assumes the realloc is in place (or worse, inconsistently uses both the old pointer and the return value of realloc).
This will be confusing to users and could lead them on a wild good chase, looking for bugs in poisoning. |
|
Thanks for having a look.
I will try to improve the message and try to avoid the bare "use-after-poison".
I am a little confused now - when I read the documentation to HeapReAlloc I understand the paragraph of HEAP_REALLOC_IN_PLACE_ONLY as it is not allowed to return a different pointer. And if resize cannot be done in place it has to fail e.g return NULL. |
Sorry, my bad. I wasn't familiar with the HEAP_REALLOC_IN_PLACE_ONLY API. Thanks for the pointer! After reading it, I still wonder whether implementing this will reduce bug detection. For example, some code might incorrectly not expect NULL to be returned (even though the allocator is allowed to do so), or perhaps returning NULL will force the user code to call realloc without HEAP_REALLOC_IN_PLACE_ONLY (thereby allowing stronger use-after-free detection). |
I made now a bigger modification, which still tries to leave the chunk in an allocated state. But to show the stack of the partially free needed disabling a few checks An example output of the test is here, would that be usable? |
I'm kind of nervous that this patch now involves changes to the core ASan allocator. Indeed, it breaks Linux tests: https://buildkite.com/llvm-project/github-pull-requests/builds/184040#0197181e-8ad6-4377-bf55-de70d4efb035
"partially freed" (as opposed to "freed") is an unnecessary and potentially confusing distinction IMO. The stack trace will already show it is from realloc. |
This patch allows reallocations in place if the size is below or equal to the initial allocated size.
I added this "partially" because the lower part of the chunk is still valid, it is just the upper part which got "freed". Thanks for looking into it, I fear a chunk which is allocated and "freed" is not fitting the asan_allocator, maybe splitting the chunk into an allocated and into a freed one would be possible ... |
This patch allows reallocations in place if the size is below or equal to the initial allocated size.