[win/asan] Improve SharedReAlloc with HEAP_REALLOC_IN_PLACE_ONLY. #132558
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
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. |
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 ... |
bernhardu
force-pushed
the
HeapReAlloc_1
branch
2 times, most recently
from
43a7fe9 to
882cf04
Compare
|
Sorry for the delay. The main motivation for this pull request is to avoid to return a different pointer while IN_PLACE is requested. |
There was a problem hiding this comment.
My earlier comment stands:
"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)."
Would you consider placing the new behavior configurable via an ASAN_OPTION?
There was a problem hiding this comment.
What's the reason for doing the check here, instead of on line 367 (which would become redundant)?
There was a problem hiding this comment.
This is because line 367 is not reached. Instead the function is left via reallocFunc(hHeap, dwFlags, lpMem, dwBytes); in line 321.
There was a problem hiding this comment.
Sorry, correction, not line 321, the function is left in line 352, so line 367 is not reached.
There was a problem hiding this comment.
Thanks, I see. I had misunderstood the code.
|
@thurstond, sorry for not responding to this earlier.
In the previous version shrinking was possible, but either accesses wrote an "use-after-poison" message, or a later version took changes to the core asan allocator. I think these two approaches were not a reduction in bug detection.
From my point of view an application using HEAP_REALLOC_IN_PLACE_ONLY and receiving NULL is allowed to access the memory via the old pointer, as the allocation should stay unmodified in that case (see documentation, near HEAP_REALLOC_IN_PLACE_ONLY.
This is the intention of the last version to force the application into using using the failure path. I think the application has to be always prepared for receiving NULL when growing the allocation.
I did not yet consider making this configurable because in my opinion returning a differenct location with HEAP_REALLOC_IN_PLACE_ONLY is simply a bug. But if this is the desired way to proceed I would happily try to add this in the next push? |
There was a problem hiding this comment.
Thanks, I see. I had misunderstood the code.
There was a problem hiding this comment.
What if this were all replaced with return nullptr;'? Would that provide stronger protection, while still staying spec-compliant?
There was a problem hiding this comment.
Sorry for the delay, it took me a while to run some tests.
And I found following "real world" location as an example [1] in the Wine tree to fail when always returning nullptr with HEAP_REALLOC_IN_PLACE_ONLY.
So in the end it boils down to the question if an application is allowed to rely on at least shrinking to succeed?
What do you think?
There was a problem hiding this comment.
Thanks for investigating! Given this, I concur with the approach of your patch.
There was a problem hiding this comment.
|
@thurstond, sorry again for the delay and thanks for your patience. |
Since the new behavior is supposed to be more correct, we can try it without adding a configuration option. If we discover that users in the wild need the old behavior for compatibility, we can revert and/or add a configuration option. Is the patch ready to merge? |
I planned to do another push with the style thing and another check if the test is still really testing what I want to test, I hope that is ok. |
Sure, no rush! |
Currently with HEAP_REALLOC_IN_PLACE_ONLY a new allocation gets returned with the content copied from the original pointer, which gets freed. But applications may rely on HEAP_REALLOC_IN_PLACE_ONLY returning the same pointer as they give as input to e.g. RtlReAllocateHeap. If e.g. growing is not possible it fails without modifying the input pointer. Downside of this patch is, it won't detect accesses to the area getting "free" by a shrinking reallocation.
bernhardu
force-pushed
the
HeapReAlloc_1
branch
from
882cf04 to
14e71e2
Compare
I pushed another version which just removes the superfluous curly brackets. |
|
@bernhardu Thanks for the patch! Merged. |
* main: (1483 commits) [clang] fix error recovery for invalid nested name specifiers (llvm#156772) Revert "[lldb] Add count for errors of DWO files in statistics and combine DWO file count functions" (llvm#156777) AMDGPU: Add agpr variants of multi-data DS instructions (llvm#156420) [libc][NFC] disable localtime on aarch64/baremetal (llvm#156776) [win/asan] Improve SharedReAlloc with HEAP_REALLOC_IN_PLACE_ONLY. (llvm#132558) [LLDB] Make internal shell the default for running LLDB lit tests. (llvm#156729) [lldb][debugserver] Max response size for qSpeedTest (llvm#156099) [AMDGPU] Define 1024 VGPRs on gfx1250 (llvm#156765) [flang] Check for BIND(C) name conflicts with alternate entries (llvm#156563) [RISCV] Add exhausted_gprs_fprs test to calling-conv-half.ll. NFC (llvm#156586) [NFC] Remove trailing whitespaces from `clang/include/clang/Basic/AttrDocs.td` [lldb] Mark scripted frames as synthetic instead of artificial (llvm#153117) [docs] Refine some of the wording in the quality developer policy (llvm#156555) [MLIR] Apply clang-tidy fixes for readability-identifier-naming in TransformOps.cpp (NFC) [MLIR] Add LDBG() tracing to VectorTransferOpTransforms.cpp (NFC) [NFC] Apply clang-format to PPCInstrFutureMMA.td (llvm#156749) [libc] implement template functions for localtime (llvm#110363) [llvm-objcopy][COFF] Update .symidx values after stripping (llvm#153322) Add documentation on debugging LLVM. [lldb] Add count for errors of DWO files in statistics and combine DWO file count functions (llvm#155023) ...
|
Thanks for merging. |
4 participants
Currently with HEAP_REALLOC_IN_PLACE_ONLY a new allocation
gets returned with the content copied from the original pointer,
which gets freed.
But applications may rely on HEAP_REALLOC_IN_PLACE_ONLY returning
the same pointer as they give as input to e.g. RtlReAllocateHeap.
If e.g. growing is not possible it fails without
modifying the input pointer.
Downside of this patch is, it won't detect accesses to the area
getting "free" by a shrinking reallocation.